search

w3c / json-ld-api

JSON-LD 1.1 Processing Algorithms and API Specification
https://w3c.github.io/json-ld-api/
Other
73 stars 29 forks source link

Add graph container array tests. #585

Open davidlehn opened 6 months ago

davidlehn commented 6 months ago

The expand/toRdf parallel testing and number is a bit confusing. Not all the tests have a twin. I jumped up to 0200 thinking maybe going forward from there new tests might align better.

As of now, different implementations have different results for these sorts of tests. For instance, for 0203:

People have been looking at this construct in places like the VC work. Providing URLs in the verifiableCredential property of a VP (I think). This additional coverage is needed to understand what should happen here unless this is specifically undefined behavior. (I'm not sure if some tests duplicate other tests, added variations for clarity.)

I'm not sure what the output should be of these tests. Currently it has the jsonld.js empty output, but that may not be correct in all cases?

What other variations of these tests should be added?

Also related is what to do with a "@container": ["@graph", "@id"] with only a IRI provided instead of a mapping. I'm not sure that has test either. It outputs <ex:outer> <ex:p> <ex:test> ./... "ex:test" triples in jsonld.js and others, which seems closer to the behavior people might expect without the @id, but the feature was designed for only mappings instead of values like this?

gkellogg commented 5 months ago

The implication in the Expand Algorithm step 13.12.1 is that if must either be a node object or a graph object, and not a value object. I agree that if it is a value object, should be omitted. However, I do get an expanded output after accounting for this:

[
  {
    "@id": "ex:outer",
    "ex:p": []
  }
]

I don't know of a step in the expansion algorithm which would eliminate this entry.

It did require a change in 13.12.1 to eliminate expanded values which are not either node objects or graph objects if container includes @graph. We may need an Erratum and issue to add wording to this effect.

gkellogg commented 4 months ago

This issue was discussed in a meeting.

PR w3c/json-ld-api#585
https://github.com/w3c/json-ld-api/pull/585 -> Pull Request 585 Add graph container array tests. (by davidlehn) [test:missing-coverage]
David I. Lehn: Do we have other tests that have a property expanding to an empty array.
... It doesn't change the N-Quads output, but there may be some ambiguity on the expected results.
Gregg Kellogg: It might be a warning condition if the result contains a property with no values.
gkellogg commented 2 months ago

If we are to make a significant change to the spec for handling this corner case (which, IIUC, would be required to justify these test results), I think I would rather lean towards raising an error on these input.

The way we've handled other such issues is to create an erratum and note it as a substantive change. Our charter allows us to address errata, which IMO would include this. Tests are appropriate once the erratum is accepted and changes have been added to the draft.