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

Framing and @container:@set for scoped contexts #64

Closed
azaroth42 opened this issue Aug 14, 2019 · 4 comments · Fixed by #65
Closed

Framing and @container:@set for scoped contexts #64

azaroth42 opened this issue Aug 14, 2019 · 4 comments · Fixed by #65

Comments

@azaroth42
Copy link
Contributor

Playground: http://tinyurl.com/y4ry8o3m

With a very simple identity frame (e.g. give the top node's id and a context), the expected output would be to follow the context's definition for @container: @set and wrap single objects in an array. This works as expected for the context when applied via compaction (see the compaction tab in the playground).

However, in both the javascript and python implementations, the part property does not generate an array from the frame, where it does from compaction. Note that it is not due to Production within Production, as changing the class of either to (e.g.) Activity results in the same structure.

This seems like a bug?

@gkellogg
Copy link
Member

gkellogg commented Aug 14, 2019

A simpler formation still shows the problem (playground).

{
  "@context": {
    "@version": 1.1,
    "@vocab": "http://example.org/vocab#"
  },
  "@id": "http://example.org/1",
  "@type": "HumanMadeObject",
  "produced_by": {
    "@type": "Production",
    "_label": "Top Production",
    "part": {
      "@type": "Production",
      "_label": "Test Part"
    }
  }
}

Frame:

{
  "@context": {
    "@version": 1.1,
    "@vocab": "http://example.org/vocab#",
    "Production": {
      "@context": {
        "part": {
          "@type": "@id",
          "@container": "@set"
        }
      }
    }
  },
  "@id": "http://example.org/1"
}

The issue seems to be in the last paragraph of 4.2.2:

Recursively, replace all entries in compacted results where the key is @preserve with the value of the entry. If the value of the entry is @null, replace the value with null. If, after replacement, an array contains a single array value, replace the array with that value. If, after replacement, an array contains only the value null remove the value, leaving an empty array.

The key bit recently added is the italicized bit, which does not consider the context.

Logically, it would seem to make sense to do this last bit before compaction, although I suspect there was a reason we didn't do this.

@azaroth42 azaroth42 added this to Discuss-Call in JSON-LD Management Aug 15, 2019
@gkellogg
Copy link
Member

So, the problem with cleaning up @preserve and @null before compaction, is that, in some cases, the null value should be kept when framing, but eliminated if it's just one component of an array. I've never understood why this was important, but perhaps @dlongley could shed some light on this.

The simplest thing would be do just do that cleanup before compaction, which would allow the regular compaction dynamics to work.

If keeping the null output is important, we could separate replacing @null with null until after compaction, although this would still likely leave odd results, with some values of null removed, and others remaining.

Preserving null must have had some value, otherwise, the whole use for @null is questionable.

Otherwise, we may need to compact, do @preserve/@null cleanup, then expand and re-compact to get this right, which seems silly and wastefull.

@gkellogg
Copy link
Member

With a trial implementation, 18 framing tests fail, where null is expected and an empty array is what is actually produced.

For example, in #t0005 we expect the following:

{
  "@context": {
    "dcterms": "http://purl.org/dc/terms/",
    "ex": "http://example.org/vocab#"
  },
  "@graph": [
    {
      "@id": "http://example.org/test/#library",
      "@type": "ex:Library",
      "ex:contains": {
        "@id": "http://example.org/test#book",
        "@type": "ex:Book",
        "dcterms:title": "My Book",
        "ex:contains": {
          "@id": "http://example.org/test#chapter",
          "@type": "ex:Chapter",
          "dcterms:title": "Chapter One",
          "ex:null": null
        }
      }
    }
  ]
}

But, what is actually produced is:

{
  "@context": {
    "dcterms": "http://purl.org/dc/terms/",
    "ex": "http://example.org/vocab#"
  },
  "@graph": [
    {
      "@id": "http://example.org/test/#library",
      "@type": "ex:Library",
      "ex:contains": {
        "@id": "http://example.org/test#book",
        "@type": "ex:Book",
        "dcterms:title": "My Book",
        "ex:contains": {
          "@id": "http://example.org/test#chapter",
          "@type": "ex:Chapter",
          "dcterms:title": "Chapter One",
          "ex:null": []
        }
      }
    }
  ]
}

@iherman
Copy link
Member

iherman commented Aug 16, 2019

This issue was discussed in a meeting.

  • RESOLVED: merge api#134 and syntax#212, and close syntax#204
View the transcript Benjamin Young: See Framing issue #64
Rob Sanderson: In framing context, I had a `@container:@set`. I did various things in playground, and discovered that in framing, container:set does not get handled properly. It does work fine in compaction.
… Also in Python implementation.
Gregg Kellogg: This is because there’s a step to handle @preserve values post-compaction.
… This is because there are cases where there are null values that are replaced during compaction.
… My suggested fix would emit empty array instead of null. But this does not preserve null in output anymore.
Dave Longley: JSON devs expect null there, so it must be preserved. They want to see the null there.
Gregg Kellogg: Except if null is part of an array.
Dave Longley: Indeed. That’s another case.
… I wonder if we can do something during compaction to indicate preserve:null.
… Can we leave a note during compaction for this?
Gregg Kellogg: One way would be to separate processing of preserve:null, and do preserve before compaction. Once it has been compacted, there may be values with null. If in array, would be removed, if not, it would be preserved.
… There’s no easy way to handle this. But handling null after compaction may be the best way.
Dave Longley: Not great, but may be a good solution.
Gregg Kellogg: Not needed to discuss this further during this call. There is a way to fix this. But this is implementation work.
4.2. Link header for HTML and JSON-LD #204
Benjamin Young: Link header: preemptive conneg.
Benjamin Young: See Syntax issue #204
Benjamin Young: See API PR #134
Benjamin Young: See Syntax PR #212
Benjamin Young: 2 PRs are ready to go, right?
Gregg Kellogg: Yes. Open question for base during redirect.
… If you retrieve something non-json. Doc loader will detect rel-alternative links, and redirect to that resource.
… I think the original requested document should be preserved as base URL.
… Just like 303 redirect semantics. In 302, this would not be the case.
Rob Sanderson: JS will silently follow redirects and give you the results. So you don’t know which redirects got you there.
Gregg Kellogg: JS should correctly implement redirect semantics.
Dave Longley: +1 to using 303 semantics, so on, +1 to gregg’s position.
Gregg Kellogg: You have processing state to keep track on this.
Proposed resolution: merge api#134 and syntax#212, and close syntax#204 (Ivan Herman)
Rob Sanderson: +1
Dave Longley: +1
Benjamin Young: +1
Rob Sanderson: :shipit:
Ivan Herman: +1
Gregg Kellogg: +1
Ruben Taelman: +0.5
Resolution #2: merge api#134 and syntax#212, and close syntax#204
4.

@gkellogg gkellogg moved this from Discuss-Call to Editorial Work in JSON-LD Management Aug 16, 2019
@gkellogg gkellogg self-assigned this Aug 17, 2019
@gkellogg gkellogg moved this from Editorial Work to Editorial work complete in JSON-LD Management Aug 17, 2019
@gkellogg gkellogg removed this from Editorial work complete in JSON-LD Management Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants