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.

Sign up for GitHub

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

Clarify parsing of @httpQueryParams members #957

Merged
merged 1 commit into from
Nov 9, 2021
Merged

Clarify parsing of @httpQueryParams members #957

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions docs/source/1.0/spec/core/http-traits.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ request URI and labels which are used to insert named components into the
request URI.

The resolved absolute URI of an operation is formed by combining the URI of
the operation with the endpoint of the service. (that is, the host and any
base URL of where the service is deployed). For example, given a service
the operation with the endpoint of the service. (that is, the host and any
base URL of where the service is deployed). For example, given a service
endpoint of ``https://example.com/v1`` and an operation pattern of
``/myresource``, the resolved absolute URI of the operation is
``https://example.com/v1/myresource``.
Expand Down Expand Up @@ -1067,6 +1067,49 @@ An example HTTP request would be serialized as:
POST /things?thingId=realId&otherTag=value
Host: <server>

When deserializing HTTP request query string parameters into members with the
``httpQueryParams`` trait, servers MUST treat all values as strings and produce
empty string values for keys which do not have values specified. For example,
given the following model:
Comment on lines +1070 to +1073
Copy link
Contributor

@JordonPhillips JordonPhillips Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't quite right.

Suggested change
When deserializing HTTP request query string parameters into members with the
``httpQueryParams`` trait, servers MUST treat all values as strings and produce
empty string values for keys which do not have values specified. For example,
given the following model:
When deserializing HTTP request query string parameters into members with the
``httpQueryParams`` trait, servers MUST treat parameters without values as
empty strings. For example, given the following model:

In summary:

  • ?queryParam => {"queryParam": ""}
  • ?queryParam= => {"queryParam": ""}
  • ? => {"queryParam": null}

Though.... that does kinda imply you can't have a sparse list, which I don't think we enforce.... I'll need to investigate that more Nevermind a sparse list would just omit that index I really need to talk to some of the other folks when they wake up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your point, are you referring to the fact that values could be split on commas to be treated as sets/lists of strings? Anything else is forbidden by the trait's selector, right? If so, it might be worth adding this to the documentation too (I'm happy to do it here), e.g. is splitting on commas correct/standardised, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And in your third example, would ? not simply yield {}? Or in the example given, tags: {}?

Copy link
Contributor

Choose a reason for hiding this comment

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

To your point, are you referring to the fact that values could be split on commas to be treated as sets/lists of strings?

No, query string lists duplicate the value. (Except in some aws protocols that embed the index as part of the key, which is causing some of my confusion). I really need to talk to the rest of the team about this when they're awake

Copy link
Contributor

Choose a reason for hiding this comment

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

Jordon's correct, you get list values from repeating the key, like a=x&a=y&a=z yielding a: ["x", "y", "z"]. There's no way to introduce an explicit null value -- it's either not defined, or it's a non-null string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, based on this discussion, I've added a last example of the k= case as well. I am happy to accept the above suggestion but feel the original is better because it clarifies (or at least, it was my intent to clarify) that there should be no parsing/treatment of values as anything other than strings. For instance, if I parse k1=true&k2=24 into @httpQueryParams, the fact that it is a map { key: String, value: String } shape means I get { "k1": "true", "k2": "24" } and not e.g. { "k1": true, "k2": 24 } (note the parsing of the boolean/number). Thoughts? Apologies if I've missed the point.


.. tabs::

.. code-tab:: smithy

@http(method: "POST", uri: "/things")
operation PostThing {
input: PostThingInput
}

structure PostThingInput {
@httpQueryParams
tags: MapOfStrings
}

map MapOfStrings {
key: String,
value: String
}


And the following HTTP request:

::

POST /things?thingId=realId&otherTag=true&anotherTag&lastTag=

A server should deserialize the following input structure:

.. code-block:: json

{
"tags": {
"thingId": "realId",
"otherTag": "true",
"anotherTag": "",
"lastTag": ""
}
}

.. smithy-trait:: smithy.api#httpResponseCode
.. _httpResponseCode-trait:
Expand Down