digitalbazaar / jsonld.js

A JSON-LD Processor and API implementation in JavaScript
https://json-ld.org/
Other
1.65k stars 195 forks source link

Update expandMap and tests. #464

Closed davidlehn closed 1 year ago

davidlehn commented 2 years ago

This is a continuation of the expansionMap work from: https://github.com/digitalbazaar/jsonld.js/pull/452

@tplooker This likely needs your input.

That expansionMap work was merged but never released. I wanted to get it out there and active for use in VCs and elsewhere to at least try and check for dropped types and such. It turns out I don't know how to use this feature! expansionMap is now called lots more than before. It has many variations, and the lack of documentation is now rather painful, since it's not obvious what the expansionMap code should do for what situations.

In the case of VCs, it seems like type scoped properties like credentialSubject end up with a couple calls, but maybe it shouldn't? Perhaps the callback is getting called while the algorithm does normal processing rather than just when there's an issue?

In any case, I updated the tests to count all the calls to expansionMap, and (maybe) what they were for. Just so it's clear in examples what is happening.

I do think some more test cases are needed, if for nothing else than as an example. Have various inputs that have unknown properties, types, etc, and an example expansionMap handler that knows how to reject all that. Basically a common use case for strict signing software.

codecov-commenter commented 2 years ago

Codecov Report

Merging #464 (0967732) into main (b7bc6d6) will decrease coverage by 67.43%. The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #464       +/-   ##
===========================================
- Coverage   92.67%   25.24%   -67.44%     
===========================================
  Files          23       23               
  Lines        2880     2880               
===========================================
- Hits         2669      727     -1942     
- Misses        211     2153     +1942     
Impacted Files Coverage Δ
lib/frame.js 3.89% <0.00%> (-92.54%) :arrow_down:
lib/fromRdf.js 7.25% <0.00%> (-91.13%) :arrow_down:
lib/compact.js 2.63% <0.00%> (-90.91%) :arrow_down:
lib/nodeMap.js 8.33% <0.00%> (-90.16%) :arrow_down:
lib/toRdf.js 10.28% <0.00%> (-88.79%) :arrow_down:
lib/JsonLdError.js 20.00% <0.00%> (-80.00%) :arrow_down:
lib/documentLoaders/node.js 21.87% <0.00%> (-73.44%) :arrow_down:
lib/flatten.js 38.46% <0.00%> (-61.54%) :arrow_down:
lib/expand.js 35.40% <0.00%> (-60.28%) :arrow_down:
lib/jsonld.js 25.88% <0.00%> (-56.96%) :arrow_down:
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b7bc6d6...0967732. Read the comment docs.

tplooker commented 2 years ago

Hey @davidlehn, sorry totally missed your ping here, this looks like a great improvement over measuring the behaviour of the function in the tests. From memory I do remember noting that the function was called multiple times during expansion, I also remember tracing the various cases and them making sense, however unfortunately I did not write it down.

davidlehn commented 2 years ago

@tplooker No problem. I'm dug into the issue now, so will come up with something to address what this was aiming at. I'm not quite sure what to do with the code as is. It seems awkward to write an expansionMap. And it's named poorly if it's being used to just handle malformed input. But at least the call sites are there now! It's likely some other code will be added in the same spots to handle the common error handler case.

I think I found part of the duplicated call issue. _expandIri is called multiple times needlessly at one point. I'll address that soon.

If anyone knows a use case for expansionMap or compactionMap that involves... mapping, let me know. :-) All I know to use these hooks for is for warning and error handling.

davidlehn commented 1 year ago

This changed and grew quite a bit in another branch. Merging back to main through this PR.