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

Allow URLInputSource to get content-negotiation links from the Link headers #1436

Merged
merged 22 commits into from
Mar 17, 2022

Conversation

ashleysommer
Copy link
Contributor

  1. Allow URLInputSource to get content-negotiation links from the Link headers of HTTP responses.
  2. Use those Links to resolve schema.org-style json-ld conneg redirections.
  3. Fix the ability to run the remote-url arm of the JSON-LD test suite (got most of them working!)
    Fixes Failed to parse graph (json-ld, JSONDecodeError)  #1423

…eaders of HTTP responses. Use Links to resolve schema.org-style json-ld conneg redirections.

Fix the ability to run the `remote-url` arm of the JSON-LD test suite (got most of them working!)
rdflib/parser.py Outdated Show resolved Hide resolved
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

A few OPTIONAL ideas...

rdflib/parser.py Outdated Show resolved Hide resolved
rdflib/parser.py Outdated Show resolved Hide resolved
test/jsonld/runner.py Outdated Show resolved Hide resolved
test/jsonld/test_onedotone.py Outdated Show resolved Hide resolved
test/jsonld/test_onedotone.py Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

@ashleysommer @nicholascar may be worth merging this, I'm hitting at least one of the problems this fixes in the move to pytest in #1452 - see https://drone.rdflib.ashs.dev/RDFLib/rdflib/520/1/2

2717	self = <rdflib.plugins.shared.jsonld.context.Context object at 0x7f9651f87690>
226s
2718	source = {'@import': 'so08-context.jsonld', '@protected': True, '@version': 1.1, 'term': 'http://example.org/redefined'}
226s
2719	source_url = None, referenced_contexts = set()
226s
2720	
226s
2721	    def _read_source(self, source, source_url=None, referenced_contexts=None):
226s
2722	        imports = source.get(IMPORT)
226s
2723	        if imports:
226s
2724	            if not isinstance(imports, str):
226s
2725	                raise INVALID_CONTEXT_ENTRY
226s
2726	    
226s
2727	            imported = self._fetch_context(
226s
2728	                imports, self.base, referenced_contexts or set()
226s
2729	            )
226s
2730	            if not isinstance(imported, dict):
226s
2731	>               raise INVALID_CONTEXT_ENTRY
226s
2732	E               NameError: name 'INVALID_CONTEXT_ENTRY' is not defined

@aucampia
Copy link
Member

aucampia commented Oct 19, 2021

not sure why drone is failing here though, also there may be other problems causing failures on my branch - but this PR looks good to me at least.

Update 1

Actually the issue I had did not really have anything to do with what is fixed here, and tests there pass fine now. Still may be good to merge this before we merge the migration to pytest.

nicholascar and others added 6 commits November 21, 2021 21:24
Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
@nicholascar
Copy link
Member

Still may be good to merge this before we merge the migration to pytest.

Ooops... As noted in another PR, we are going to have to update PRs with old-style tests in them to pytest before the next release.

@nicholascar nicholascar self-requested a review November 21, 2021 11:39
@aucampia
Copy link
Member

aucampia commented Nov 24, 2021

@nicholascar will try make a PR to fix this the coming weekend.

Also ping me if there are more PRs that need updating.

@aucampia
Copy link
Member

Just to clarify regarding this:

Still may be good to merge this before we merge the migration to pytest.

Ooops... As noted in another PR, we are going to have to update PRs with old-style tests in them to pytest before the next release.

This will be a problem because it does yield tests which are not picked up by pytest but is a nosetest thing, and this is what I will fix. python unittest.UnitTest based tests integrate fine with pytest.

@nicholascar
Copy link
Member

@aucampia great, thanks. There are only a couple of current, pending PRs with tests that may need consideration since the pytest switch - PRs here, https://github.com/rdflib/rdflib/pulls, since about September.

For most pending PRs, I'm waiting for reviews from other maintainers or feedback from proponents.

test/jsonld/test_onedotone.py got a bit messed up with a merge from
master. Looking at the original changes from @ashleysommer, all he did
was change a condition. This applies the same change but essentially
rebased on master.

For comparison see: ab31c5e...c4b679f

Also add back import os in rdflib/parser.py

This is now needed after #1441 was
merged.
@aucampia
Copy link
Member

@nicholascar #1480 fixes tests

@aucampia
Copy link
Member

@ashleysommer @nicholascar mind if I rebase this onto latest master keep it mergeable?

@nicholascar
Copy link
Member

mind if I rebase...

Please do!

This is being used and should be imported.
@aucampia
Copy link
Member

aucampia commented Dec 28, 2021

Merged master back in and I went through the code again, I don't see any problems with it, but there are parts not covered by tests. I'm fine merging it though, we need to expand our testing of json-ld quite a bit anyway. But we can also look into that first before merging it.

@aucampia
Copy link
Member

I think we should clarify what the conditions is for this to be integrated into master, like I said I would like to see more testing but I think this is okay without that, but if others want to see more tests I don't mind working to add them. I want to at some point work on the test suite compliance but that will still be in some time.

Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

Looks good to me. Also the example mentioned in #1423 failed on master and works on this branch for me.

white-gecko and others added 3 commits March 16, 2022 21:45
Note re `_urlopen(full_link)` -> `_urlopen(Request(full_link))`

`_urlopen` expects a request object and will potentially use
`Request.full_url` which won't be available on a string.
@aucampia
Copy link
Member

pre-commit.ci autofix

@aucampia
Copy link
Member

Planning to merge this weekend if not merged yet and if there is no objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to parse graph (json-ld, JSONDecodeError)
7 participants