Closed tplooker closed 3 years ago
@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?
Super excited to start throwing errors instead of silently dropping terms... this is huge.
@dlongley @davidlehn I have updated the implementation as per your suggestion @dlongley, can you suggest some further test cases to verify this behaviour?
Outstanding questions
@vocab
usage?@type
and @id
propertyDo 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.
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?
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 @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
@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 -- 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 thanks I think I've addressed all of the points you raised in your last review, ready for another pass.
Hey @dlongley @davidlehn anything outstanding with this PR, keen to merge when possible so I can continue to work upstream?
@tplooker -- Just letting you know that I see this and that I'm going to get @davidlehn to have a look.
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.