Possible bug in Expansion Algorithm step 13.4.16

[kasei]

Expansion Algorithm step 13.4.16 says:

Unless expanded value is null, and expanded property is @value, and input type is @json, set the expanded property entry of result to expanded value.

I believe test t0122 highlights a case where this is problematic. It's possible by way of 13.4.3.2 and (IRI Expansion step 2) that expanded value is null here, but the test expects the resulting value to be an empty string. I'm not sure where else the null value might be converted to an empty string. Is it possible the test is wrong? Or does the wording in 13.4.16 need updating?

[gkellogg]

You might be referring to compact#tjs11, which doesn't have a related expansion test. t0122 is about catching IRIs having the form of a keyword. I'll add a couple of tests for this.

And, that test should read, instead:

Unless expanded value is null, and expanded property is @value, and input type is not @json, set the expanded property entry of result to expanded value.

But, this step overlaps a change in PR #275.

[kasei]

I'm not doing anything related to compaction, so I doubt there's any confusion with tjs11.

[gkellogg]

Can you describe what it is about t0122 that relates to this processing step?

[gkellogg]

To be clear, t0122 contains the following input file:

{
  "@context": {
    "@base": "http://example.org/"
  },
  "http://example.org/vocab/at": {"@id": "@"},
  "http://example.org/vocab/foo.bar": {"@id": "@foo.bar"},
  "http://example.org/vocab/ignoreme": {"@id": "@ignoreMe"}
}

which doesn't seem to relate to JSON literals. One test I was going to add uses the following input file:

{
  "@context": {
    "@version": 1.1,
    "e": {"@id": "http://example.org/vocab#null", "@type": "@json"}
  },
  "e": null
}

I think this would trigger the issue you were concerned about.

[kasei]

@gkellogg

which doesn't seem to relate to JSON literals

Is the "unless" that begins 13.4.16 meant to apply only to the first clause about expanded value and not to the entire 3-clause conjunction?

[kasei]

My concern here is is about @ignoreMe turning into the empty string in 0122-out.jsonld. My reading of 13.4.16 is that it isn't about JSON literals, as I parsed it as something like:

"if not((expanded value is null) && (expanded property is @value) && (input type is @json))

Given that reading/parsing, I can't/couldn't figure out how to end up with an empty string as: 13.4.3.2 will set expanded value to null; then 13.4.16 is triggered (as a result of expanded property not being @value; it is @id) and result[@id] is set to null.

[gkellogg]

Note that 13.4.16 is updated by PR #275 to be "Unless expanded value is null, and expanded property is @value, and input type is not @json, set the expanded property entry of result to expanded value."

This would be true for any non-null values of expanded value, any null values where property is not @value, or any null values where property is @value and input type is @json. So, it's about JSON literals when @value is null and @type is @json.

[gkellogg]

Relating to your original question (which I now finally understand), yes it does return null. It seems a bug in my code, from which the result is taken, turned it into an empty string. The test result should be updated to be the following:

[{
  "http://example.org/vocab/at": [{"@id": "http://example.org/@"}],
  "http://example.org/vocab/foo.bar": [{"@id": "http://example.org/@foo.bar"}],
  "http://example.org/vocab/ignoreme": [{"@id": null}]
}]

[gkellogg]

@kasei I believe the corrected t0122 is correct, and I added two other tests for null JSON literals.

Because we're changing expected results, the WG will need to consider this.

[kasei]

@gkellogg updated tests looks good, thanks.

[iherman]

This issue was discussed in a meeting.

• RESOLVED: Accept #270 including both text change to the algorithm and fix to the expected test result, noting that this would potentially affect other implementations’ test results, but that we do not believe this requires re-entry to CR

View the transcript

Possible bug in Expansion Algorithm step 13.4.16
Rob Sanderson: link: #270
Rob Sanderson: so the next one is issue 270
… after some discussion gkellogg believes the test is fixed
… and added two more tests
… but since we’re changing the tests, we do need to consider this as a group
… any change to the algorithm that doesn’t change the test results is fine
… but this one changes the test results…so we need to determine the weight of this one
… it doesn’t seem like a dropping us out of CR sort of change
… gkellogg can you elaborate?
Gregg Kellogg: there was a bug in my implementation based on my misinterpretation of my own text
… I’ll have to dig into it again more closely
… I’d misinterpreted what his concern was initially
Rob Sanderson: what I recall is that it was a null vs. empty list question
… and that the test results needed to test for whichever one it should have been rather than the other one
Gregg Kellogg: so the text had previously said if an input type was @json
… because null would be kept only in the case for @value when the type is @json
… that was a change to the algorithm text
… in terms of what the test result was…
… one of these was redundant
… expand 122 out. it had been an empty string…and should have stayed null
… so it partly related to that wrong step that shouldn’t have been in there
Rob Sanderson: so the good thing is that Greg agrees with these fixes
… so I think that we should have a resolution
… that there was essentially a typo in the test result
… and that we’re fixing it by tweaking both the algorithm and the test result
… and the issue submitter (Greg) agrees with this fix
… and that we agree this shouldn’t trigger a new CR
… my proposal, then, would be that we suggest a non-normative tweak to the algorithm text and fix what amounts to a typo in the tests
Ivan Herman: so, since as you say the algorithm is non-normative, then we should be fine
… certainly the algorithm and tests should be in sync
Rob Sanderson: pchampin you’re ok with this also?
Pierre-Antoine Champin: yes. it’s fine.
Rob Sanderson: so we should note that this would effect other implementation’s test results potentially
Proposed resolution: Accept #270 including both text change to the algorithm and fix to the expected test result, noting that this would potentially affect other implementations’ test results, but that we do not believe this requires re-entry to CR (Rob Sanderson)
Rob Sanderson: but that we do not believe this requires re-entry into CR
Gregg Kellogg: +1
Benjamin Young: +1
Jeff Mixter: +1
Tim Cole: +1
Pierre-Antoine Champin: +1
Rob Sanderson: +1
Resolution #3: Accept #270 including both text change to the algorithm and fix to the expected test result, noting that this would potentially affect other implementations’ test results, but that we do not believe this requires re-entry to CR
Ivan Herman: +1