Expand expansion map to support calling when a relative IRI is detected

[tplooker]

As documented in #199 when the value of @type or @id is a relative IRI it is silently dropped when the resulting expanded object is converted to an RDF representation because RDF does not support relative IRIs. This is problematic for cases where a JSON-LD document is being digitally signed as information can be silently dropped.

This PR expands the expansionMap function so that it is called when a relative IRI is encountered when expanding an @id or @type value. This then allows libraries like jsonld-signatures to throw errors through a custom strict expansion map.

[dlongley]

@tplooker, Thanks heaps! A PR to address this has been a long time coming.

The implementation here has me a bit worried that we may be missing some other cases though, based on it explicitly targeting @id and @type in _expandObject. For example, I think it will miss the usage of @type in values. Perhaps it should instead target _expandIri. So we'd always pass the expansionMap in there (perhaps as another processing option), and then use the new code from this PR when base: true is in relativeTo (and we may also need to cover when vocab is relative and falls back to using base).

@davidlehn, what are your thoughts?

[OR13]

Super excited to start throwing errors instead of silently dropping terms... this is huge.

[tplooker]

@dlongley @davidlehn I have updated the implementation as per your suggestion @dlongley, can you suggest some further test cases to verify this behaviour?

[tplooker]

Outstanding questions

• Do we need similar protections around @vocab usage?
• Do we need to bubble up more info other than the value of the relative Iri in the expansion map, such as the property (if there is one) it pertains to? So that a strict expansion map can suitably error report when the relative iri is for an @type and @id property

[dlongley]

Do we need similar protections around @vocab usage?

Since @vocab can be relative and depend on @base (via @context or via API options), we'll need to ensure we cover that case (I didn't look at the code to see what the simplest change is for that case). We'll want a test for this case. Ultimately, we'll want expansionMap tests for relative URIs that appear with any keyword (whether in a node object or literal value) that accepts them.

Do we need to bubble up more info other than the value of the relative Iri in the expansion map, such as the property (if there is one) it pertains to? So that a strict expansion map can suitably error report when the relative iri is for an @type and @id property?

That does sound useful.

[tplooker]

Since @vocab can be relative and depend on @base (via @context or via API options), we'll need to ensure we cover that case (I didn't look at the code to see what the simplest change is for that case). We'll want a test for this case. Ultimately, we'll want expansionMap tests for relative URIs that appear with any keyword (whether in a node object or literal value) that accepts them.

Ok I have added a test for when @vocab has a value of ./ but im not sure I have got all other cases yet.

That does sound useful.

Ok any suggestion on the approach here because at the moment the term/property the value is being expanded against in the expandIri function is not in scope so we would need to thread this in from every possible call of the expandIri function?

[dlongley]

Ok any suggestion on the approach here because at the moment the term/property the value is being expanded against in the expandIri function is not in scope so we would need to thread this in from every possible call of the expandIri function?

Yeah, let's punt that to a future PR if it becomes clear that users really need the additional information (we're just guessing right now). It's more important to enable some kind of error to be thrown here via expansion maps.

[dlongley]

Thanks, @tplooker!

Approving with some style-nits and a suggested test for typed literals.

@davidlehn -- can you please take a look at this?

[tplooker]

@dlongley @davidlehn I have now added another expansionMap hook called "prependIri" which is fired whenever a values expansion is going to occur that will involve it being prepended with either the value of '@base' or '@vocab'. This should allows us at the JSON-LD signatures layer to build an expansion map that at the very least warns a caller that an expansion occured using @base which could be un-expected in certain cases, such as the case outlined here

[dlongley]

I think we can simplify the logic here and consolidate the new code into one place with a single conditional on just relativeTo.base at the top-level ... and then getting the base value from @base in activeCtx and falling back to options.base. That would also make sure we still use the expansionMap when activeCtx['@base'] is null ... because I think we may be failing to do so here at the moment.

@davidlehn -- any feedback on what we're passing the expansionMap handler here?

[tplooker]

@dlongley thanks I agree, have updated the PR accordingly.

[dlongley]

@tplooker -- I think the approach here is ok and I've asked for bikeshedding feedback (if any) on the naming. See my comment ... I noticed we may have a gap when base is null that we could avoid by also making the code more DRY.

[tplooker]

@tplooker -- I think the approach here is ok and I've asked for bikeshedding feedback (if any) on the naming. See my comment ... I noticed we may have a gap when base is null that we could avoid by also making the code more DRY.

Ready for review again, can you elaborate on this case some more? We have coverage over the case when activeCtx['@base'] is null but not when options.base is, is that what you mean?

[dlongley]

@tplooker,

Ready for review again, can you elaborate on this case some more? We have coverage over the case when activeCtx['@base'] is null but not when options.base is, is that what you mean?

What I mean is that we don't call the expansionMap prepend handler when activeCtx['@base'] is falsy -- which means relative IRIs can sneak in without the handler having the opportunity to evaluate whether the value was to be expanded using @vocab or @base or whether the value was associated with an @type.

Additionally, unless I missed it, I didn't see where we signal whether the value is associated with @type or not. This will be key information for use cases such as jsonld-signatures in determining whether or not to throw an error -- as I fully expect us to want to throw if @base is being applied to a value associated with @type. We should add a flag such as typeExpansion that can indicate whether the value is associated with @type, default it to false and only set it to true where we are expanding @type values.

[tplooker]

@dlongley thanks I think I've addressed all of the points you raised in your last review, ready for another pass.

[dlongley]

Thanks, @tplooker, this is awesome! LGTM -- @davidlehn any thoughts before this gets pulled in to finally address #199?

[tplooker]

Hey @dlongley @davidlehn anything outstanding with this PR, keen to merge when possible so I can continue to work upstream?

[dlongley]

@tplooker -- Just letting you know that I see this and that I'm going to get @davidlehn to have a look.

[davidlehn]

• I have been following along, but wasn't sure how to review. I think we'll put this out there and iterate later if needed.
• The lack of docs on these sorts of features is a problem, but that's a larger issue to tackle later. We'll at least have the tests for people to look at.
• I'll add the changelog entry. In the future, nice to add that in the PRs.
• There's probably some overlap with what I was trying to do in Add event handler #366. Will have to explore if these features can be combined somehow later.

[OR13]

was there a release that included this?

[dlongley]

No, because we're working on a new "safe mode" feature right now that more cleanly and efficiently addresses the problem of "lossy" JSON that goes through JSON-LD transformations.

[dlongley]

@OR13 -- a release was made with the new safe mode that does a more low-level and targeted approach to solving the "undefined term / relative URL" problem with a much simpler API (set safe: true in the options). The latest version of jsonld.js (v8) now sets safe: true by default in canonize so that method will fail closed if you don't define your terms or use absolute URLs. You can override the default settings of course, as usual.