Skip to content

Tighten language around GET request bodies (Mnot 202) #250

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

Merged
merged 11 commits into from
Nov 3, 2019
Merged

Conversation

mnot
Copy link
Member

@mnot mnot commented Sep 3, 2019

Fixes #202.

@mnot mnot requested review from reschke and royfielding September 3, 2019 07:55
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

Not sure about havign a new normative requirement for DELETE. Did we agree on this.

That said, we also need to mention the normative change for GET in "Changes from 7231".

@reschke reschke changed the title Mnot 202 Tighten language around GET and DELETE request bodies (Mnot 202) Sep 3, 2019
Copy link
Member

@royfielding royfielding left a comment

Choose a reason for hiding this comment

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

For editorial consistency, I have been removing cases where requirements are applied to plurals or to larger-than-necessary scope. In this case, I think it should be

   A payload within a GET request message has no defined semantics.
   A client SHOULD NOT generate a payload body in a GET request.
   Sending a payload body on a GET request might cause some existing
   implementations to reject the request.

@mnot
Copy link
Member Author

mnot commented Sep 4, 2019

I'm fine with that; it was borderline.

# Conflicts:
#	draft-ietf-httpbis-semantics-latest.xml
@mnot mnot requested review from reschke and royfielding September 4, 2019 04:24
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

  1. This is a new normative requirement on GET; good, but needs to be mentioned as a change.

  2. The change on DELETE is not what we discussed in the WG. I propose to take this out here and move it into a separate issue.

@mnot
Copy link
Member Author

mnot commented Sep 30, 2019

Change note added.

Creating a new issue doesn't make any difference; they're just how we track what we need to do.

IIRC there was one person who mentioned an existence proof of a DELETE body -- which is very analogous to the situation with GET. We know that some people do them, but we also know that they're not interoperable, and this is not an intended extension of the protocol.

@reschke
Copy link
Contributor

reschke commented Sep 30, 2019

I strongly believe that the issues for GET and DELETE are very different, and that handling them in a single ticket is harmful.

Can we please focus on GET first?

@mnot
Copy link
Member Author

mnot commented Sep 30, 2019

Please update the PR and create the issue, then.

@reschke
Copy link
Contributor

reschke commented Sep 30, 2019

will do.

@asbjornu
Copy link

asbjornu commented Sep 30, 2019

Would it be possible to mention the problems around caching with a body on GET requests? As GET with a body is in use by large vendors such as Elasticsearch (see elastic/elasticsearch#16024) and their interpretation of the spec being that this is "just fine" (see elastic/elasticsearch-php#737 (comment)), I think a stronger language should be used to discourage implementations from using a body with GET requests.

This also brings up what we discussed at the HTTP Workshop in May around creating a header to declare that the request body should also be considered a part of the cache key, as well as the possibility of creating a new SEARCH method (currently drafted by @jasnell).

@reschke reschke changed the title Tighten language around GET and DELETE request bodies (Mnot 202) Tighten language around GET request bodies (Mnot 202) Sep 30, 2019
@reschke
Copy link
Contributor

reschke commented Oct 1, 2019

@asbjornu - the pull request has this:

A payload within a GET request message has no defined semantics. A client SHOULD NOT generate a body in a GET request. Sending a payload body on a GET request might cause some existing implementations to reject the request.

IIUC, you'd like this also to mention other failure modes such as GET request suceeding, but result might be inproperly cached?

(the other points IMHO are out of scope for the HTTP core revision; maybe they should be tracked in the http-extensions repo instead).

@asbjornu
Copy link

asbjornu commented Oct 1, 2019

@reschke:

IIUC, you'd like this also to mention other failure modes such as GET request suceeding, but result might be inproperly cached?

Yes, exactly. Just as an informational warning so implementers understand the dangers they are putting themselves into by ignoring "SHOULD NOT".

(the other points IMHO are out of scope for the HTTP core revision; maybe they should be tracked in the http-extensions repo instead).

Should I create an issue for SEARCH and one for a cache header in the httpwg/http-extensions repository?

@reschke
Copy link
Contributor

reschke commented Oct 1, 2019

  1. Ack. @mnot ?

  2. Yes.

@asbjornu
Copy link

asbjornu commented Oct 2, 2019

A payload within a GET request message has no defined semantics. A client
&SHOULD-NOT; generate a body in a GET request. Sending a payload body on a
GET request might cause some existing implementations to reject the
request.
Copy link
Contributor

Choose a reason for hiding this comment

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

"... or might cause surprising effects in HTTP caches which ignore the payload."

Copy link
Member

Choose a reason for hiding this comment

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

caches that ignore the request payload? I'm not sure what you mean here.

Copy link
Contributor

Choose a reason for hiding this comment

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

A cache might just pass the request payload to the server, but then cache the result despite the fact it might vary on the request body. So that kind of failure would only be indirectly observable.

I think it's worth pointing this out...

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps "... or might cache caches to reuse the response erroneously."

…of a payload on a GET request shall be piked and roasted over a blast furnace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Tighten language around GET request bodies
4 participants