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

Reintroduce an Exception type for invalid UTF-8 #10553

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 19, 2023

Introduce Utf8StringBuilder.InvalidUtf8Exception as a substitute for the removed Utf8Appendable.NotUtf8Exception.

Introduce `Utf8StringBuilder.InvalidUtf8Exception` as a substitute for the removed `Utf8Appendable.NotUtf8Exception`.
@gregw gregw requested a review from joakime September 19, 2023 22:23
@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Sep 19, 2023
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Can we get by with a nested cause instead of reintroducing the top level exception?

@@ -329,7 +329,7 @@ private static void decodeUtf8To(String query, int offset, int length, BiConsume
switch (c)
{
case '&':
value = buffer.takeCompleteString(() -> new IllegalArgumentException("Invalid value: Bad UTF-8"));
value = buffer.takeCompleteString(() -> new Utf8StringBuilder.InvalidUtf8Exception("Invalid value: Bad UTF-8"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer something like this instead ...

value = buffer.takeCompleteString(() -> new IllegalArgumentException(
new CharacterCodingException("Invalid value: Bad UTF-8")));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that CharacterCodingException does not take a message. It doesn't even take a cause, so we really have to extend it and force the toString and message, otherwise we don't know what the character coding problem is.

So now I have:

    public static class Utf8CharacterCodingException extends CharacterCodingException
    {
        @Override
        public String getMessage()
        {
            return "Invalid UTF-8";
        }

        @Override
        public String toString()
        {
            return "%s@%x: Invalid UTF-8".formatted(CharacterCodingException.class.getSimpleName(), hashCode());
        }
    }

    public static class Utf8IllegalArgumentException extends IllegalArgumentException
    {
        public Utf8IllegalArgumentException()
        {
            super(new Utf8CharacterCodingException());
        }
    }

So any exception will always have a CharacterCodingException in its causal chain. It will also have the "Invalid ITF-8" message reported.

@gregw gregw requested a review from joakime September 21, 2023 03:31
@joakime
Copy link
Contributor

joakime commented Sep 21, 2023

Almost feels like we should use java.nio.charset.MalformedInputException instead (which extends from CharacterCodingException).

Like the JDK itself does in the java.lang.String class

https://github.com/openjdk/jdk/blob/jdk-17%2B35/src/java.base/share/classes/java/lang/String.java#L1238-L1241

@gregw gregw merged commit 57b953b into jetty-12.0.x Sep 22, 2023
4 checks passed
@gregw gregw deleted the fix/jetty-12/InvalidUtf8Exception branch September 22, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants