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

Fix nested list expansion in JSON-LD #2517

Merged
merged 14 commits into from
Aug 15, 2023
Merged

Conversation

avillar
Copy link
Contributor

@avillar avillar commented Aug 4, 2023

Summary of changes

This PR fixes parsing of deeply nested arrays in JSON-LD:

  • by default:
    • Before fix: [ [ [ 1, 2, 3] , [4, 5] ] ] --> ( "[1, 2, 3]", "[4, 5]" ) (list contents converted to string literals)
    • After fix: [ [ [ 1, 2, 3] , [4, 5] ] ] --> ( 1 2 3 4 5 ) (array is properly flattened)
  • when @container is set to @list:
    • Before fix: [ [ [ 1, 2, 3] , [4, 5] ] ] --> ( "[1, 2, 3]", "[4, 5]" ) (list contents converted to string literals)
    • After fix: [ [ [ 1, 2, 3] , [4, 5] ] ] --> ( ( ( 1 2 3 ) ( 4 5 ) ) ) (array nesting preserved)

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Checked that all tests and type checking passes.
  • If the change adds new features or changes the RDFLib public API:
    • Created an issue to discuss the change and get in-principle agreement.
    • Considered adding an example in ./examples.
  • If the change has a potential impact on users of this project:
    • Added or updated tests that fail without the change.
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@aucampia
Copy link
Member

aucampia commented Aug 4, 2023

pre-commit.ci autofix

@aucampia
Copy link
Member

aucampia commented Aug 7, 2023

I'm not really that familiar with JSON-LD as I probably should be, as far as I understand the flattening is pursuant to JSON-LD 1.1: 9.14 Property Nesting

@avillar
Copy link
Contributor Author

avillar commented Aug 8, 2023

@aucampia, yes, RDFLib does first-level and second-level flattening correctly, but fails when arrays are nested more deeply. For this JSON-LD playground example, this is what RDFLib outputs (note the incorrect "[4]"):

@prefix ns1: <http://example.com/> .
@prefix xsd: <http://www.w3.org/2001/XMLSchema#> .

[] ns1:a 1,
        2,
        3,
        5,
        "[4]" .

And, if @container is set to @list, flattening should not be happening at all. Again, JSON-LD Playground vs RDFLib output:

@prefix ns1: <http://example.com/> .
@prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> .
@prefix xsd: <http://www.w3.org/2001/XMLSchema#> .

[] ns1:a ( "[1, 2, 3]" "[[4]]" 5 ) .

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

@avillar thanks for fixing this, and apologies for only reviewing it now. There are some minor comments that I will address with some commits shortly, but the fix looks very good overall.

test/jsonld/test_nested_arrays.py Outdated Show resolved Hide resolved
rdflib/plugins/parsers/jsonld.py Outdated Show resolved Hide resolved
rdflib/plugins/parsers/jsonld.py Outdated Show resolved Hide resolved
test/jsonld/test_nested_arrays.py Show resolved Hide resolved
rdflib/plugins/parsers/jsonld.py Outdated Show resolved Hide resolved
test/jsonld/test_nested_arrays.py Outdated Show resolved Hide resolved
rdflib/plugins/parsers/jsonld.py Outdated Show resolved Hide resolved
@aucampia aucampia added review wanted This indicates that the PR is ready for review ready to merge The PR will be merged soon if no further feedback is provided. labels Aug 10, 2023
@aucampia aucampia requested a review from a team August 10, 2023 20:13
@aucampia
Copy link
Member

I will merge this before 2023-08-14.

@avillar
Copy link
Contributor Author

avillar commented Aug 10, 2023

Thank you, and thanks for all the fixes!

@coveralls
Copy link

Coverage Status

coverage: 90.934% (+0.002%) from 90.932% when pulling 3041924 on avillar:fix-nested-arrays into f45bd9b on RDFLib:main.

@aucampia aucampia merged commit e8e549c into RDFLib:main Aug 15, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants