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

Add HTTP Method QUERY support #499

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

desiderantes
Copy link
Contributor

This PR introduces support for the HTTP Method QUERY, a safe-idempotent method that has a body. Since Node added support for the query method, there is now a need for clients to support it and behave properly (like sending content length == 0 when skipping a body).

@arturobernalg
Copy link
Member

@desiderantes From my understanding, the QUERY method is not defined in any official HTTP specification

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hi @desiderantes
I'd like to see all new public and protected elements come with Javadoc (with since tags).

@arturobernalg
Copy link
Member

arturobernalg commented Oct 19, 2024

I have a couple of concerns regarding the addition of the QUERY method:

  • Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

  • Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

@desiderantes
Copy link
Contributor Author

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

@desiderantes
Copy link
Contributor Author

Hi @desiderantes I'd like to see all new public and protected elements come with Javadoc (with since tags).

Besides a since tag, what else should the builder methods document? All other methods don't have a javadoc.

@arturobernalg
Copy link
Member

arturobernalg commented Oct 19, 2024

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

A draft is not a finalized standard

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

Since the behavior around GET with a body is often debated, there's a risk of inconsistent handling across different implementations.

@garydgregory
Copy link
Member

Hi @desiderantes I'd like to see all new public and protected elements come with Javadoc (with since tags).

Besides a since tag, what else should the builder methods document? All other methods don't have a javadoc.

I think it would be good to carry standard Javadoc tags like param, return, throws, whatever is appropriate for that element. For the description, I like to be able to understand why I'd use one thing versus another. In this case, a query is lile a get plus a body.

@desiderantes
Copy link
Contributor Author

A draft and not a finalized standard

I think a major web framework adding it pushes forward the need for support by a lot, since there will be services using it now.

Since the behavior around GET with a body is often debated, there's a risk of inconsistent handling across different implementations.

To bring a quick summary, GET is for resources, QUERY is for, well, queries. You'd GET /entities/1 but you'd QUERY /entities with GraphQL, JSONPath, SQL, or whatever the server accepts as a body to perform an idempotent query.

@desiderantes
Copy link
Contributor Author

desiderantes commented Oct 19, 2024

I think it would be good to carry standard Javadoc tags like param, return, throws, whatever is appropriate for that element. For the description, I like to be able to understand why I'd use one thing versus another. In this case, a query is lile a get plus a body.

Should I add similar javadoc for all the other builder $method$(...) methods? I feel like it would be weird to only have a couple being documented

@garydgregory
Copy link
Member

garydgregory commented Oct 19, 2024

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

I wonder what the likelihood is of this graduating from draft status. There must be a ton of drafts that have gone nowhere...

@garydgregory
Copy link
Member

I think it would be good to carry standard Javadoc tags like param, return, throws, whatever is appropriate for that element. For the description, I like to be able to understand why I'd use one thing versus another. In this case, a query is lile a get plus a body.

Should I add similar javadoc for all the other builder $method$(...) methods? I feel like it would be weird to only have a couple being documented

I would not ask a contributor to pick up our dropped ball. Let's see what others think about the overall goal first.

@desiderantes
Copy link
Contributor Author

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

I wonder what the likelihood is of this graduating from draft status. There must be a ton of drafts that have gone nowhere...

I think it's likely to graduate "soon". The document is being refined lately and discussion is pretty healthy

@arturobernalg
Copy link
Member

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

I wonder what the likelihood is of this graduating from draft status. There must be a ton of drafts that have gone nowhere...

I think it's likely to graduate "soon". The document is being refined lately and discussion is pretty healthy

First, about the standardization aspect: relying on a draft, no matter how 'soon' you think it will graduate, is risky. We can't build a reliable core library on something that might become a standard. Drafts come and go; many have promising discussions and then disappear. If this draft does become an RFC, great—we can add support then. Until that point, we should not prematurely introduce something that carries compatibility risk without any guarantee.

As for adding the QUERY method to support use cases like GET with a body: IMO Many server implementations can/will handle such requests inconsistently, leading to unpredictable behavior. There is no widespread consensus among major HTTP servers to support GET with a body, and adding this as a content-enclosing method doesn't align with current expectations of how safe and idempotent methods operate.

Regarding the Javadocs, I agree with @garydgregory . If we're adding public methods, they must be properly documented according to the Apache standards, including the @SInCE tags, parameter descriptions, and proper guidance on usage. The fact that some existing methods lack Javadocs is not an excuse to perpetuate this issue. We should be raising the bar, not repeating past shortcomings.

