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

Convenience method for checking header value lists #1066

Merged
merged 15 commits into from
Oct 1, 2022

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented Dec 5, 2021

For release 4.0 I'd like to propose a convencience method for response/request contexts which I missed a lot in the past years and which I implemented again and again in multiple ways -- most of them lacking either performance or perfection.

The problem it solves is that it is rather hard to get it perfectly correct checking for items in comma-separated headers - without losing valuable performance.

Example: How to check if there is a Cache-Control: no-store value set in case the actual sent headers contains Cache-Control multiple times and each of the findings is possibly a comma-separated list? RegEx over getHeaderString() comes into mind, but this is rather slow, as it first concatenates everything just to split it up afterwards. Iterating all findings comes into mind next, which is everything but convenient nor particularly readable.

Solution: An implementation of this new method would be provided by the runtime vendor, hence could provide not only most convenient usage to the caller, but also perform best possible as it is optimized according to the actual implementation's internal map design

WDYT?

@mkarg mkarg added enhancement New feature or request api javadoc labels Dec 5, 2021
@mkarg mkarg added this to the 4.0 milestone Dec 5, 2021
@mkarg mkarg self-assigned this Dec 5, 2021
@jansupol
Copy link
Contributor

jansupol commented Dec 7, 2021

Error: Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.3.0:jar (attach-javadocs) on project jakarta.ws.rs-api: MavenReportException: Error while generating Javadoc:
Error: Exit code: 1 - /home/runner/work/jaxrs-api/jaxrs-api/jaxrs-api/src/main/java/jakarta/ws/rs/container/ContainerResponseContext.java:129: error: reference not found
Error: * @see #getHeaderString()

@mkarg
Copy link
Contributor Author

mkarg commented Dec 21, 2021

Error: Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.3.0:jar (attach-javadocs) on project jakarta.ws.rs-api: MavenReportException: Error while generating Javadoc: Error: Exit code: 1 - /home/runner/work/jaxrs-api/jaxrs-api/jaxrs-api/src/main/java/jakarta/ws/rs/container/ContainerResponseContext.java:129: error: reference not found Error: * @see #getHeaderString()

Thanks. Fixed.

@mkarg
Copy link
Contributor Author

mkarg commented May 28, 2022

If nobody objects to adopting this small feature to JAX-RS 4.0, then I would invest some more time and add a TCK test in this PR and remove the draft state flag. Any objects? Please veto NOW so I spare the time for writing the tests. Thanks.

@chkal
Copy link
Contributor

chkal commented May 29, 2022

A few thoughts about this proposal.

First of all: Personally, I never had a situation where this method would have been helpful. However, I see use cases for it, so I'm not against adding it to the API.

One thing I'm wondering about is how to deal with case-insensitivity. For the header name, the situation is clear, as HTTP header names are case-insensitive, which should be handled correctly by the implementation. But what about the header value? My first feeling was that it should do a case-sensitive match. However, in case of Cache-Control mentioned above, directives are case-insensitive. So I'm wondering how we should deal with such cases.

@arjantijms
Copy link
Contributor

Could Servlet not need the exact same utility?

@mkarg
Copy link
Contributor Author

mkarg commented Sep 5, 2022

One thing I'm wondering about is how to deal with case-insensitivity. For the header name, the situation is clear, as HTTP header names are case-insensitive, which should be handled correctly by the implementation. But what about the header value? My first feeling was that it should do a case-sensitive match. However, in case of Cache-Control mentioned above, directives are case-insensitive. So I'm wondering how we should deal with such cases.

We could provide two methods, one case-sensitive and one case-insensitive, just like the JRE does it https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/String.html#equalsIgnoreCase(java.lang.String).

@mkarg
Copy link
Contributor Author

mkarg commented Sep 5, 2022

Could Servlet not need the exact same utility?

OT: Frankly spoken, I personally do not see any use of Servlets anymore at all these days. JAX-RS is superior from the user's view in most cases other than performance wrt to HTTP use. I have not used neither Servlets, nor Servlet-based JAX-RS since years (this is why I lead the Java SE Bootstrap project). If you ask me, the core idea of Servlet API is deprecated, as it was invented as a pure Java replacement for NSAPI / ISAPI, hence to enable Java code run on popular web servers like IIS or Apache (which simply tried to prevent starting native processes using (Fast) CGI). Those times are over since decades. Since most people meanwhile migrated to "real" Java based application servers (hence: products able to natively run JAX-RS), and since mainstream users are migrating to cloud / servlerss architectures, there is not much use left over for Servlets. If you ask me, Servlet API should be dropped in favor of a "Streamlet API" which just provides the interface between a JAX-RS implementation and an NIO engine like Netty or Grizzly. The HTTP part should be completely hidden in the JAX-RS engine; if Grizzly / Netty want to process HTTP, they could natively support JAX-Rs instead of Servlet API instead. Having said that: If somebody likes, he could copy my work provided here for JAX-RS, but I personally will not spend a minute into Servlet API anymore.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 5, 2022

One thing I'm wondering about is how to deal with case-insensitivity. For the header name, the situation is clear, as HTTP header names are case-insensitive, which should be handled correctly by the implementation. But what about the header value? My first feeling was that it should do a case-sensitive match. However, in case of Cache-Control mentioned above, directives are case-insensitive. So I'm wondering how we should deal with such cases.

We could provide two methods, one case-sensitive and one case-insensitive, just like the JRE does it https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/String.html#equalsIgnoreCase(java.lang.String).

Thinking deeper about it, I do not see the need for different methods just to prevent a single "if". I modified the proposal accordingly. WDYT?

@mkarg
Copy link
Contributor Author

mkarg commented Sep 6, 2022

One thing I'm wondering about is how to deal with case-insensitivity. For the header name, the situation is clear, as HTTP header names are case-insensitive, which should be handled correctly by the implementation. But what about the header value? My first feeling was that it should do a case-sensitive match. However, in case of Cache-Control mentioned above, directives are case-insensitive. So I'm wondering how we should deal with such cases.

We could provide two methods, one case-sensitive and one case-insensitive, just like the JRE does it https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/String.html#equalsIgnoreCase(java.lang.String).

Thinking deeper about it, I do not see the need for different methods just to prevent a single "if". I modified the proposal accordingly. WDYT?

Or should we go with a super-flexible solution instead, like providing a predicate? One could then pass even precompiled regex, a lambda expression, or method handles like String::equalsIgnoreCase. 😃 To me, this sounds more powerful. @chkal WDYT?

boolean containsHeaderString(String name, Predicate<String> valuePredicate)

@jansupol
Copy link
Contributor

jansupol commented Sep 6, 2022

Now, this can possibly have a default implementation, nicht wahr?

@mkarg
Copy link
Contributor Author

mkarg commented Sep 6, 2022

Now, this can possibly have a default implementation, nicht wahr?

Well, yes and no. Technically yes, it certainly could, and I would be happy to sponsor one once a majority agreed upon one of the proposal made so far (case-insensitive always? customizable? separate methods? boolean flag? predicate?). But the original idea was that perfect performance can only be provided with the knowledge of the underlying map implementation: Is it a map? Is it a list of entries? Is it a string array? etc. So all vendors should implment it. Given the fact that I would be happy to provide a PR for Jersey, and that only few competitors do exist, I do not see the benefit of a default implementation, frankly spoken.

@jamezp
Copy link
Contributor

jamezp commented Sep 6, 2022

I would think the default should be case-insensitive since in HTTP/1 header names should be case-insensitive. AIUI in HTTP/2 they must be lowercase.

@jansupol
Copy link
Contributor

jansupol commented Sep 12, 2022

I am not convinced about this method. It could be good for some headers, such as Cache-Control but not for others, that use the comma separator for not separating the header values. HTTP RFC, Section 4.2 says:

Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list [i.e., #(values)].

So there is plenty of HTTP headers that use commas for other purposes than separator. All dates containing headers is such an example (with syntax Date: day-name, day month year hour:minute:second GMT). Set-Cookie, or Cookie are also the exmples. Cookie specifies value as

A list of name-value pairs in the form of <cookie-name>=<cookie-value>. Pairs in the list are separated by a semicolon and a space ('; ').

For the generic usage, I'd suggest a separator argument (nullable for no split) to separate the header values.

@spericas
Copy link
Contributor

@jansupol Seems like a reasonable suggestion, and we can still use commas by default.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 15, 2022

For the generic usage, I'd suggest a separator argument (nullable for no split) to separate the header values.

Good idea, I will add this in the next iteration of this PR, stay tuned. :-)

@mkarg
Copy link
Contributor Author

mkarg commented Sep 15, 2022

For the generic usage, I'd suggest a separator argument (nullable for no split) to separate the header values.

Good idea, I will add this in the next iteration of this PR, stay tuned. :-)

On the other hand, we are using a comma (and exactly a comma but nothing else) in #getHeaderString. Maybe we should extend that one, too with an optional concatenation character in analogy?

@jansupol
Copy link
Contributor

Maybe we should extend that one, too with an optional concatenation character in analogy?

Sounds good.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 18, 2022

For the generic usage, I'd suggest a separator argument (nullable for no split) to separate the header values.

Good idea, I will add this in the next iteration of this PR, stay tuned. :-)

Implemented by b5f0a9e.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 18, 2022

@jansupol Seems like a reasonable suggestion, and we can still use commas by default.

How do you envision such a default? By an additional variant of the same method having just two parameters?

@mkarg
Copy link
Contributor Author

mkarg commented Sep 18, 2022

Maybe we should extend that one, too with an optional concatenation character in analogy?

Sounds good.

What would you like more, breaking backwards compatibility by adding a new argument to the existing method, or adding a second variant of the same method?

@spericas
Copy link
Contributor

Maybe we should extend that one, too with an optional concatenation character in analogy?

Sounds good.

What would you like more, breaking backwards compatibility by adding a new argument to the existing method, or adding a second variant of the same method?

@jansupol Seems like a reasonable suggestion, and we can still use commas by default.

How do you envision such a default? By an additional variant of the same method having just two parameters?

Yes, if we think using commas is a sufficiently common case --as suggested by your original PR.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 19, 2022

How do you envision such a default? By an additional variant of the same method having just two parameters?

Yes, if we think using commas is a sufficiently common case --as suggested by your original PR.

Done. :-)

@mkarg
Copy link
Contributor Author

mkarg commented Sep 24, 2022

@jamezp @jansupol @chkal Kindly requesting approval of this PR. :-)

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement New feature or request javadoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants