Resolve ResourceKeyInvalidException

[Wealthyturtle]

Description

Fixes #2412 by adding a regex check each iteration of the sound array

---

Target Minecraft Versions: Any
Requirements: None
Related Issues: #2412

[Blueyescat]

1. The regex forces the sound name to be only one character
2. The / character needs to be escaped using \ in regex, and this also needs to be escaped using another \ because java.
3. The check should only be done if soundEnum is null
4. The sound name should be converted to lower case before the check
5. if( -> if ( (code style)
6. Add a white space after // (code style)

Final regex: [a-z0-9\/._-]+ in a java string: [a-z0-9\\/._-]+
https://regex101.com

[Wealthyturtle]

1. The regex forces the sound name to be only one character
2. The / character needs to be escaped using \ in regex, and this also needs to be escaped using another \ because java.
3. The check should only be done if soundEnum is null
4. The sound name should be converted to lower case before the check
5. if( -> if ( (code style)
6. Add a white space after // (code style)

Final regex: [a-z0-9\/._-]+ in a java string: [a-z0-9\\/._-]+
https://regex101.com

Thanks for the feedback. I've modified the regex string and placed it in SOUND_VALID_CHARACTERS to reduce duplication of the regex string (in case Mojang ever decides to change the rule), and I've also converted the individual sound Strings to lowercase beforehand in addition to following the code style.

[Blueyescat]

But you done it wrong, why would it convert to lower case and then to upper case again

---

However, actually just adding a try catch should be faster, because no additional regex:

// It may throw an exception if there are invalid characters in the sound name
try {
	p.playSound(location, sound, category, volume, pitch);
} catch (Exception ignored) {}

[Wealthyturtle]

So sorry, forgot about the inefficiency of swapping cases twice... I've swapped from the regex checks to the exception catch ahhh

[bensku]

Swallowing all exceptions thrown by playSound is not acceptable. Some of them might be important for users to see.

[Blueyescat]

@bensku currently there is no an exception by Spigot, but true, if it ignored the exception thrown by Minecraft we couldn't know the problem. But we can't catch that specific exception, also i don't think this is a good reason to add another format check. Can we make it so it will print the exception if debug mode is on?

[bensku]

Requirements for sound names are documented in Minecraft Wiki. Validating against them should be enough.

[Blueyescat]

I'm talking about unnecessary little performance loss

[Wealthyturtle]

Reverted back to Regex as requested

[FranKusmiruk]

I'm talking about unnecessary little performance loss

Exceptions should be handled when possible, catching them is not always ideal. And the performance isn't something to worry about in this specific case, if anything, a compiled pattern could be used.

[Blueyescat]

It will run an additional regex every time you play a sound, for no reason. You would not do this in Java, you would store the correct sound name. The exception doesn’t exist in the API anyways, so not Skript’s problem. The try/catch method should be used. Make it print the exception if in debug mode if you want..

[Wealthyturtle]

How about this, it catches all Exceptions, but once it catches an Exception, it gets the class of the Exception, and converts the class into a String. If it contains “ResourceKeyInvalidException” with the exact error message, then it’ll ignore it, else it will throw it back out? That way, no use of NMS, while still handling only the ResourceKeyInvalidException and not messing around with any other exceptions?

[Matocolotoe]

Why converting the class name into a string
Also you still swap cases twice...

[Wealthyturtle]

In my original code (the first commit), I only swapped cases once - for the enum parsing, but otherwise whatever sound name the user provides will be used. The issue was that’s Skript would display the error of ResourceKeyInvalidException when a name was deemed invalid by Minecraft, and that’s not what should occur (Skript should suppress that exception and that exception only).

Due to the coding convention rules on no NMS imports, and the exception being thrown from Minecraft itself (and not Spigot/Paper), there’s no real way to catch only that exception without using Class -> String conversion.

Ultimately, there are 2 separate issues at play here.

Firstly, as bensku and FranKusmiruk have mentioned earlier, it is not appropriate for Skript to ignore all exceptions, hence I thought of that Class -> String converter for handling the exception.

Note: The alternative to "directly" handling the Exception is to prevent it from ever occurring in the first place via a Regex check. However, this also leads to the issue of double Regex being performed, once by Skript and another by Minecraft itself. So, I believe that catching the Exception, then getting it's Class in a String format is likely more efficient (feel free to correct me).

Secondly, as for the inefficiency of double case conversion, it doesn’t need to be done if we just leave it up to the user to provide the specific sound name that Minecraft expects (without any intermediate case changing by Skript), otherwise don’t we always have to do double case conversion? Once for the enum parsing (uppercase) and another for the sound value passing (lowercase).