Finally, it would also be better if this PR came with a proper test case. Proper tests ensure we catch edge cases and unintended consequences before this ends up in users' hands.

@desiderantes
Copy link
Contributor Author

I have a couple of concerns regarding the addition of the QUERY method:

Sure

* Lack of Standardization: The QUERY method isn't defined in any official HTTP specification (like RFC 7231). Adding a non-standard method could lead to compatibility issues and confusion, especially since users might assume it's widely recognized. Should we really be adding this as a core method, or is there a specific context or internal protocol that necessitates this?

There is a draft in progress for it: https://datatracker.ietf.org/doc/draft-ietf-httpbis-safe-method-w-body/

* Content-Enclosing Method: The change marks QUERY as a content-enclosing method, similar to POST, PUT, and PATCH. This seems unconventional since typical query operations (like GET) don't usually involve a request body. What’s the rationale behind allowing QUERY to enclose content

The main purpose of QUERY is to be a GET with body.

I wonder what the likelihood is of this graduating from draft status. There must be a ton of drafts that have gone nowhere...

I think it's likely to graduate "soon". The document is being refined lately and discussion is pretty healthy

First, about the standardization aspect: relying on a draft, no matter how 'soon' you think it will graduate, is risky. We can't build a reliable core library on something that might become a standard. Drafts come and go; many have promising discussions and then disappear. If this draft does become an RFC, great—we can add support then. Until that point, we should not prematurely introduce something that carries compatibility risk without any guarantee.

As for adding the QUERY method to support use cases like GET with a body: IMO Many server implementations can/will handle such requests inconsistently, leading to unpredictable behavior. There is no widespread consensus among major HTTP servers to support GET with a body, and adding this as a content-enclosing method doesn't align with current expectations of how safe and idempotent methods operate.

Regarding the Javadocs, I agree with @garydgregory . If we're adding public methods, they must be properly documented according to the Apache standards, including the @SInCE tags, parameter descriptions, and proper guidance on usage. The fact that some existing methods lack Javadocs is not an excuse to perpetuate this issue. We should be raising the bar, not repeating past shortcomings.

Finally, it would also be better if this PR came with a proper test case. Proper tests ensure we catch edge cases and unintended consequences before this ends up in users' hands.

I do not understand the argument about inconsistencies of GET with bodies, this is a different method, services would either support it or not. I think that's the main point of this new method, to have a clean way to handle GET-like requests with body, without having to guess if any part of the infrastructure assumes GET don't have bodies (like a proxy stripping them) and having surprising results and failures.

@arturobernalg
Copy link
Member

I do not understand the argument about inconsistencies of GET with bodies, this is a different method, services would either support it or not. I think that's the main point of this new method, to have a clean way to handle GET-like requests with body, without having to guess if any part of the infrastructure assumes GET don't have bodies (like a proxy stripping them) and having surprising results and failures.

Your point is understood, but the problem remains fundamentally the same.

The fact that QUERY is technically a different method doesn't eliminate the inconsistencies; it just shifts them. Introducing QUERY as a workaround for the issues with GET bodies assumes the entire HTTP ecosystem will uniformly recognize and support it. In reality, infrastructure like proxies, caching layers, and older servers are unlikely to know what to do with QUERY requests. This could lead to dropped requests, unexpected stripping of bodies, or flat-out rejections, similar to the problems you're trying to avoid with GET and a body. Simply put, anything that doesn’t explicitly recognize QUERY may mishandle it.

@desiderantes
Copy link
Contributor Author

I do not understand the argument about inconsistencies of GET with bodies, this is a different method, services would either support it or not. I think that's the main point of this new method, to have a clean way to handle GET-like requests with body, without having to guess if any part of the infrastructure assumes GET don't have bodies (like a proxy stripping them) and having surprising results and failures.

Your point is understood, but the problem remains fundamentally the same.

The fact that QUERY is technically a different method doesn't eliminate the inconsistencies; it just shifts them. Introducing QUERY as a workaround for the issues with GET bodies assumes the entire HTTP ecosystem will uniformly recognize and support it. In reality, infrastructure like proxies, caching layers, and older servers are unlikely to know what to do with QUERY requests. This could lead to dropped requests, unexpected stripping of bodies, or flat-out rejections, similar to the problems you're trying to avoid with GET and a body. Simply put, anything that doesn’t explicitly recognize QUERY may mishandle it.

