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

Prefix modifier and exploded tests #27

Closed
wants to merge 1 commit into from
Closed

Prefix modifier and exploded tests #27

wants to merge 1 commit into from

Conversation

fxa
Copy link

@fxa fxa commented Jan 31, 2013

Hi there,
during refactoring of my varspec parser (cyclomatic complexity reached 20 meanwhile!) I found, that the parser misses some strange corner cases.
One was {var:011} -> my parser used the build in parseInt, which interpreted 011 as octal number and returned 9. {var:08} parsed only the 0, so the length was zero. {var:} returned a NaN. {?} returned a varspec with an empty name and so on.

@hannesg
Copy link
Contributor

hannesg commented Jan 31, 2013

Good catch! Found two bugs on my implementation.
I'm only 95% sure that it's correct that empty arrays and assocs are allowed with prefix modifiers. The spec say they are undefined, but does that mean they aren't arrays and assocs? I guess so, but I'm wondering what the use-case for that could look like.

@mnot
Copy link
Member

mnot commented Feb 4, 2013

The spec says: "A prefix modifier indicates that the variable expansion is limited to a prefix of the variable's value string."

Note variable's value string.

So, I think that some of those "false"s shouldn't be there...

@fxa
Copy link
Author

fxa commented Feb 6, 2013

Hi Mark,

I sent You the following tests:
["{text:0}", false],
["{text:08}", false],
["{text:8}", "I%20like%20o"],
["{text:9999}", "I%20like%20octal%20numbers"],
["{text:10000}", false],
["{text:1.0}", false],
["{text:1e+2}", false],
["{text:9876543210987654321098765432109876543210}", false],
["{emptyObject:1}", ""],
["{filledObject:1}", false],
["{emptyArr:1}", ""],
["{filledArr:1}", false],
["{text:1:1}", false],
["{?}", false],
["{?:1}", false],
["{?a,:1}", false],
["{
}", false],
["{text*}", false],
["{text:1
}", false],
["{text*:1}", false]

I think, you are worried about the following test cases:
["{filledObject:1}", false],
["{filledArr:1}", false]

All others are clear (to me).
What about, if I make a new pull request without the two “critical” test cases and we discuss these two separately?

@hannesg
Copy link
Contributor

hannesg commented Feb 7, 2013

@mnot Can you be a bit more precise? Does that mean that a prefix modifier is ignored for non-strings? or that they get expanded and then truncated? I always interpreted section 2.4.1

Prefix modifiers are not applicable to variables that have composite values.

as "expanding a list or associative array (=composite value) with a prefix modifier is an error", because the standard doesn't contain any example covering this case.

@fxa 👍

@damnhandy
Copy link
Contributor

I think what @mnot is getting at is if these are negative tests case (which they are) they likely belong in the negative-tests.json file rather than extended-tests.json .

As far as the strings vs. composite values goes, I've interpreted section 2.4.1 as an error case and treating it as such in my impl. I think what @mnot is getting at is that prefix values are only every appropriate for a single value (i.e. not an array or assoc). I should note that "string" here could be a loaded term. For example, if the value of "text" were 300 and you had the following expression:
{text:1}

I would expect the result to be "3". The end result is still a string.

As far as the tests @fxa sumitted, I think it's a matter of where the tests should go. Since these are testing an error condition, these cases should be moved negative-tests.json

@fxa
Copy link
Author

fxa commented Feb 7, 2013

@mnot
So you think "{?list:1}" with {list:["red", "green", "blue"]} shall be expanded to "?list=red,green,blue"?
(Or to "?list=r,g,b"??)

@damnhandy
Copy link
Contributor

@fxa My understanding of the spec is that would be an error condition. It is definitely not "?list=r,g,b or even ?list=red

@fxa
Copy link
Author

fxa commented Feb 7, 2013

@damnhandy
I am with you, Ryan!

@fxa
Copy link
Author

fxa commented Mar 7, 2013

ping!
mnot, any comments?

@mnot
Copy link
Member

mnot commented Mar 18, 2013

Sorry, been busy / on the road.

2.4.1 says that prefixes aren't applicable to composite values, but composite values are denoted by a "*", which these tests don't have. So, my interpretation is that you calculate the string, then truncate.

Also, it's surprising (in a not good way) that you can successfully evaluate a prefix on an empty object, but not one that has values.

Therefore, "{filledObject:1}" would be "c".

@royfielding any comment here?

@fxa
Copy link
Author

fxa commented Jul 13, 2013

ping!
The last commment is 4 months old.
We should come to a solution

@mnot
Copy link
Member

mnot commented Aug 29, 2013

Any thoughts about my comments above? That's my reading still.

@fxa
Copy link
Author

fxa commented Sep 15, 2013

Ok, let's recap:

  • combination of prefix values and combined values are not allowed, so {a:1*} should result in a parsing error
  • when prefix values are applied to values, that are not simple strings, these values are first converted to a string and then truncated.
  • Example: "{?list:1}" with {list:["red", "green", "blue"]}. The value will be converted to a string, that is "red,green,blue" and than truncated to "r". So the result is "?list=r"

I will withdraw the pull request and make a new one.

@damnhandy
Copy link
Contributor

The first bullet a fully agree with. The second two I do not. As @hannesg pointed out, section 2.4.1 of the spec states that:

Prefix modifiers are not applicable to variables that have composite values.

I think that is pretty clear. With that said {?list:1} should raise an error. The spec doesn't suggest anything else.

@hannesg
Copy link
Contributor

hannesg commented Sep 15, 2013

@damnhandy I now believe that I was wrong. I guess the term "composite value" doesn't refer to lists in this context but to explode modifiers ( explained one section later ). From my understanding {?list:1} is valid.

@damnhandy
Copy link
Contributor

Not sure. Section 2.3 makes a pretty clear definition as to what a composite value is:

In Level 4 templates, a variable may have a composite value in the
form of a list of values or an associative array of (name, value)
pairs. Such value types are not directly indicated by the template
syntax, but they do have an impact on the expansion process
(Section 3.2.1).

I read that as a composite value is a list or associative array. As we dig further down into section 2, I don't see anything that changes what the notion of a "composite value" is or anything that changes it context. So when I see

Prefix modifiers are not applicable to variables that have composite values.

I read

Prefix modifiers are not applicable to variables that are list or associative arrays values.

@fxa
Copy link
Author

fxa commented Sep 15, 2013

That’s the reason, why we are talking 7 months about that point…

Von: Ryan J. McDonough [mailto:notifications@github.com]
Gesendet: Sonntag, 15. September 2013 21:28
An: uri-templates/uritemplate-test
Cc: Franz X Antesberger
Betreff: Re: [uritemplate-test] Prefix modifier and exploded tests (#27)

Not sure. Section 2.3 http://tools.ietf.org/html/rfc6570#section-2.3 makes a pretty clear definition as to what a composite value is:

In Level 4 templates, a variable may have a composite value in the
form of a list of values or an associative array of (name, value)
pairs. Such value types are not directly indicated by the template
syntax, but they do have an impact on the expansion process
(Section 3.2.1).

I read that as a composite value is a list or associative array. As we dig further down into section 2, I don't see anything that changes what the notion of a "composite value" is or anything that changes it context. So when I see

Prefix modifiers are not applicable to variables that have composite values.

I read

Prefix modifiers are not applicable to variables that are list or associative arrays values.


Reply to this email directly or view it on GitHub #27 (comment) . https://github.com/notifications/beacon/z007K-oJ-ZnzAnsfdzRoGZgEIF6LPmV4TCs6vA0sVXxTsVgkwTCzKSbVW93cz0DQ.gif

@damnhandy
Copy link
Contributor

Hmm, it seems that section 2.4.2 is suggesting that the * modifier is a composite value, and this is confusing. I've been thinking that list and associative arrays are always considered "composite values" and the * explode modifier is use to modify how the composite value is rendered in the expanded URI. The fact that the term "composite" is use in different ares of the spec is somewhat confusing.

@damnhandy
Copy link
Contributor

So recapping, the second to last paragraph of section 2.3 states that a composite value is a list and associative array. It doesn't yet make mention of the explore modifier. Section 3.2.1 also suggests that there's ambiguity around what a composite value is. Going back to section 2.4.2 after reading section 3.2.1 again, I read this section as merely how the expansion process is altered with the presence of the explode modifier. With that said, given the variable values:

list := ("red", "green", "blue")

We all agree that this is not valid:

{?list:1*}

Nothing in the spec suggests otherwise. The crux of the issue is:

{?list:1}

There are a few possible answers:

  1. ?list=red
  2. ?list=r,g,b
  3. ?list=r
  4. error

I'm in the error camp since there is little in the spec to suggest what the correct would be if it were a valid case to expand a list of associative array with a prefix modifier.

BTW @fxa, I'm a little slow sometimes so sorry for the seven month lag time ;)

@mnot
Copy link
Member

mnot commented Sep 17, 2013

@royfielding can you take a look at this please? I think we need an errata to clear this up.

@fxa
Copy link
Author

fxa commented Sep 23, 2013

I reported an errata to the rfc at http://www.rfc-editor.org/errata_search.php?rfc=6570

@royfielding
Copy link
Member

There was a time when the draft spec allowed just about anything, including prefixes on list values, to avoid run-time error cases. However, the end result was that it caused more confusion than it is worth. Hence, the RFC says that a prefix operator is not applicable to list values, as Ryan indicated. However, a template does not reveal that a given variable is a list or a string, so the processor won't know that this is an error until fairly late in the process. The spec deliberately does not require a specific form of error handling here -- the processor can either indicate an error or treat the list value as one string and take a prefix of the entire string (not the prefix of each list value).

Yes, there is a definition for composite values an explode operator that impacts composite values. I don't find that confusing.

@fxa
Copy link
Author

fxa commented Sep 28, 2013

Hi all!
So it is both valid.
When
list := ("red", "green", "blue")
and
template := {?list:1}
then for template.expand(list) both

  • error
  • "?list=r"

are valid results from the processor (damnHandy's #3 and #4)?
When both are valid, we should add both to the test project.
Is this common sense? Then I will close this issue and add a new one

@damnhandy
Copy link
Contributor

I see. I recall older versions of the spec having a lot more features such as join operators and a means to flag a variable as being a collection of values. Obviously, {?list:1} can't indicate that its value is an array or associative array and a processor would only know that at expansion time. I now see @mnot 's and @hannesg 's interpretation and I think I now agree with them.

@fxa, I'm now not sure how we'd be able to express both in the test suite. For implementors, you're either doing it one way or the other and not both, so having both in there could be more confusing. It may be that we drop the tests?

@fxa
Copy link
Author

fxa commented Sep 28, 2013

Hi Ryan,
I think, dropping the test is an bad idea. But the json-structure allows multiple results. So I think of something like

{
"strange tests": {
"variables": {
"list": ["red", "green", "blue"]
},
"testcases": [
"{?list:1}": [
null,
"?list=r"
]
]
}
}

@fxa
Copy link
Author

fxa commented Oct 5, 2013

Hi Mark,
this issue is quite of outdated. So I would like to do

  • close this pull request
  • add a new one with the remaining accepted test cases (after inspecting the meanwhile changes)
  • after that add a new pull request with a new file containing the challenging test cases with the new result
    <array with null or "something">
    Is that ok for you?

@mnot
Copy link
Member

mnot commented Feb 17, 2014

Hey,

Sorry for the delay. Sounds good.

@fxa
Copy link
Author

fxa commented Feb 17, 2014

Ok, I will rearange the tests and make a new request

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.

None yet

5 participants