Skip to content

Converting NamespacedKey parameters to lowercase Strings#13910

Closed
TurboJax wants to merge 1 commit into
PaperMC:mainfrom
TurboJax:turbo/key_change
Closed

Converting NamespacedKey parameters to lowercase Strings#13910
TurboJax wants to merge 1 commit into
PaperMC:mainfrom
TurboJax:turbo/key_change

Conversation

@TurboJax
Copy link
Copy Markdown

It does it in the other constructor where the namespace is derived from a plugin, but not when it is a string. It just runs String#toLowerCase on the namespace and key parameters, using the Locale.ROOT parameter.

@TurboJax TurboJax requested a review from a team as a code owner May 24, 2026 08:31
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue May 24, 2026
@masmc05
Copy link
Copy Markdown
Contributor

masmc05 commented May 24, 2026

The other constructor is doing that because the plugin name can be not lowercase which is valid usage. Here this is masking invalid inputs without the developer properly considering side effects

@TurboJax
Copy link
Copy Markdown
Author

How is it valid usage because of the namespace being lowercase? The key in the other constructor is made lowercase automatically, so I thought this would be acceptable.

@davidmayr
Copy link
Copy Markdown
Contributor

davidmayr commented May 24, 2026

How is it valid usage because of the namespace being lowercase? The key in the other constructor is made lowercase automatically, so I thought this would be acceptable.

It is made lowercase automatically when supplying a plugin as input because you do not control the input directly. Most plugins have a uppercase letter in their name somewhere which is a valid thing, and it would be a breaking change if every author would have to change this in order to use NamespacedKey. When specifying the namespace manually, you are in control, so this is no longer an issue and the correct requirements are enforced

@lynxplay lynxplay closed this May 24, 2026
@github-project-automation github-project-automation Bot moved this from Awaiting review to Closed in Paper PR Queue May 24, 2026
@TurboJax
Copy link
Copy Markdown
Author

TurboJax commented May 24, 2026

because you do not control the input directly

You do though? I was talking about the key, not the namespace. You directly control the key, but it is still made lowercase. Should this change be removed?

When specifying the namespace manually, you are in control

I haven't been talking about the namespace, I've been talking about the key.

In both scenarios, you have full control over the key, but it is still changed to be made lowercase when the plugin class is specified. I also made the namespace string be lowercase because I thought it'd be weird to throw errors for a capitalized namespace but not for a capitaized key.

@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented May 24, 2026

I mean, we cannot really remove it now as it already exists in the plugin constructor of namespacedkey without breaking backwards compatibility. that bit of the namespaced key constructor was inherited from spigot,

I agree with the above comments tho. Manually and quietly changing the key to lowercase seems only harmful when developers should very much be forced to use correct casing for these inputs.

@TurboJax TurboJax deleted the turbo/key_change branch May 24, 2026 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

4 participants