An explicit failure mode (not supporting unknown/custom HTTP methods) is substantially better than mishandled request bodies, so I'd say it is not a shift, but an enforcement, either the method is supported/allowed to pass (and the server ends up getting the request) or it is not, and the request fails. No middle ground, no inconsistent behaviour, other than caching proxies not caching responses (which would just be a matter of the server having a misconfigured cache).

@arturobernalg
Copy link
Member

An explicit failure mode (not supporting unknown/custom HTTP methods) is substantially better than mishandled request bodies, so I'd say it is not a shift, but an enforcement, either the method is supported/allowed to pass (and the server ends up getting the request) or it is not, and the request fails. No middle ground, no inconsistent behaviour, other than caching proxies not caching responses (which would just be a matter of the server having a misconfigured cache).

@desiderantes I disagree.

The idea that an explicit failure mode is better doesn’t hold up here. The impact of introducing a non-standard method like QUERY isn’t simply 'support or reject.' In practice, proxies, load balancers, and other intermediaries often handle unknown methods in unpredictable ways—stripping bodies, dropping headers, or routing inconsistently. These behaviors are rarely explicit or predictable.

@ok2c
Copy link
Member

ok2c commented Oct 19, 2024

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

@garydgregory
Copy link
Member

An explicit failure mode (not supporting unknown/custom HTTP methods) is substantially better than mishandled request bodies, so I'd say it is not a shift, but an enforcement, either the method is supported/allowed to pass (and the server ends up getting the request) or it is not, and the request fails. No middle ground, no inconsistent behaviour, other than caching proxies not caching responses (which would just be a matter of the server having a misconfigured cache).

@desiderantes I disagree.

The idea that an explicit failure mode is better doesn’t hold up here. The impact of introducing a non-standard method like QUERY isn’t simply 'support or reject.' In practice, proxies, load balancers, and other intermediaries often handle unknown methods in unpredictable ways—stripping bodies, dropping headers, or routing inconsistently. These behaviors are rarely explicit or predictable.

I think what I'm hearing @desiderantes say is that if my stack supports QUERY, please let me use it. It's up to me as an app author to test and document what my app does and expects.

@garydgregory
Copy link
Member

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

I like the idea of annotating the code with @Experimental("RCF...") for all new public and protected elements.

@desiderantes
Copy link
Contributor Author

Or just not merge it now. I don't know if there's a policy regarding PR status and longevity, but I could also just move this to a draft PR and

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

This makes a lot of sense, thank you. I see that the @experimental annotation does not include any field. Is it OK if I add an "until" or "reason" field?

@arturobernalg
Copy link
Member

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

@ok2c seems like a reasonable compromise.

@ok2c
Copy link
Member

ok2c commented Oct 19, 2024

Or just not merge it now. I don't know if there's a policy regarding PR status and longevity, but I could also just move this to a draft PR and

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

This makes a lot of sense, thank you. I see that the @experimental annotation does not include any field. Is it OK if I add an "until" or "reason" field?

@desiderantes Sure. reason sounds more generic to me, but I leave it up to you.

@garydgregory
Copy link
Member

Or just not merge it now. I don't know if there's a policy regarding PR status and longevity, but I could also just move this to a draft PR and

@desiderantes @garydgregory @arturobernalg We can mark the new method as @Experimental and even @Internal until the standardisation is complete. I personally do not see a problem here.

This makes a lot of sense, thank you. I see that the @experimental annotation does not include any field. Is it OK if I add an "until" or "reason" field?

@desiderantes Sure. reason sounds more generic to me, but I leave it up to you.

I would make it a plain String called "value" with a default null or empty value.

@ok2c
Copy link
Member

ok2c commented Oct 19, 2024

@desiderantes japicmp does not like new fields added to @Experimenal. Just add a comment next to it to keep japicmp happy.

@ok2c
Copy link
Member

ok2c commented Oct 19, 2024

@desiderantes

Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.21.2:cmp (default) on project httpcore5: There is at least one incompatibility: org.apache.hc.core5.annotation.Experimental.value():METHOD_ABSTRACT_ADDED_TO_CLASS -> [Help 1]

It is not worth it. Leave the poor thing be and just add a comment

@arturobernalg arturobernalg self-requested a review October 29, 2024 17:21
@ok2c
Copy link
Member

ok2c commented Oct 29, 2024

If I hear no objections I will merge this PR.

@ok2c ok2c merged commit bce3474 into apache:master Oct 30, 2024
10 checks passed
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