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

query: switch Accept-Query to SF (closes #2934) #2935

Closed
wants to merge 15 commits into from
Closed

Conversation

reschke
Copy link
Contributor

@reschke reschke commented Nov 5, 2024

Summary:

  • use list of Tokens holding "type/subtype" media types (not Strings @mnot) - not that if we'd use HTTP's definition of type/subtype as tokes, this would allow "*" in both places (is this a problem in HTTP spec???)
  • allows parameters

TODO:

  • clarify that the Tokens do not hold full media types, which would include parameters
  • clarity that these are not media ranges, thus no wildcards (but see above)

Copy link

@zenomt zenomt left a comment

Choose a reason for hiding this comment

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

LGTM other than the typo i mentioned.

@reschke reschke self-assigned this Nov 5, 2024
The Accept-Query header field specifies a comma-separated listing of media
types (with optional parameters) as defined by
<xref target="HTTP" section="8.3.1"/>. <cref>field syntax currently discussed in <eref target="https://github.com/httpwg/http-extensions/issues/2860"/></cref>
"Accept-Query" contains a list of media types (<xref target="HTTP" section="8.3.1"/>),
Copy link

Choose a reason for hiding this comment

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

sorry, i missed this on first review. here you're saying "a list of media types" with reference to the "Media Types" section of RFC 9110. however, "Media Types" doesn't allow wildcards like image/*. for that you need "Media Ranges".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentionally.

Copy link

Choose a reason for hiding this comment

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

a justification for the intentional "explicit media types, no wildcards" (either in the Issue or here) would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this in the other ticket: what's the use case? Can you give an example?

Copy link

Choose a reason for hiding this comment

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

sorry, i'm not finding where a decision to not allow wildcards was made. i saw you mention */* in #2860 (comment) , and in #2860 (comment) you list as an open question

how does a server say that it supports any format? Does this make any sense at all? We currently require one value, and IMHO we can stick with that

which i took to mean "at least one value".

a use case that comes immediately to mind is a fancy AI™-powered image search that can accept any image type (at least as "any" as a web browser that tells a server it accepts any image type):

Accept-Query: image/*

a possible use case for */* could be a query-by-value of a collection/container resource: given any kind of serialization of any kind of object, answer my contained URI of a resource (and maybe metadata about it) if i happen to contain a resource matching that serialization and media type, or 204 if i don't.

Copy link
Contributor Author

@reschke reschke Nov 15, 2024

Choose a reason for hiding this comment

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

I'm not convinced by these use cases, for "image/*" I would expect that more than the image would be needed, thus some kind of multipart would be used. For "*/*", the situation seems similar.

With that said, I'd propose the simplest possible way to get this resolved would be to indeed allow these values.

Copy link

@zenomt zenomt left a comment

Choose a reason for hiding this comment

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

doing a "request changes" to undo my approval, to address the "media types" vs "media-ranges" concern.

@reschke reschke marked this pull request as draft November 6, 2024 07:41
<xref target="HTTP" section="8.3.1"/>. <cref>field syntax currently discussed in <eref target="https://github.com/httpwg/http-extensions/issues/2860"/></cref>
"Accept-Query" contains a list of media types (<xref target="HTTP" section="8.3.1"/>),
represented by a List Structured Header Field, containing
Token Items, optionally parameterized (<xref target="STRUCTURED-FIELDS" section="3"/>).
Copy link
Member

Choose a reason for hiding this comment

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

What is/are the type(s) of the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good question - I guess "token or string" - or we can restrict them to strings

re: token vs string - it just occured to me that if we use strings instead of tokens (in general), we lose nice properties wrt leading/trailing whitespace.

Copy link

Choose a reason for hiding this comment

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

the parameter-value of a media-type or media-range can be a token or a quoted-string. in order to express media types and their parameters in a SF as commonly given, both token and quoted string need to be supported. quoted string is definitely needed because some media types' parameters (can) have spaces in them that mean something (for example, the profile parameter of application/ld+json).

Copy link
Contributor

Choose a reason for hiding this comment

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

Though not likely useful, q=0.9 would be sf-decimal .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinthomson - I believe @mnot prefers to explicitly restrict the allowed types to the bare minimum.

@reschke reschke marked this pull request as ready for review November 19, 2024 18:20
draft-ietf-httpbis-safe-method-w-body.xml Outdated Show resolved Hide resolved
draft-ietf-httpbis-safe-method-w-body.xml Outdated Show resolved Hide resolved
draft-ietf-httpbis-safe-method-w-body.xml Outdated Show resolved Hide resolved
draft-ietf-httpbis-safe-method-w-body.xml Outdated Show resolved Hide resolved
draft-ietf-httpbis-safe-method-w-body.xml Outdated Show resolved Hide resolved
@reschke
Copy link
Contributor Author

reschke commented Nov 20, 2024

@mnot , @martinthomson , @MikeBishop : "This branch cannot be rebased due to conflicts" - what's going on here?

"Accept-Query" contains a list of media ranges (<xref target="HTTP" section="12.5.1"/>)
using "Structured Fields" syntax (<xref target="STRUCTURED-FIELDS"/>).
Media ranges are represented by a List Structured Header Field of either Tokens or
Strings, containing the media range value without parameters. Parameters,
Copy link

@zenomt zenomt Nov 23, 2024

Choose a reason for hiding this comment

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

saying "Parameters, if any, are mapped to Parameters of type String" is both unnecessarily prescriptive and confusing. if i say application/foo+json; x=3.1, where parameter x is (or could be interpreted as) a SF decimal, i should (1) be allowed to say it that way (that is, i shouldn't be forced to say application/foo+json; x="3.1"), and (2) it's nobody else's business whether i, when receiving this field, treat the 3.1 as a string or a number. that should be between the definition & semantics of the media-type/range and my implementation.

how about words like

"Parameters of the media-range, if any, are mapped to Structured Field Parameters. The media-range with its parameters <bcp14>MUST</bcp14> be parsable as a valid Structured Field."

perhaps using "expressed" or "serialized" in place of "mapped" in the first sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The restriction is intentional. For instance, when not quoting values, surprising things may happen (integer vs floats etc).

On-the-wire verbosity really is no issue here.

Copy link

Choose a reason for hiding this comment

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

media types & ranges have an established syntactic form & semantics, and an established ecosystem beyond HTTP. we want to use this field to tell clients what media types (or ranges of them) are acceptable for this method.

The restriction is intentional. For instance, when not quoting values, surprising things may happen (integer vs floats etc).

i don't think this restriction is a necessary one. i'm not convinced that guarding against a hypothetical insufficient implementation warrants restricting otherwise valid expressions of media types/ranges in a structured field. i think the only restrictions should be the minimum actually necessary so that an expression of a media type/range is syntactically valid as both a media-range and a structured field, thereby maintaining as much compatibility as possible with the media-type ecosystem (and all of the different normative and common forms of different media types) while conforming to Structured Field syntax.

@reschke
Copy link
Contributor Author

reschke commented Dec 5, 2024

@mnot , @martinthomson , @MikeBishop - absent further comments I plain to "merge" this soonish.

"merge" in double quotes, as git does not let me. So I'll first create a new PR, link to that from here, and then close this one.

@reschke
Copy link
Contributor Author

reschke commented Dec 5, 2024

Superseded by #2966

@reschke reschke closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants