Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4209] chore replace hardcoded value with std definition #6354

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

zzzk1
Copy link
Contributor

@zzzk1 zzzk1 commented Jan 22, 2025

What changes were proposed in this pull request?

Use standard definition lib value to replace the hardcoded value: UTF-8.

Why are the changes needed?

Fix: #4209

Does this PR introduce any user-facing change?

no

How was this patch tested?

no

@zzzk1 zzzk1 changed the title [#4209] Chore replace hardcoded value with std definition [#4209] chore replace hardcoded value with std definition Jan 22, 2025
return new String(bb.array(), bb.arrayOffset() + bb.position(), bb.remaining(), "UTF-8");
} catch (UnsupportedEncodingException e) {
throw new IllegalCharsetNameException("UTF-8"); // won't happen.
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this code?

The String constructor used here does not declare that it throws an UnsupportedEncodingException because StandardCharsets.UTF_8 guarantees that the encoding is supported. However, an IndexOutOfBoundsException could occur in this context.

Copy link
Member

@justinmclean justinmclean Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems Ok, but I assume it was there for some reason for it originally, so it may not be best to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems Ok, but I assume it was there for some reason for it originally, so it may not be best to remove it.

we can't do that, if we use:

try {
       return new String(bb.array(), bb.arrayOffset() + bb.position(), bb.remaining(), StandardCharsets.UTF_8);
     } catch (UnsupportedEncodingException e) {
       throw new IllegalCharsetNameException("UTF-8"); // won't happen.
     }

the complier will report Exception java.io.UnsupportedEncodingException is never thrown in the corresponding try block like this:

Screenshot 2025-01-23 at 10 53 59

maybe we can use Exception replace UnsupportedEncodingException?

@justinmclean
Copy link
Member

We use Spotless to ensure our code is consistently formatted. You'll need to run ./gradlew :server-common:spotlessApply to correct the formatting to get the CI checks to pass.

@jerryshao
Copy link
Contributor

Can you please update the PR description like other PRs?

@zzzk1
Copy link
Contributor Author

zzzk1 commented Jan 23, 2025

Can you please update the PR description like other PRs?

yes

Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the improvement

@justinmclean justinmclean merged commit d99ced3 into apache:main Jan 23, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] In KerberosServerUtils.java replace hardcoded string
3 participants