Skip to content
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

Circular imports: Abort, Retry, Fail? #14

Closed
gkellogg opened this issue Aug 1, 2018 · 8 comments
Closed

Circular imports: Abort, Retry, Fail? #14

gkellogg opened this issue Aug 1, 2018 · 8 comments

Comments

@gkellogg
Copy link
Member

gkellogg commented Aug 1, 2018

From json-ld/json-ld.org#645 originally reported by @azaroth42

In trying to build several inter-related contexts, we ran into the circular import issue. The current specification says that circular imports are an error condition.

Terms MUST NOT be used in a circular manner. That is, the definition of a term cannot depend on the definition of another term if that other term also depends on the first term.

The trouble arises when two contexts need to reference each other, potentially only in particular scopes, and when it's unknown which is the context that's used at the top of the tree.

For example, imagine two contexts. One defines the domain of Activities that are performed on resources. The other defines classes and properties of some subset of those possible resources, and wants to refer to the activities that are performed. From one perspective the Activity context is primary, and it merely talks about the things being acted upon. From the other perspective, the Object context is primary, and it references things that have happened to the resource of interest.

This example (and the equivalent with scoped contexts) is not valid:

{
  "@context": "activities-context",
  "type": "Processing",
  "target": {
    "@context": objects-context",
    "type": "Thing",
    "generated_by": {
      "@context": "activities-context",
      "type": "Generation",
      "actor": "person"
    }
  }
}

However if there are conflicts between the two (one defines label as a string, the other as a language map, for example), the simple resolution of pulling them both out and putting them at the top no longer works. If the contexts are maintained by different organizations and are updated at different schedules (schema.org and LDP for example) then managing exceptions is particularly hard. We ran into this issue here: IIIF/api#1571

The above JSON (and even nicer with scoped contexts) seems like a reasonable thing to do. Advice (and subsequent recording of best practices) on how to resolve these sorts of issues would be appreciated!

@iherman
Copy link
Member

iherman commented Oct 12, 2018

My instinctive reaction is to avoid adding complications to the language. I view it as one of those cases where such micro level issue can be solved via a complicated piece of specification, accompanied with documented best practices, but many of these micro level decisions lead to the macro level image of JSON-LD as being extremely complicated. And this is, at the end of the day, detrimental.

My option is therefore to Fail (or Abort, I am not sure what the difference between these two is).

@ajs6f
Copy link
Member

ajs6f commented Oct 12, 2018

Here's a high-level question that may say more about my lack of understanding of our process than anything else (:flushed:):

Does our understanding of the backward compatibility we are required to maintain permit that constructions which would error out in 1.0 could, in 1.1, successfully complete processing? I can see why it would, but I can also imagine people being surprised by systems that suddenly start producing results in situations in which they previously did not.

@gkellogg
Copy link
Member Author

Does our understanding of the backward compatibility we are required to maintain permit that constructions which would error out in 1.0 could, in 1.1, successfully complete processing?

I would say no, that we can do things that were illegal in 1.0, and in fact, we do. The value of @container can now be an array, for example.

@azaroth42
Copy link
Contributor

WG call - @gkellogg and @azaroth42 to come up with test cases and proposal. Discuss F2F at TPAC if time allows.

@gkellogg
Copy link
Member Author

@azaroth42 Looking at the example again, it should not be an error. The remote contexts array is set to an empty array if not passed in as an argument when processing a context. Any remote context is added to it as it runs, recursively. Between invocations of the context processing algorithm, remote contexts aren't remembered.

Can you provide an example where you're seeing a problem that will evaluate in the playground?

Note that this should be true for scoped contexts as well, as the scoped contexts aren't actually evaluated until their used in expansion.

@azaroth42
Copy link
Contributor

azaroth42 commented Oct 24, 2018

Attached zip has two example instances with the same structure, but the scoped context version (manifest-1.json) fails due to recursion and the inline @context version (manifest-2.json) works as intended.

  • manifest-1.json -- uses scoped contexts (in presentation.json, image.json)
  • manifest-2.json -- uses 1.0 inline context references (presentation-b.json, image-b.json)
  • presentation[-b].json, image[-b].json, anno.jsonld -- the context files
  • process.py -- python document loader and trivial expand() call to demonstrate the problem, at least in current PyLD.

recursive.zip

Will make dereferencable versions to also demonstrate in playground.

Edit:

Created a branch with the documents, with updated references to work online in the playground: http://tinyurl.com/yayh6e74 which demonstrates the cyclical error

@iherman
Copy link
Member

iherman commented Feb 9, 2019

This issue was discussed in a meeting.

  • RESOLVED: Update the algorithm to describe desired behavior of disallowing unbounded loading of contexts, still allow ping-ponging between two contexts in the document (or scoped contexts). {: #resolution8 .resolution}
View the transcript circular context references
Adam Soroka: #14
Rob Sanderson: but object could be also be the top level node
Gregg Kellogg: the reason we keep a list of imported context is to prevent ourselves from running away but someone figured out an attack…
… that created contexts that imported themselves, so we established a max depth.
David I. Lehn: error in the spec or error in an implementation?
Rob Sanderson: the verifiable claim that talks about a verifiable claim has the circular issue as well.
Gregg Kellogg: issue is a document that includes a document that is an array that includes itself.
Ivan Herman: if you put an array of context that is activity context, activity. What is left in the end is only things that overlap, so in this case the
… activity context rules.
Gregg Kellogg: propose that instead of seeing that something is included in the array, check a max size. Essentially elimate 3.2.2
Gregg Kellogg: limit mandated or set by application.
Ivan Herman: example in issue isn’t recursion …
Gregg Kellogg: If we keep the inclusion array, but once we’ve stopped we get rid of it .. but it doesn’t solve the attack problem. If we add in …
… a limit on the stack size
Gregg Kellogg: Move away from how the issue is handled and specify behavior instead.
Gregg Kellogg: we should have tests for both situations
Proposed resolution: Update the algorithm to describe desired behavior of disallowing unbounded loading of contexts, still allow ping-ponging between two contexts in the document (or scoped contexts). (Rob Sanderson)
Ivan Herman: +1
Jeff Mixter: +1
Rob Sanderson: +1
Simon Steyskal: +1
Harold Solbrig: +1
David I. Lehn: +1
Gregg Kellogg: +1
Resolution #7: Update the algorithm to describe desired behavior of disallowing unbounded loading of contexts, still allow ping-ponging between two contexts in the document (or scoped contexts). {: #resolution8 .resolution}
Adam Soroka: +1

@gkellogg gkellogg moved this from Discuss-F2F to Editorial Work in JSON-LD Management DEPRECATED Feb 10, 2019
gkellogg added a commit that referenced this issue Feb 16, 2019
…overflow` error. Obsoletes two expansion tests.

For #14.
@gkellogg gkellogg moved this from Editorial Work to Editorial work complete in JSON-LD Management DEPRECATED Feb 16, 2019
gkellogg added a commit that referenced this issue Feb 16, 2019
…overflow` error. Obsoletes two expansion tests.

For #14.
@azaroth42
Copy link
Contributor

Fixed with the stack depth approach. Resolved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants