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

Adapt to new Netty5 Header API #2484

Merged
merged 7 commits into from Sep 20, 2022

Conversation

pderop
Copy link
Member

@pderop pderop commented Sep 14, 2022

This PR attempts to adapt to the following Netty5 changes:

These changes have a quite important impact on the RN API. I have made some choices (it's a first attempt, let me know what you think ...)

So, this PR provides the following logical commits (in order to try to make easier the review):

  1. 8257106: renamed io.netty.buffer.api package to io.netty.buffer

  2. 0060895 renamed io.netty5.handler.codec.http.HttpHeaders package to io.netty5.handler.codec.http.headers.HttpHeaders

  3. 743c579: Adapted to new HttpHeaders API: This is a quite large commit, however, all changes are similar

  4. 6fb29bf: Removed the old ServerCookieEncoder/ServerCookieDecoder/ClientCookieDecoder/ClientCookieEncoder classes, which are not available anymore. Replaced old netty Cookie class by HttpCookiePair/HttpSetCookie.The RN Cookies class is used to cache Set-Cookie headers found from http response headers, and the ServerCookies class is used to cache Cookie headers found from http request headers.

About the 7062ace commit (Adapted to new HttpHeaders API):

  • The HttpHeaders methods are now returning CharSequence instead of String, same for the setter methods which accepts CharSequence, so adaptations are required
  • HttpHeaders.setInt method is not available anymore, we need to adapt
  • HttpHeaders.contains has been replaced by containsIgnoreCase
  • HttpHeaders.remove methods do not return the HttpHeaders, but a boolean, we can't chain remove(...).remove(...)
  • The DefaultHeaders.NameValidator is not available anymore, so the HttpServerOperations.TrailerHeaders has to be adapted where the new HttpHeaders.validateKeys method is now overriden by the HttpServerOperations.TrailerHeaders class

Regarding the last commit 6fb29bf for Cookie/Set-Cookie changes:

  • Using the new API, a Cookie header is represented by HttpCookiePair and a Set-Cookie header is represented by HttpSetCookie, and the old Cookie is not available anymore.
    In this PR, the HttpCookiePair is now used instead of the old Cookie.

  • The RN Cookies class was used to cache Set-Cookie headers from http response, so the new HttpSetCookie class is now used internally. Simplified the Cookies class which is only caching Set-Cookie header.

  • The RN ServerCookies class was used to cache Cookie headers from http requests , so the new HttpCookiePair is now used internally.

  • For inbound requests, the headers are validated using DefaultHttpHeaders.validateHeaderValue, and more specifically, Set-Cookies header are validated using DefaultHttpSetCookie.parseSetCookie. Such validation can be deacrivated as before, using HttpDecoderSpec.validateHeaders(boolean) method.

  • The ClientCookieEncoder is not available anymore. in STRICT mode, this encoder was checking if Cookie name and value chars were in the valid scope (RFC6265), and for methods that accept multiple cookies, the encoder sorted cookies into order of decreasing path length. But it seems we don't have the equivalent encoder using the new HttpHeader API (to be investigated further on).

  • The ClientCookieDecoder is not available anymore: in STRICT mode, It was used to decode Set-Cookie headers from http response, and was checking if Set-Cookie chars are in valid scope (RFC6265).

  • The ServerCookieEncoder is not available anymore: in STRICT mode, it was used to encode a Set-Cookie header and it validated that name and value chars were in the valid scope defined in RFC6265, and (for methods that accept multiple cookies) that only one cookie is encoded with any given name.

  • The ServerCookieDecoder is not available anymore: in STRICT mode, it was used to decode Cookie headers from http requests, and it was validating if chars were in valid scope (RFC6265).

@pderop pderop added the type/enhancement A general enhancement label Sep 14, 2022
@pderop pderop added this to the 2.0.0-M2 milestone Sep 14, 2022
@pderop pderop self-assigned this Sep 14, 2022
@violetagg
Copy link
Member

violetagg commented Sep 14, 2022

@pderop Is the decoding/encoding now STRICT or is it LAX?

@pderop
Copy link
Member Author

pderop commented Sep 14, 2022

@violetagg ,

most of the time, encoding/decoding is STRICT, except one use case, which seems to be a bug, see below, last use case:

  • when decoding Cookie headers: it's STRICT (can be disabled): headers are validated here

  • when decoding Set-Cookie headers: it's STRICT (can be disabled), headers are validated here

  • When encoding a Set-Cookie: it's STRICT, headers are validated here

  • when encoding a Cookie: it's inconsistent (bug ?): sometimes it's STRICT, sometimes it's LAX: when more than one Cookie are encoded using multiple calls to add(HttpCookiePair), then only the first Cookie is validated, not the subsequent cookies, see here, where the added cookies are not validated.
    Indeed, it's the put method, so only the firs added Cookie will be validated, see put method that is done only for the first Cookie .

Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

I think the port is OK.
Please list in build.gradle the removed APIs. Also prepare deprecation PR for 1.1.x branch.

@violetagg
Copy link
Member

@violetagg ,

most of the time, encoding/decoding is STRICT, except one use case, which seems to be a bug, see below, last use case:

  • when decoding Cookie headers: it's STRICT (can be disabled): headers are validated here
  • when decoding Set-Cookie headers: it's STRICT (can be disabled), headers are validated here
  • When encoding a Set-Cookie: it's STRICT, headers are validated here
  • when encoding a Cookie: it's inconsistent (bug ?): sometimes it's STRICT, sometimes it's LAX: when more than one Cookie are encoded using multiple calls to add(HttpCookiePair), then only the first Cookie is validated, not the subsequent cookies, see here, where the added cookies are not validated.
    Indeed, it's the put method, so only the firs added Cookie will be validated, see put method that is done only for the first Cookie .

Please address this also

@pderop
Copy link
Member Author

pderop commented Sep 15, 2022

@violetagg ,

I have addressed some of your feedbacks, except the following:

  • for the "cookie(Supplier cookieBuilder" method, please check my comment, I need confirmation.
  • I will update the build.gradle after all the feedbacks will be fully resolved.

For the encoding (STRICT mode) of the Cookie header, I will make a PR for Netty.
Once this issue is merged, I'll prepare a PR for the 1.0.x for deprecation.

@pderop
Copy link
Member Author

pderop commented Sep 15, 2022

I see there is a conflict, let me check before reviewing.

…er/ClientCookieDecoder/ClientCookieEncoder classes, which are not available anymore.

Replaced old netty Cookie class by HttpCookiePair/HttpSetCookie.
The Cookies class is used to cache Set-Cookie headers found from http response headers.
The ServerCookies class is used to cache Cookies headers found from http request headers.
@pderop pderop force-pushed the netty5-port-new-httpheader-api branch from 32b1129 to 7f5f064 Compare September 15, 2022 15:48
@pderop
Copy link
Member Author

pderop commented Sep 15, 2022

rebased on top of netty5 branch, to get #2462 and merge it.

@pderop
Copy link
Member Author

pderop commented Sep 19, 2022

@violetagg ,

I have attempted to address all feedbacks in the last commit.

Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

Let's see CI green and then you can merge.

@pderop pderop merged commit ffd76c8 into reactor:netty5 Sep 20, 2022
@pderop pderop deleted the netty5-port-new-httpheader-api branch September 20, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants