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

Possible issue with Expansion handling of @reverse #294

Closed
kasei opened this issue Jan 1, 2020 · 18 comments
Closed

Possible issue with Expansion handling of @reverse #294

kasei opened this issue Jan 1, 2020 · 18 comments

Comments

@kasei
Copy link

@kasei kasei commented Jan 1, 2020

Based on test t0066, I think there is a problem with the handling of @reverse data in Expansion steps 13.4.13.3 and 13.4.13.4.

13.4.13.2 sets the value of expanded value to the result of a recursive call to the Expansion algorithm, and then 13.4.13.3 and 13.4.13.4 both test expanded value as if it were a map. However, I think based on the current text of the expansion algorithm, expanded value will always be an array at this point. Is it possible 13.4.13.3 and 13.4.13.4 need to loop over all values returned from the recursive call instead of operating directly on expanded value?

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

Looks like we changed the wording in PR #175 to always return an array, where before, it would return an object; I don't recall now why we did this.

But, 13.4.13.2 needs to be updated, and perhaps others; I'll have a look around.

@gkellogg gkellogg added the wr:open label Jan 3, 2020
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

This also goes back to changes in #184. I think the change to always return an array came about because there is text at the end of the 1.0 algorithm, which is missing:

If, after the above algorithm is run, the result is a JSON object that contains only an @graph key, set the result to the value of @graph's value. Otherwise, if the result is null, set it to an empty array. Finally, if the result is not an array, then set the result to an array containing only the result.
This paragraph, which is run after the algorithm, so is not part of the recursive algorithm, got rolled into steps 20-22. We should probably revert the change from #184 which moved these up, and go back to the 1.0 description, so that when the expansion algorithm is passed an object, it typically returns an object, and not an array. We can re-examine some of the other changes that came as a consequence of this.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

From #175:

It is not clear to me if this post-handling of the result is applied universally to all values returned out of the expansion algorithm. For example, step 1 ends return null. Why use null here instead of an empty array if the post-processing will always do the conversion?

I think what was unclear here is that this paragraph only applied to the end of the outermost layer of processing, where we are returning a normalized form (without sole outer @graph and as an array). Perhaps changing the text to say "After the above algorithm is run at the outermost level, perform the following steps before final completion of the algorithm:"

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

I opened #175 because having algorithm step-like rules included in prose and not in numbered steps was confusing. I think the need for language like "the outermost level" already hints at the fact that such prose is trying to make up for the fact that what is being described is really two different algorithms. If it's not possible to describe it explicitly as two separate algorithms, I much prefer the current approach of having everything be a part of the numbered steps, but it's clear that the specifics of the current text might still need some work.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

(After reading your comments on my gist, here's a fuller response:)

FWIW, I would strongly prefer not reverting to the 1.0 language without updates to the language used at most of the places where the expansion algorithm is called. Ideally I think this would be better resolved by breaking up Expansion into multiple sub-algorithms:

  • the main body of the current expansion algorithm
  • steps 13 and 14 which currently require the use of the "recursively repeat" language (which differs from all other uses of "recursive" invocation in the spec)
  • the 1.0-era post-processing text, but described using algorithmic steps, not prose

I understand CR makes this sort of change difficult, but I think it would result in a much simpler description of the expansion algorithm.

gkellogg added a commit that referenced this issue Jan 3, 2020
…a paragraph following the algorithm steps, into the `expand()` API method to be performed after the algorithm has completed.

This restores previous desired behavior that the expansion algorithm typically results in a map when starting with a map, and not an array containing that map.

For #294.
@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

@kasei this should resolve this particular issue, and some related issues where the result of the expansion algorithm was expected to be a map, not an array, which was inadvertently introduced in PR #184.

Please acknowledge if this satisfies the issue.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

@gkellogg FYI, #299 fixed 4 tests for me, but caused regression in 11 others. I'll look into them and see what's going on.

@gkellogg gkellogg added this to Editorial work complete in JSON-LD Management Jan 3, 2020
@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

@gkellogg first issue I see is with the new expand JsonLdProcessor text step 7.1:

If expanded output is a map that contains only an @graph entry, set expanded output that value.

By moving this out of the main Expansion algorithm, I think test t0084 ends up with a double @graph (set in 13.8.3.7.1).

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

I think step 13.8.3.7.1 should say:

If container mapping includes @graph, and item is not a graph object, set item to a new map containing the key-value pair @graph-item, ensuring that the value is represented using an array.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

That improves the situation, but I think the following text about ensuring index is an array is still required.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

Can you point out in the updated PR, where text is missing?

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

Oh, actually, I think this might instead be about the JsonLdProcessor expand step 7.3. By pulling that out of the Expansion algorithm, t0084 ends up with "@graph": {...} instead of "@graph": [{...}].

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

Expected result is the following:

[{
  "http://example.org/input": [{
    "@index": "g1",
    "@graph": [{
      "http://example.org/value": [{"@value": "x"}]
    }]
  }]
}]

This would come from step 13.8.3.7.1 saying "ensuring that the value is represented using an array".

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

@gkellogg I'm now confused. The suggested text change to 13.8.3.7.1 will result in that step not being processed in this case, because item is a graph object. So we're left with an item that is a map, that I think results in output containing "@graph": {...} (without an array).

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

I think this might be solved by adding a new step 13.8.3.7.1a (between .1 and .2):

Otherwise, if item is a graph object and the value of @graph in item is not an array, set it to a new array containing only that value.

or words to that effect.

Using the #299 language, this change fixes 6 test failures for me (t0084, t0087, t0098, t0101, t0105, t0106) without causing any regressions.

@gkellogg

This comment has been minimized.

Copy link
Member

@gkellogg gkellogg commented Jan 3, 2020

It should already be in an array form because of 13.4.5:

If expanded property is @graph, set expanded value to the result of using this algorithm recursively passing active context, @graph for active property, value for element, and the frameExpansion and ordered flags, ensuring that expanded value is an array of one or more maps.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

Yes, you're right. I had a bug in 13.4.5.

@kasei

This comment has been minimized.

Copy link
Author

@kasei kasei commented Jan 3, 2020

@gkellogg I'm happy with #299, thanks.

gkellogg added a commit that referenced this issue Jan 3, 2020
…a paragraph following the algorithm steps, into the `expand()` API method to be performed after the algorithm has completed.

This restores previous desired behavior that the expansion algorithm typically results in a map when starting with a map, and not an array containing that map.

For #294.
kasei added a commit to kasei/perl-jsonld that referenced this issue Jan 6, 2020
@gkellogg gkellogg closed this Jan 10, 2020
@gkellogg gkellogg removed this from Editorial work complete in JSON-LD Management Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.