Skip to content

Context overflow. #416

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

Merged
merged 6 commits into from
Mar 17, 2020
Merged

Context overflow. #416

merged 6 commits into from
Mar 17, 2020

Conversation

davidlehn
Copy link
Contributor

@davidlehn davidlehn commented Mar 13, 2020

  • Work in progress.
  • I think e003 was supposed to have a level of indirection? Seemed to be like e002. I changed it to e003-in -> e003-context -> e003-in.
  • jsonld.js doesn't test the e002 and e003 1.0 tests, so unsure they are correct.
  • Should implementations try to handle the 1.0 tests? In particular, handle the recursive error code name change.
  • Should the "context overflow" term description be marked as "changed"? I'm not sure when that happened.
  • Some error tests added for "context overflow" code. It's hard to really test true overflow since implementations have their own limits, and lots of context files seems silly. In jsonld.js it's just detecting there will be recursion and failing with the overflow code. I assume implementations don't have to actually run till their limit. Testing the real overflow code seems either better done per-implementation, or we could add a test flag to only allow, say, 1 level and check for overflow. That requires implementations to have a custom max ability.
  • toRdf clones of tests are still needed if these tests are ok.
  • There are likely more strange recursion patterns that could be tested. Any ideas?

Preview | Diff

@gkellogg
Copy link
Member

Tests marked with specVersion: json-ld-1.0 are typically incompatible with the 1.1 spec, and there are 1.1 versions where appropriate.

@gkellogg
Copy link
Member

  • Work in progress.
  • I think e003 was supposed to have a level of indirection? Seemed to be like e002. I changed it to e003-in -> e003-context -> e003-in.

👍

  • jsonld.js doesn't test the e002 and e003 1.0 tests, so unsure they are correct.

They are included for historical purposes, so that pure 1.0 processors still have tests available that were used for conformance reporting. 1.1 implementations are expected to not execute them.

  • Should implementations try to handle the 1.0 tests? In particular, handle the recursive error code name change.

No.

  • Should the "context overflow" term description be marked as "changed"? I'm not sure when that happened.

It changed because the semantics changed. Before, it was an array of remote contexts, which didn't account for legitimate repetition; thus, it changed to "context overflow" to allow for legitimate repetition.

Really, it comes in with scoped contexts, where terms may be in different included contexts, which could legitimately reference each other. (similar issue for improved

  • Some error tests added for "context overflow" code. It's hard to really test true overflow since implementations have their own limits, and lots of context files seems silly. In jsonld.js it's just detecting there will be recursion and failing with the overflow code. I assume implementations don't have to actually run till their limit. Testing the real overflow code seems either better done per-implementation, or we could add a test flag to only allow, say, 1 level and check for overflow. That requires implementations to have a custom max ability.

We could put something about this in the tests/README, or index. Probably want to set some reasonably small limit when testing, but it should work with a larger limit, too.

  • toRdf clones of tests are still needed if these tests are ok.
  • There are likely more strange recursion patterns that could be tested. Any ideas?

These look good, and I currently fail 0124-0126, so I'll need to look at them more closely.

I can push up toRdf versions.

@gkellogg gkellogg force-pushed the context-overflow-fixes branch from f6f22e4 to ed754df Compare March 15, 2020 20:44
@gkellogg
Copy link
Member

gkellogg commented Mar 15, 2020

Actually, algorithm text needs to be modified to allow checking for such recursive scoped context. The logic for imported contexts is different, and doesn't allow recursion at the time of processing (testing is done after merge).

  • Both Context Processing and Create Term Definition algorithms require a new validate scoped context option defaulting to true. This is passed to Create Term Definition, and set to false when validating a scoped context.
  • Add a new step before 5.2.5 in Context Processing to skip processing loaded context if validate scoped context is false. This will continue to validate scoped contexts which are local, but defer the processing for remote contexts until it is actually used in expansion or compaction algorithms.

Note that these steps don't change the behavior of any existing tests, and stick to the intent of validating scoped contexts without infinite recursion introduced due to the validation, while continuing to perform the validation later on, implicitly.

@gkellogg
Copy link
Member

@davidlehn This will have some implications in the pre-loading of contexts in jsonld.js and pyld, but really just to defer the preload if processing a scoped context, so may require the addition of some state.

@azaroth42 I believe we can consider this editorial, as it doesn't impact any existing tests and allows the usage patterns discussed in the syntax document.

Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

With the changes I added.

@pchampin
Copy link
Contributor

I am not sure I fully grasp its impact. I see how it prevents an overflow in the three added tests. But it seems to me that it also does not completely validate a scoped context that would contain a URL. For example in

{
  "@context": {
    "@version": 1.1,
    "prop": {
      "@id": "ex:prop",
      "@context": "0125-context-2.jsonld"
    },
    "value": {
      "@id": "ex:value"
    }
  }
}

the context at 0125-context-2.jsonld is not even passed to Context Processing, so if it was invalid, this would not longer be detected at this point. Correct?

@gkellogg
Copy link
Member

Yes, it ends up postponing the validation until the context is used, which is somewhat contrary to the statement that the scoped context must be valid, but I see no other way to halt recursion.

In your case, if "prop" were never used, then we could not detect that "0125-context-2.jsonld" was valid (although it could be eventually valid :) ).

