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

Jetty 12 - New HTTP Cookie interface #9205

Merged
merged 3 commits into from
Jan 30, 2023
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jan 25, 2023

This PR replaces the HttpCookie class with an interface, that allows us to facade the servlet API cookies to our API, rather than copy over the state.
Furthermore, this also embraces the "everything is an attribute" approach taken by the servlet API cookies, so we no longer have the difference in the attribute sets from a standard cookie vs our own cookie. This greatly reduces (but not eliminates) the copying of attribute maps.
Also, the hacks for extracting attributes from comments have been moved to be ee8/ee9 only

@gregw gregw force-pushed the jetty-12-HttpCookie-interface branch from 4f89b37 to 04cc8a0 Compare January 25, 2023 05:15
@gregw gregw marked this pull request as ready for review January 25, 2023 07:23
@joakime joakime added this to the 12.0.x milestone Jan 25, 2023
@joakime joakime changed the title Jetty 12 http cookie interface Jetty 12 - New HTTP Cookie interface Jan 25, 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.

I like the direction this has taken, the HttpCookieFacade makes is clear how powerful this approach is.

The API could use some tweaking (builder please).
The Test case could use some cleanup to avoid setting map entries for those things that are defaulted already.
Other nits around typos, etc.

@gregw gregw requested a review from joakime January 25, 2023 20:55
@sbordet
Copy link
Contributor

sbordet commented Jan 30, 2023

I'd like to chime in with a client-side view about this PR.

I like HttpCookie becomes an interface, would be easier to have an implementation based on java.net.HttpCookie, if necessary.

However, it has become clunky to use, as it is quite common for a client to specify domain, path, expirations for "hardcoded" cookies to send to the server.
For example:

HttpCookie cookie = new HttpCookie("foo", "bar");
cookie.setDomain("domain.com");
cookie.setPath("/");
cookie.setMaxAge(TimeUnit.DAYS.toSeconds(1));

httpCookieStore.add(uri, cookie);

IIUC, now the API is just from(name, value, Map), but I would prefer a builder-style API like HttpURI as in:

HttpCookie.from(name, value).domain("domain.com").maxAge(-1).path("/path").build();

Also, there is a little mess with regards to the client-side HttpCookieStore, which is currently in jetty-util and not in jetty-http where it should be.
However, it is currently an implementation of java.net.CookieStore, which again ties us to the JDK implementation as in particular it store/returns java.net.HttpCookie instances.

So I'm proposing to ditch that class and write a oej.http.HttpCookieStore interface and implementation based on our own HttpURI and HttpCookie instead of JDK classes, so if we need to change something we can (for example, the JDK implementation does not do path matching like it is supposed to, so the client needs to do a double pass at filtering the cookies to send for every request).

Thoughts?

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.

Lots of good here.

Small change of a name of constant left

@sbordet
Copy link
Contributor

sbordet commented Jan 30, 2023

Also, CookieCache and CookieCutter can probably be united into CookieParser (a better name for CookieCutter?

@gregw
Copy link
Contributor Author

gregw commented Jan 30, 2023

@sbordet if the client needs a fluent interface, then we can easily add one. But can it be done in a different PR rather than keeping this one unmerged for even longer?

Only issue is the class with the from name, but elsewhere we have used build so we can easily add:

    HttpCookie cookie = HttpCookie.build("name","value").domain("domain.com").maxAge(-1).path("/path").build()

But I'd like to do that in a PR that actually used it, i.e one that does changes to the client.

@gregw
Copy link
Contributor Author

gregw commented Jan 30, 2023

@sbordet again I think changes to CookieCutter should be in a different PR, especially if there is going to be work done on adding a cookie store to the client.

@gregw gregw requested a review from joakime January 30, 2023 21:20
@gregw gregw merged commit 50a8818 into jetty-12.0.x Jan 30, 2023
@gregw gregw deleted the jetty-12-HttpCookie-interface branch January 30, 2023 23:02
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Feb 1, 2023
…x-documentation-operations-logging

* upstream/jetty-12.0.x: (83 commits)
  Fix connection pool's forcible stop (jetty#9280)
  jetty#9240 add null checks to methods that can be used on a stopped pool
  retry failing tests one more time (jetty#9274)
  suppress stack
  Jetty 12.0.x reenable jetty ee9 tests (jetty#9224)
  Jetty 12.0.x silence tests' stacktraces (jetty#9225)
  More narrowly focused DEBUG logging for JavadocTransparentProxy flaky test
  Revert "More DEBUG on CI build/test"
  More DEBUG on CI build/test
  Adding debug to failing test to see what is causing it to fail.
  Removing unused / old deps
  Remove duplicate dependency declarations.
  Jetty 12 - New HTTP Cookie interface (jetty#9205)
  Converted PathMappings to be an AbstractMap (jetty#9213)
  Removing jetty-unixsocket-* references (doesn't exist in Jetty 12)
  Bump versions.maven.plugin.version to 2.14.2
  Bump apache.directory.api.version to 2.1.2
  Bump hazelcast.version to 4.2.6
  Bump slf4j.version to 2.0.6
  Bump osgi-service-component-version to 1.5.1
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants