Skip to content

Base URL is not always absolute in Context Processing #369

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

Closed
kasei opened this issue Feb 14, 2020 · 12 comments · Fixed by #377
Closed

Base URL is not always absolute in Context Processing #369

kasei opened this issue Feb 14, 2020 · 12 comments · Fixed by #377

Comments

@kasei
Copy link
Contributor

kasei commented Feb 14, 2020

(Related to #265, #356)

Context Processing step 5.2.1:

Initialize context to the result of resolving context against base URL.

This base URL comes directly as the argument to this algorithm. However, in at least one case, the value of base URL is not an absolute IRI.

The expand() defintion in JsonLdProcessor Interface step 5 says:

If the expandContext option in options is set, update the active context using the Context Processing algorithm, passing the expandContext as local context and expandContext as base URL.

In test t0077, the value of "expandContext" is simply "0077-context.jsonld", which will cause Context Processing to attempt to resolve a relative IRI against another relative IRI. I don't think that's what is intended here.

@gkellogg
Copy link
Member

See #370 (comment), but in this case "expandContext" should have "@type": "@id", which means that "0077-context.jsonld" would be resolved relative to the manifest location, so would be https://w3c.github.io/json-ld-api/tests/0077-context.jsonld.

The convention had been to resolve the expandContext against the input file location, but I don't see any discussion of this. This would be https://w3c.github.io/json-ld-api/tests/expand/0077-context.jsonld, which is the intent.

The manifest context should be updated to set @type: @id on expandContext and the value changed to "expand/0077-context.jsonld", which would generate a valid absolute IRI when processing.

@gkellogg gkellogg self-assigned this Feb 14, 2020
gkellogg added a commit that referenced this issue Feb 14, 2020
@gkellogg
Copy link
Member

Pending fix in #377.

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

@gkellogg I think it's a very bad idea to require correct JSON-LD expansion of the test manifest in order to correctly run the tests. Changes in #377 are nice to have, but I don't think requiring that sort of processing should be necessary.

@gkellogg
Copy link
Member

Manifests are RDF documents, you can parse them to RDF and use them that way, as you might with Turtle or SPARQL. I typically turn them into JSON-LD for convenience.

Everywhere else a property is an IRI it is typically given as a relative value, this is really no different. Why would expandContext be treated any differently than input or result?

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

Manifests are RDF documents

Am I right in saying that the current manifests are only available in JSON-LD and HTML? If so, I don't think it's reasonable to assert that they're to be treated as RDF documents, as that means that anyone wanting to run the JSON-LD tests must already have access to a fully conforming JSON-LD implementation of the expansion and toRdf algorithms. If you make the manifests available in another (simpler) RDF format my objection here would be reduced, but I can't understand requiring a fully confirming JSON-LD implementation to support testing a JSON-LD implementation.

@gkellogg
Copy link
Member

Turtle manifests are in Turtle (as are N-Triples). Whether parsed as RDF, expanded, or used as is (that is the advanatage of JSON-LD, after all) it’s important to be consistent. The expandContext as a string is interpreted as a URL.

But, I’m really not clear on what point you’re going for and what change would satisfy you.

@gkellogg
Copy link
Member

Updated in #377.

@kasei
Copy link
Contributor Author

kasei commented Feb 15, 2020

Turtle manifests are in Turtle (as are N-Triples).

With all due respect, JSON-LD is substantially more complicated than Turtle and N-Triples, and neither of those manifests is more complicated than listing an input file, and an expected output file.

The expandContext as a string is interpreted as a URL.
But, I’m really not clear on what point you’re going for and what change would satisfy you.

I think at the very least, the manifest documentation needs to call this out as a field that needs to be treated as a URI (and so resolved before the test is run).

gkellogg added a commit that referenced this issue Feb 16, 2020
@gkellogg
Copy link
Member

I added wording in the README and manifest HTML documents about this. It can also be inferred by looking at the context definition in tests/context.jsonld.

gkellogg added a commit that referenced this issue Feb 16, 2020
… containing manifest.

For #369.

(cherry picked from commit 01dd367)
@kasei
Copy link
Contributor Author

kasei commented Feb 17, 2020

OK. The new wording looks reasonable.

More generally, though, I'm having a hard time figuring out why I'm running into all of these issues trying to track the new changes, when I was previously able to pass the entire expansion test suite based on just the changes I listed in #265. None of this used to cause be trouble, and with the proposed changes in #356, I'm scrambling to figure out why things are broken again in parts of the algorithm that don't seem related to tracking the @context URLs (at least not related in ways that would affect my extensions to the previous spec text that allowed me to pass all tests).

@kasei
Copy link
Contributor Author

kasei commented Feb 17, 2020

@gkellogg Strike that last comment. Not sure it looks good. The updated text says:

Expansion tests may have a expandContext option, which is treated as an IRI relative to the manifest.

However, test t0077 has:

      "option": {
        "expandContext": "0077-context.jsonld"
      },

That isn't relative to the manifest, it has to be relative to one of the test input files (which are one path segment down from the manifest in /expand/).

@gkellogg
Copy link
Member

It's fixed in the PR:

"expandContext": "expand/0077-context.jsonld"
.

gkellogg added a commit that referenced this issue Feb 18, 2020
gkellogg added a commit that referenced this issue Feb 18, 2020
… containing manifest.

For #369.

(cherry picked from commit 01dd367)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants