Skip to content

Commit

Permalink
Add expansion test (re)defining a compact IRI using itself
Browse files Browse the repository at this point in the history 
@dlongley, this currently leads to an error in your implementation. I don't see compelling reason why we should disallow this.
  • Loading branch information
@lanthaler
lanthaler committed Apr 15, 2013
1 parent 44ac763 commit 657c90f
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 0 deletions.
9 changes: 9 additions & 0 deletions test-suite/tests/expand-0069-in.jsonld
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"@context": {
"rdfs": "http://www.w3.org/2000/01/rdf-schema#",
"rdfs:subClassOf": { "@id": "rdfs:subClassOf", "@type": "@id" }
},
"@id": "http://example.com/vocab#class",
"@type": "rdfs:Class",
"rdfs:subClassOf": "http://example.com/vocab#someOtherClass"
}
9 changes: 9 additions & 0 deletions test-suite/tests/expand-0069-out.jsonld
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[
{
"@id": "http://example.com/vocab#class",
"@type": [ "http://www.w3.org/2000/01/rdf-schema#Class" ],
"http://www.w3.org/2000/01/rdf-schema#subClassOf": [
{ "@id": "http://example.com/vocab#someOtherClass"}
]
}
]
6 changes: 6 additions & 0 deletions test-suite/tests/expand-manifest.jsonld
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,12 @@
"name": "prefix:suffix values are not interpreted as compact IRIs if prefix is an underscore",
"input": "expand-0068-in.jsonld",
"expect": "expand-0068-out.jsonld"
}, {
"@id": "#t0069",
"@type": ["jld:PositiveEvaluationTest", "jld:ExpandTest"],
"name": "Redefine compact IRI to define type mapping using the compact IRI itself as value of @id",
"input": "expand-0069-in.jsonld",
"expect": "expand-0069-out.jsonld"
}
]
}

12 comments on commit 657c90f

@dlongley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lanthaler, this has been fixed and pushed to the playground. I also noticed that the algorithm does not ensure that using a term definition like this:

{"foo": {"@id": "@context"}}

is prohibited. I noticed this because I simplified the term definition code by processing non-expanded term definitions (strings) just like expanded ones w/just @id set (similarly, your processor seems to allow keyword aliases via the @id key). The syntax spec currently disallows keywords in the @id position in expanded terms, though I don't know how much sense that makes given that you can use a term there that is just a keyword alias (and I think we should continue to permit that).

Anyway, I'd update the algorithm text for this right now but I'm not sure if that's kosher given the current state of the documents. In any case, the way I changed my code, that should be also reflected in the algorithm, is to just change non-expanded term definitions (strings) to expanded ones (by creating an expanded term definition w/@id set to the string value), then drop out and process them as usual (like objects), and finally, before adding the term definition at the end, ensure that the IRI mapping is not @context. This seems to be the shortest code and it takes all the term definitions through the same code path.

@lanthaler
Copy link
Member Author

@lanthaler lanthaler commented on 657c90f Apr 15, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlongley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean keyword-aliasing in general or aliasing @context?

I was talking about @context, specifically.

Omitting @id is, IMO, just syntactic sugar.

I agree.

The syntax spec currently disallows ...

Are you sure? I had a look and couldn't see where it disallows it.

Under B.7 it states:

If the expanded term definition contains the @id keyword, its value must be null, an absolute IRI, a blank node identifier, a compact IRI, or a term.

So, by omission, using a keyword there is not permitted. Like you said, I think it would just be a minor bug fix to correct.

OK, that raises error when an alias for @context is encountered. What about the cyclic IRI mapping error? How did you fix that one?

Oh, sorry, I forgot to mention that. Now, if an expanded term definition has an @id property, the IRI mapping is only expanded and added to the term definition if the value of the @id property does not equal the term. Then, the only other change is, instead of dropping down to an "else" after looking for @id, the constructed term definition is checked to see if it has an IRI mapping yet, and if not, then the prefix check and other code that previously ran in the else is run. Everything else remains the same. Does that make sense?

@lanthaler
Copy link
Member Author

@lanthaler lanthaler commented on 657c90f Apr 15, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlongley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that latest test case is incorrect. I've added another test case to show where it causes a conflict with @vocab.

@lanthaler
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. we should definitely talk about this as a group. I don't really like this asymmetry. If you modify your test to

{
  "@context": [
    {
      "v": "http://example.com/vocab#",
      "term": "v:somethingElse"
    },
    {
      "@vocab": "http://example.com/anotherVocab#",
      "otherTerm": "term"
    }
  ],
  "otherTerm": "value of term"
}

the problem becomes obvious IMO. otherTerm would expand to http://example.com/vocab#somethingElse.

@dlongley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem becomes obvious IMO. otherTerm would expand to http://example.com/vocab#somethingElse.

I don't think that's a problem, rather, since you're not redefining "term", it makes perfect sense to me. But, yes, I agree that we should talk about this as a group.

@lanthaler
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESOLVED: When re-defining a term 'A', any previous definition for term 'A' is removed before the right hand side for the new re-definition is evaluated.

@gkellogg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not require that the spec-text for Create Term Definition be updated? Right now, I fail expand-0069 on because rdfs:subClassOf is being re-defined during a definition. The check in the first step of Create Term Definition would seemingly explicitly dis-allow this.

The resolution relates to a new definition of a previously completely defined term, not a term which is being defined. The steps seem to be the following:

  • call Create Term Definition for rdfs:subClassOf
  • set defined['rdfs:subClassof'] to false
  • In step 12.2 call IRI Expansion algorithm
  • In step 2 of IRI Expansion, as we have a local context and value in defined is not true invoke Create Term Definition algorithm.
  • Back in step 1 of Create Term Definition, as the value is false, raise cyclic IRI mapping.

@lanthaler
Copy link
Member Author

@lanthaler lanthaler commented on 657c90f May 4, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlongley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My implementation now converts the short-hand "term": "iri" form to an expanded term definition:

https://github.com/digitalbazaar/jsonld.js/blob/master/js/jsonld.js#L4325

And then checks the @id value against the term:

https://github.com/digitalbazaar/jsonld.js/blob/master/js/jsonld.js#L4369

And then only does the prefix check if there's no @id mapping in the expanded term definition:

https://github.com/digitalbazaar/jsonld.js/blob/master/js/jsonld.js#L4376

@lanthaler
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've fixed the algorithm in 2a2477c. It's very similar to what Dave did. I made it clear that value is initialized to a copy of the definition in the local context - otherwise we couldn't convert it to an object as we never modify input values.

Please sign in to comment.