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.

Sign up for GitHub

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

Jump to bottom

Improve CookieCompliance testing #9399

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Improve CookieCompliance testing #9399

merged 7 commits into from
Feb 23, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Feb 20, 2023

  • Improved handling of CookieCompliance.from method
  • Added tests for request and response cookie handling
  • Use from in jetty.xml

@gregw
Improve CookieCompliance testing
71111a4 
Improved handling of CookieCompliance.from method
Added tests for request and response cookie handling
Use from in jetty.xml
@gregw gregw requested review from olamy and lorban February 20, 2023 04:39
@gregw
Improve CookieCompliance testing
66fc0c3 
Improved handling of CookieCompliance.from method
Added tests for request and response cookie handling
Use from in jetty.xml
@gregw
Copy link
Contributor Author

gregw commented Feb 20, 2023

@olamy these fixes may help with the TCK cookie failure. At the very least the ServerHttpCookieTest can be used to recreate the TCK failure to investigate deeper.

gregw added 2 commits February 20, 2023 23:40
@gregw
Improve CookieCompliance testing
597956e 
Improved handling of CookieCompliance.from method
Added tests for request and response cookie handling
Use from in jetty.xml
@gregw
Improve CookieCompliance testing
7708988 
InvalidCookieException
@gregw gregw requested a review from lorban February 20, 2023 13:00
joakime
joakime requested changes Feb 20, 2023
View reviewed changes
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.

Just a niggle that the InvalidCookieException is a declared throwable in some APIs, and it a useful exception type to work with, it shouldn't be a nested private class, it should be it's own type.

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java Outdated Show resolved Hide resolved
@gregw gregw requested a review from joakime February 20, 2023 21:45
@gregw
Copy link
Contributor Author

gregw commented Feb 20, 2023

@olamy any chance of testing this against the TCK before the merge?

joakime
joakime approved these changes Feb 20, 2023
View reviewed changes
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.

Not a fan of the hidden inner exception that isn't at least static, but lets get this merged.

@olamy
Copy link
Member

olamy commented Feb 20, 2023

@olamy any chance of testing this against the TCK before the merge?

I did but this didn't look to fix the TCK test.

@gregw
Copy link
Contributor Author

gregw commented Feb 21, 2023

@joakime it is not hidden. It's static by default because it is in an interface. If I add the static keyword, intellij tells me that it is not needed. It is not different to Handler.Wrapper, Handler.Collection, Request.Wrapper, Content.Sink, Content.Source. etc. We now have lots of classes and interfaces scoped inside an interface.

@gregw
Copy link
Contributor Author

gregw commented Feb 21, 2023

I'm putting this PR on hold an updating #9402 to include the bulk of these changes, and then merge them back through from 10.

@gregw gregw marked this pull request as draft February 21, 2023 06:16
gregw added a commit that referenced this pull request Feb 21, 2023
@gregw
Fix #9334 Cookie Compliance
78c9074 
Fix incorrect change to RFC6265 to not support dollars in cookie names.
Included updates and tests from #9399

Signed-off-by: gregw <gregw@webtide.com>
gregw added a commit that referenced this pull request Feb 21, 2023
@gregw
Fix #9334 Cookie Compliance
79bba38 
Fix incorrect change to RFC6265 to not support dollars in cookie names.
Included updates and tests from #9399

Signed-off-by: gregw <gregw@webtide.com>
@gregw gregw marked this pull request as ready for review February 21, 2023 11:35
@gregw
Copy link
Contributor Author

gregw commented Feb 21, 2023

@sbordet @lorban This PR was mostly done in #9402 in jetty-10 and has now been merged forward to jetty-12. What is left are just a few cleanups to use modern switches and to align the other compliance mode impls.

Can you review and if OK please merge.

@joakime
Copy link
Contributor

joakime commented Feb 21, 2023

@joakime it is not hidden. It's static by default because it is in an interface. If I add the static keyword, intellij tells me that it is not needed. It is not different to Handler.Wrapper, Handler.Collection, Request.Wrapper, Content.Sink, Content.Source. etc. We now have lots of classes and interfaces scoped inside an interface.

Exceptions will propagate through lots of complex logging scenarios.
The non-static inner, means the outer instance is now tied to the instances of that inner.
This could potentially lead to scenarios which GC is impacted.

The other examples don't propagate through other layers like logging.

@gregw
Copy link
Contributor Author

gregw commented Feb 21, 2023

@joakime it is static. All classes within an interface are static:
Screenshot from 2023-02-22 08-00-40

@gregw gregw merged commit 6c114b0 into jetty-12.0.x Feb 23, 2023
@gregw gregw deleted the jetty-12-Request-CookieCompliance-from branch February 23, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorban lorban lorban approved these changes

@joakime joakime joakime approved these changes

@olamy olamy Awaiting requested review from olamy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregw @olamy @joakime @lorban