The intention is to allow chains of contexts which self reference, but validation needs to stop somewhere. Perhaps an alternative would be instead to halt dereferencing only if the referenced context is already in remote contexts.

@davidlehn
Copy link
Contributor Author

  • Should the "context overflow" term description be marked as "changed"? I'm not sure when that happened.

[...]
Really, it comes in with scoped contexts, where terms may be in different included contexts, which could legitimately reference each other. (similar issue for improved

(Looks like a thought was cut off)

By "changed", I mean in the JsonLdErrorCode section (and maybe others), the "context overflow" line doesn't have class="changed". Perhaps it should.

  • Some error tests added for "context overflow" code. It's hard to really test true overflow since implementations have their own limits, and lots of context files seems silly. In jsonld.js it's just detecting there will be recursion and failing with the overflow code. I assume implementations don't have to actually run till their limit. Testing the real overflow code seems either better done per-implementation, or we could add a test flag to only allow, say, 1 level and check for overflow. That requires implementations to have a custom max ability.

We could put something about this in the tests/README, or index. Probably want to set some reasonably small limit when testing, but it should work with a larger limit, too.

I think we might want to add test support for this as I found a jsonld.js related bug. It was only checking for depth overflow, not total context overflow. Assuming an implementation has a knob to adjust limits, it wouldn't be hard to test depth and total limits.

@davidlehn
Copy link
Contributor Author

@gkellogg Looks like 70c82dc changed file names, but didn't update the contexts loaded from those files.

@gkellogg
Copy link
Member

I am not sure I fully grasp its impact. I see how it prevents an overflow in the three added tests. But it seems to me that it also does not completely validate a scoped context that would contain a URL.

I updated the algorithm to process remote scoped contexts up to the point that they have already been processed. This should allow them to be checked when the context is evaluated, rather than when the property including the scoped context is actually expanded/compacted.

@gkellogg
Copy link
Member

@gkellogg Looks like 70c82dc changed file names, but didn't update the contexts loaded from those files.

Fixed.

davidlehn and others added 6 commits March 17, 2020 11:50
- 1.0 expand:
  - e003 looked like e002.  Made it use a level of indirection.
- 1.1 expand:
  - Copy e002 and e003 to e052 and e053 and use newer "context
    overflow" code.
  - Add e054 cross linked sibling contexts test.
  - Add 0124, 0125, 0126 scoped context recursion tests.
…Context Processing algorithms used to limit recursive context processing when validating scoped contexts.
…the context is in the _remote contexts_ array, which allows for validating scoped contexts when the term is defined up to the point that it has already been processed.
@gkellogg gkellogg force-pushed the context-overflow-fixes branch from be4b461 to 239c3eb Compare March 17, 2020 19:23
@gkellogg gkellogg mentioned this pull request Mar 17, 2020
@gkellogg gkellogg merged commit f829c79 into master Mar 17, 2020
@gkellogg gkellogg deleted the context-overflow-fixes branch March 17, 2020 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants