change 2 if statements

[PatStLouis]

This is to address #188

I believe there were 2 erroneous checks in the code.

The first one was checking the statement if not value. In python logic, an empty object would count as a non-value, therefore we need to specifically check for None. This should be raised only if the value is explicitly declared as null, not if its an empty object, as described in the json-ld-api expansion algorythm.

The second wrong check has to do with the value False being returned and the code checking if the value is not None. False won't fall into this category, therefore we should check if there is a value instead.

Are any test-suites available to validate these changes? It has resolved my issue and I was able to successfully expand the traceability context.

Special thanks to @dbluhm for helping to narrow down this issue.

@dlongley @msporny we would like to request a new release of the pyld package with these bug fixes since aca-py currently relies on it for credential verification.

[swcurran]

That was small! Now to get it merged and released so we can use it!

@dlongley — what do you think? We know it’s been awhile since this has been touched.

[msporny]

@PatStLouis @swcurran There is a JSON-LD test suite that pyld runs against, so we can use that for testing/regressions.

@davidlehn or @dlongley Might know what the current state of releases are (but both are swamped currently, so we might need someone else to take over maintenance of this package).

/cc @BigBlueHat so he knows about the release situation -- looks like we need to some some release management here. Looking at the PRs, we have quite the backlog.

[swcurran]

Thanks @msporny. Given that we are hitting a roadblock in integrating with the Traceability Test Suite, we’d really appreciate if this small change could be handled before getting into some of the larger questions like the backlog.

We do understand that getting the release pipeline going might be a bit tricky and necessary for to push a release out.

Any help we can get so we can complete our work would be appreciated! If there are things we can do — please let us know.

[BigBlueHat]

@PatStLouis so, the test suite failures and errors jumps significantly once this PR is in place, so those will need to be addressed before we can confirm this is working for all scenarios.

Checkout #182 to simplify test running a bit and moves to rdf-canon instead of normalization tests. Could you give those test results a look to see what else may need to be addressed?

[BigBlueHat]

Some more details. Many of the new test failures are failing with errors like...

Code: invalid local context Details: {'context': False}

or

Code: invalid scoped context Details: {'context': [None], 'term': 'Type'}

So...definitely related to this PR.

[PatStLouis]

@BigBlueHat thanks for reviewing this and thanks for pointing me towards the test documentation. I've run the tests and made some changes and I believe I have the same results as the original code. This issue has to do with how python treats empty dictionary as falsey when checking conditions, so {'context': False} and {'context': {}} would both match a if not context: statement. My original PR replaced this with only checking specially for None values based on the json-ld-api spec identifying null as the value which should trigger the error. However since the negative tests also test against the value False, I added a check for both None and False explicitly, preventing empty dictionaries from being picked up by those statements.

The traceability context currently has 80 empty context which was causing this behavior.

Please let me know if any further modifications are needed here!

[PatStLouis]

@BigBlueHat were you able to get a review on these changes?

[swcurran]

Good stuff! Now the next step — how do we get a new release created. @BigBlueHat — do you have The Power? Someone else listening in?

Thanks!

[davidlehn]

• Note that rdf-canon tests are not yet supported and all tests will be ignored, so test with the older test suite for now.
• Until pyld catches up with the newest test suite tests, I think the best we can do is make sure fixes don't cause any more failures than before. It seems that's the case here, so I think this is ok.
• @BigBlueHat I think this can be a patch release before we handle the other testing related PRs. (Which will disrupt that milestone you created.)
• This appears to be missing test coverage in the main test suite. I tried to extract a minimal test and added a PR to avoid regressions and test other implementations. Let us know if you find variations with a similar problem.
• https://github.com/w3c/json-ld-framing/pull/158

[BigBlueHat]

• @BigBlueHat I think this can be a patch release before we handle the other testing related PRs. (Which will disrupt that milestone you created.)

Agreed. I'll rework the milestone(s).

[PatStLouis]

@BigBlueHat do you have an ETA for this change to be released? Is end of week a reasonable expectation? Thank you

[BigBlueHat]

• @BigBlueHat I think this can be a patch release before we handle the other testing related PRs. (Which will disrupt that milestone you created.)

@davidlehn could I get your help cutting this release?

@PatStLouis can you confirm that the tests @davidlehn wrote in https://github.com/w3c/json-ld-framing/pull/158 are sufficient to test your use case (the one which uncovered this bug)?

I'm fine to ship the release with all the tests still passing at the rate they were and with @davidlehn's new one passing as well.

[PatStLouis]

@BigBlueHat these test cases do cover what triggered this bug and I confirm they are sufficient.

[PatStLouis]

@BigBlueHat @davidlehn any updates on a release ETA?

[davidlehn]

2.0.4 should be available now.

[swcurran]

Great stuff — thanks all!

[PatStLouis]

@davidlehn @BigBlueHat thanks a lot for working with us on this!