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

Reserved expansion dictionary explode alternate ordering #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexdeem
Copy link
Contributor

@alexdeem alexdeem commented Jan 8, 2023

This change brings these tests into line with those above by providing a list of acceptable expansions, one for each possible ordering of the exploded dictionary elements.

My implementation of RFC6570 (https://github.com/SwiftScream/URITemplate) happened to fail these due to it producing the alternate ordering.

@mnot
Copy link
Member

mnot commented Jan 16, 2023

They're specified as associative arrays, which is ordered:

     keys  := [("semi",";"),("dot","."),("comma",",")]

For a variable that is an associative array, expansion depends on
both the expression type and the presence of an explode modifier. If
there is no explode modifier, expansion consists of appending a
comma-separated concatenation of each (name, value) pair that has a
defined value. If there is an explode modifier, expansion consists
of appending each pair that has a defined value as either
"name=value" or, if the value is the empty string and the expression
type does not indicate form-style parameters (i.e., not a "?" or "&"
type), simply "name".

@alexdeem alexdeem force-pushed the reserved-expansion-dictionary-explode-alternate-ordering branch from 6d98913 to 0b0907b Compare January 16, 2023 11:44
@alexdeem
Copy link
Contributor Author

This is interesting.

I appreciate that the wording of the RFC implies there is an ordering; however:

  • There are several examples of alternate orderings being accepted in similar cases in the existing tests12
  • The test files provide these "associative array" variables as JSON objects, which are "an unordered collection of ... name/value pairs" see rfc8259.

This second point is primarily the problem with my implementation: the built-in JSON parsing in Swift presents these variables as an unordered collection, and enumeration of the contents happens to differ from the order they appear in the file.

I think a very common case is to provide an unordered collection of key/value and be ambivalent about the resulting ordering, eg URL query strings.

I feel we should either:

  1. Admit that ordering is unspecified for these json object variables, and merge this PR; or
  2. Decide that ordering is important: specify the associative array variables in a way that defines ordering (ie. not a JSON object), and remove the alternate orderings for the tests that currently allow them. I'm happy to help with this if it is the direction.

Footnotes

  1. https://github.com/uri-templates/uritemplate-test/blob/master/extended-tests.json#L92

  2. https://github.com/uri-templates/uritemplate-test/blob/master/extended-tests.json#L106-L121

alexdeem added a commit to SwiftScream/URITemplate that referenced this pull request Jan 20, 2023
alexdeem added a commit to SwiftScream/URITemplate that referenced this pull request Jan 20, 2023
@andreaTP
Copy link

I faced the very same issue rolling an implementation with Go, more specifically, loading a Map from Json is not going to preserve the ordering: golang/go#27179

I'm in favor of:

  1. Admit that ordering is unspecified for these json object variables, and merge this PR;

But I'm open to hearing additional opinions.

@andreaTP
Copy link

andreaTP commented Sep 4, 2023

cc. @mnot have you had time to check this one further?
I can confirm that the issue is present for Go and Swift and it forces us to use an additional ordering to get the tests passing.

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.

3 participants