Skip to content

fix: SPARQL LOAD ... INTO GRAPH handling #2554

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 30, 2023
Merged

fix: SPARQL LOAD ... INTO GRAPH handling #2554

merged 3 commits into from
Aug 30, 2023

Conversation

@aucampia
Copy link
Member

@aucampia aucampia commented Aug 28, 2023

Summary of changes

LOAD ... INTO GRAPH stopped working correctly after the change to handling of the publicID Graph.parse parameter in RDFLib 7.0.0 (#2406).

This is because LOAD evaluation relied on publicID to select the graph name. So after #2406 data would be loaded into the default graph even if a named graph is specified.

This change adds tests for LOAD ... INTO GRAPH and fixes the load evaluation.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Checked that all tests and type checking passes.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@aucampia aucampia force-pushed the aucampia/20230828T2212-fix-insert-into-named branch from 797e7bb to 0444fd7 Compare August 28, 2023 20:26
@aucampia
Copy link
Member Author

aucampia commented Aug 28, 2023

@rdfguy I think this should fix your problem https://twitter.com/oralassila/status/1695913300423225687

@aucampia aucampia requested a review from a team August 28, 2023 20:29
@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 28, 2023
@aucampia
Copy link
Member Author

aucampia commented Aug 28, 2023

A consequence of this change is also that relative IRI lookup for graphs
loaded with LOAD ... INTO GRAPH is now relative to the source document
URI instead of the base URI of the graph being loaded into, which is
more correct I think.

I will add a test for this also tomorrow to confirm.

@aucampia
Copy link
Member Author

aucampia commented Aug 28, 2023

Also seems like windows does not like file URIs, I will look into that tomorrow also.

@aucampia aucampia force-pushed the aucampia/20230828T2212-fix-insert-into-named branch from 0444fd7 to 6d14617 Compare August 28, 2023 20:50
@aucampia
Copy link
Member Author

aucampia commented Aug 28, 2023

Also seems like windows does not like file URIs, I will look into that tomorrow also.

Okay nevermind, tests pass on windows now, I just forgot to use URIs.

@coveralls
Copy link

coveralls commented Aug 28, 2023 •
edited
Loading

Coverage Status

coverage: 90.935% (+0.01%) from 90.925% when pulling 2ce064b on aucampia:aucampia/20230828T2212-fix-insert-into-named into dfe0c21 on RDFLib:main.

@rdfguy
Copy link

rdfguy commented Aug 28, 2023

Reading the commits the changes make sense, and in line with what I observed yesterday when my LOADs ended up in wrong named graphs. Haven't tried the code yet, though.

aucampia
aucampia commented Aug 28, 2023
View reviewed changes
@aucampia
fix: SPARQL LOAD ... INTO GRAPH handling
bb2ff6b 
`LOAD ... INTO GRAPH` stopped working correctly after the change to
handling of the `publicID` `Graph.parse` parameter in RDFLib 7.0.0
(<RDFLib#2406>).

This is because `LOAD` evaluation relied on `publicID` to select the
graph name. So after <RDFLib#2406> data
would be loaded into the default graph even if a named graph is
specified.

This change adds tests for `LOAD ... INTO GRAPH` and fixes the load
evaluation.

A consequence of this change is also that relative IRI lookup for graphs
loaded with `LOAD ... INTO GRAPH` is now relative to the source document
URI instead of the base URI of the graph being loaded into, which is
more correct.
@aucampia aucampia force-pushed the aucampia/20230828T2212-fix-insert-into-named branch from 6d14617 to bb2ff6b Compare August 29, 2023 04:20
@aucampia
Add tests for relative URI handling
57399a4 
The PR has changes to the publicID that is used, which affects relative
URI resolution, so that should also be tested.
@aucampia
fix docstring
2ce064b
@aucampia
Copy link
Member Author

aucampia commented Aug 29, 2023

A consequence of this change is also that relative IRI lookup for graphs loaded with LOAD ... INTO GRAPH is now relative to the source document URI instead of the base URI of the graph being loaded into, which is more correct I think.

I will add a test for this also tomorrow to confirm.

Done now.

@aucampia
Copy link
Member Author

aucampia commented Aug 29, 2023

I will merge this tomorrow if there is no futher feedback.

@aucampia aucampia merged commit 09354a5 into RDFLib:main Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aucampia @coveralls @rdfguy