Skip to content

Fix get charset from content-type header with multiple parameters, cl…#8286

Merged
normanmaurer merged 2 commits intonetty:4.1from
amizurov:4.1
Sep 14, 2018
Merged

Fix get charset from content-type header with multiple parameters, cl…#8286
normanmaurer merged 2 commits intonetty:4.1from
amizurov:4.1

Conversation

@amizurov
Copy link
Copy Markdown
Contributor

Close Resolve charset from ContentType header for SOAP 1.2 requests.

Motivation:

Get charset from Content-Type header even it contains multiple parameters.

Modification:

Extract charset value from the charset parameter if it is not last.

Result:

Fixes #8273

Copy link
Copy Markdown
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

two nits... then good to go.

int indexOfSemicolon = AsciiString.indexOfIgnoreCaseAscii(charsetCandidate, SEMICOLON, 0);
if (indexOfSemicolon == AsciiString.INDEX_NOT_FOUND) {
return charsetCandidate;
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: You can remove the else here as you return in the if block.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

public static boolean isKeepAlive(HttpMessage message) {
CharSequence connection = message.headers().get(HttpHeaderNames.CONNECTION);
if (connection != null && HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(connection)) {
if (HttpHeaderValues.CLOSE.contentEqualsIgnoreCase(connection)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@amizurov can you add comment that passing null into contentEqualsIngoreCase is fine ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we passed null to contentEqulasIgnoreCase(...) this always return false, null safe method.

Copy link
Copy Markdown
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Member

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

LGTM

@normanmaurer normanmaurer added this to the 4.1.30.Final milestone Sep 14, 2018
@normanmaurer normanmaurer merged commit 2ab3e13 into netty:4.1 Sep 14, 2018
@normanmaurer
Copy link
Copy Markdown
Member

@amizurov thanks a lot!

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.

4 participants