Removal of Username Validation in Context Resolver

Currently the resolver for online player validates the input username against a hardcoded pattern:

commands/bukkit/src/main/java/co/aikar/commands/ACFBukkitUtil.java

Lines 324 to 326 in 9ededdf

    
           public static boolean isValidName(String name) { 
        
               return name != null && !name.isEmpty() && ACFPatterns.VALID_NAME_PATTERN.matcher(name).matches(); 
        
           }

Which only serves to allow for a custom message saying "is not a valid username":

commands/bukkit/src/main/java/co/aikar/commands/ACFBukkitUtil.java

Lines 272 to 275 in 9ededdf

    
           if (!isValidName(name)) { 
        
               issuer.sendError(MinecraftMessageKeys.IS_NOT_A_VALID_NAME, "{name}", name); 
        
               return null; 
        
           }

This creates an issue for servers that allow for "invalid" usernames, ex. Geyser + Floodgate.

A useful option would be for the framework to either bypass this check in the case of perform-username-validation being set to false, or simply ditch it entirely?

I mean, is there any logical reason to do such validation?

One reason I can think of is to avoid unnecessary queries to Mojang servers.

One reason I can think of is to avoid unnecessary queries to Mojang servers.

What? If it's an online player resolver, why would it query mojang servers?

I believe this validation also happens for offline players. (I was thinking of that)

I believe this validation also happens for offline players. (I was thinking of that)

The code I embedded, from the OnlinePlayer context resolver, is the only place this is used.

(edit: I was wrong, my find usages failed me, it is in fact also used in the OfflinePlayer resolver)

(edit: but still, the path to get to the usage is different, and this check can safely be removed from the findPlayerSmart method, which only deals with online Player objects)

Removing the check from OnlinePlayer is not the best solution.
OfflinePlayer would still block the different nicknames.

IMO the pattern for valid names should be customizable.

IMO the pattern for valid names should be customizable.

That would be an entirely new mess though, I feel like it's outside of the scope of the framework to validate names at all.

Resolvers should resolve. Would be much simpler if it simply did the lookups.

Resolvers should resolve.

According to this logic, an integer resolver shouldn't validate whether the input is a number?

A number is strictly defined. You cannot define a username like that.

I guess websites shouldn't validate usernames or emails in their forms then. :)

I guess websites shouldn't validate usernames or emails in their forms then. :)

An email address is strictly defined. Minecraft username not. Servers might allow usernames with different characters. Or in some cases, require it. (Geyser)

I guess websites shouldn't validate usernames or emails in their forms then. :)

An email address is strictly defined. Minecraft username not. Servers might allow usernames with different characters. Or in some cases, require it. (Geyser)

mojang has strict criteria however

I guess websites shouldn't validate usernames or emails in their forms then. :)

An email address is strictly defined. Minecraft username not. Servers might allow usernames with different characters. Or in some cases, require it. (Geyser)

mojang has strict criteria however

Which does not apply to whackies like Geyser. And as much as I hate it, about half of your plugin users are going to be offline mode servers.

about half of your plugin users are going to be offline mode servers.

That's not a reason to support that. You can write your own context resolver for those or you can PR some neat stuff for that. Removing the pattern will fix those offline problems but as a online mode user I want the name validation there.

Why is everyone ignoring the geyser point? ._.
Having this behavior customizable (at least) would be nice.

I will look into making the name validation configurable. What that ideally looks like - needs investigated yet (haven't had time since this issue was opened)

I think it is perfectly fine to have the default behavior validate the names since from the Mojang level there is an expected pattern of names that are possible (I know there have been holes in the name length with the occasional 1 or 2 letter names). I also acknowledge the possibility of things like Geyser which could give impossible names hence why I am okay with us having an easy to configure option to change this validation.

In general, I think the ability to configure aspects of the framework is a good thing (within reason). Something like context resolvers, if you don't like the packaged resolver - there ought to be a way to easily replace it with your own. Not everything is going to be one size fits all. If there is a reasonable use-case for configuring something - I will support it as default or at least configurable. Naturally, some things are difficult to configure and that should be weighed appropriately in picking how/when to configure things.

Hope that clears the air a bit...

Resolvers should resolve.

According to this logic, an integer resolver shouldn't validate whether the input is a number?

I mean... it doesn't... it just attempts to resolve the number from the input string.

Also keep in mind here:

I guess websites shouldn't validate usernames or emails in their forms then. :)

An email address is strictly defined. Minecraft username not. Servers might allow usernames with different characters. Or in some cases, require it. (Geyser)

mojang has strict criteria however

This is not a resolver for Usernames..... This is a resolver for OnlinePlayer. And it fails to recognize that a Player... that is Online... has a certain name.

In any case, this is a bug.

I think it is perfectly fine to have the default behavior validate the names since from the Mojang level there is an expected pattern of names that are possible (I know there have been holes in the name length with the occasional 1 or 2 letter names). I also acknowledge the possibility of things like Geyser which could give impossible names hence why I am okay with us having an easy to configure option to change this validation.

In general, I think the ability to configure aspects of the framework is a good thing (within reason). Something like context resolvers, if you don't like the packaged resolver - there ought to be a way to easily replace it with your own. Not everything is going to be one size fits all. If there is a reasonable use-case for configuring something - I will support it as default or at least configurable. Naturally, some things are difficult to configure and that should be weighed appropriately in picking how/when to configure things.

I think it makes more sense to not validate an input string when I'm not asking for an input string.

I'm asking for a player, and if I type the name of a player that is online on the server, it should give me that player.

If someone were to be writing this command by hand, they would probably not be passing this input string through validation for the convenience of sending a custom message. They would try to lookup the player, and if not found, send a player not found message. It's that simple.

In any case, this is a bug.

I think it makes more sense to not validate an input string when I'm not asking for an input string.

I'm asking for a player, and if I type the name of a player that is online on the server, it should give me that player.

If someone were to be writing this command by hand, they would probably not be passing this input string through validation for the convenience of sending a custom message. They would try to lookup the player, and if not found, send a player not found message. It's that simple.

I disagree with the word choice of calling it a bug. And I also disagree with your assessment of what resolvers can/should do. Your use-case is nonstandard (although growing). But as mentioned, I will be looking into it when I have sufficient time to address it (workday is not quite over yet and have other things going IRL). There really is no reason to split hairs about your argument as I have already agreed to implement an option for you to disable/modify it.

As part of resolving this issue - it kinda sounds like a method of overriding the packaged resolver might be better for what you desire. I shall need time to open up the code and thoroughly analyze things to know what is desirable for the project to have as a default. An advantage to having a more precise check like name validation is to have more descriptive feedback about what is wrong. There is no malicious intent behind blocking out these invalid names. The command framework pre-dates geyser/floodgate junk. There is no grand conspiracy to fight here.

I disagree with the word choice of calling it a bug.

The resolver is meant to find a player who is online using the provided name. If it is not doing that, it's a bug.

Please do explain how it not doing the one thing it's supposed to be doing isn't a bug.

Your use-case is nonstandard (although growing).

My use case is simply mapping an input string to a player object. It should not be up to the framework to tell me that the input is invalid.

An advantage to having a more precise check like name validation is to have more descriptive feedback about what is wrong. There is no malicious intent behind blocking out these invalid names. The command framework pre-dates geyser/floodgate junk. There is no grand conspiracy to fight here.

I'm sort of at a lost when it comes to this response. Who is implying malicious intent? And who is implying a grand conspiracy?? This sort of seems disingenuous and condescending.

I am fully aware of what the framework is doing, I literally posted the code that does it. I am simply pointing out that it shouldn't. Whether or not it predates geyser does not justify a useless validation check.

Error: *Sxtanna is not a valid username.

is not a better or more descriptive response than

No player matching *Sxtanna is connected to this server.

If we are talking about what the framework should do. I would lean towards minimal to no resolvers being bundled in. But they are for convenience. That is what I am trying to convey. That there will always be a problem with how someone else implements a resolver. To me, these resolvers are not part of the framework, and shouldn't be a point of controversy because one should have the ability to easily override them. If that ability to override a resolver is missing then that is a significant issue. The grand conspiracy is why any of this matters? Why are people getting sweaty about something this minor? I only replied to say something was going to be done about it.... to try and avoid my emails getting further spammed about this... haha. Mission Failed.

I have moved the invalid name feedback to after the player is unable to be found. I understand this behavior is going to still be confusing/misleading to a server using geyser/floodgate which it isn't actually invalid.

I think this is a good middle ground for the default behavior. There are two solutions for someone to address that weird feedback from here. One can either override the "acf-minecraft.is_not_a_valid_name" message key. Or override the resolvers themselves.

I don't want to communicate that this is THE best solution to the reported issue. But I think it is a good stop-gap while the bigger picture is thought about further. Ultimately, a straight up removal does change behavior which makes me hesitant to rush to remove it for those that desire the enhanced feedback of stating it isn't a valid name. The new behavior does not block input which matches the pattern.

To reiterate, although I am closing this issue (the immediate problem is solved); that does not mean I am against a more direct configuration option. Such a solution is not immediately obvious to me.

	public static boolean isValidName(String name) {
	return name != null && !name.isEmpty() && ACFPatterns.VALID_NAME_PATTERN.matcher(name).matches();
	}

	if (!isValidName(name)) {
	issuer.sendError(MinecraftMessageKeys.IS_NOT_A_VALID_NAME, "{name}", name);
	return null;
	}