-
Notifications
You must be signed in to change notification settings - Fork 131
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
request JSON-LD from Link rel=alternate #129
base: master
Are you sure you want to change the base?
Conversation
This PR does not fixes the following test:
because https://schema.org gives (Pdb) p headers["Accept"]
'application/ld+json;profile=http://www.w3.org/ns/json-ld#context, application/ld+json, application/json;q=0.5, text/html;q=0.8, application/xhtml+xml;q=0.8' it's here due to 6573 # FIXME: only if html5lib loaded?
6574 headers['Accept'] = headers['Accept'] + ', text/html;q=0.8, application/xhtml+xml;q=0.8' According to json-ld:
So even if we accept HTML, if it's HTML and there's a json-ld alternative, let's use it. To do this, I'll move In other words (and with a correct indentation) I mean: --- a/lib/pyld/documentloader/requests.py
+++ b/lib/pyld/documentloader/requests.py
@@ -69,7 +69,6 @@ def requests_document_loader(secure=False, **kwargs):
'contentType': content_type,
'contextUrl': None,
'documentUrl': response.url,
- 'document': response.json() if content_type in headers['Accept'] else None
}
link_header = response.headers.get('link')
if link_header:
@@ -77,15 +76,15 @@ def requests_document_loader(secure=False, **kwargs):
LINK_HEADER_REL)
# only 1 related link header permitted
if linked_context and content_type != 'application/ld+json':
- if isinstance(linked_context, list):
- raise JsonLdError(
- 'URL could not be dereferenced, '
- 'it has more than one '
- 'associated HTTP Link Header.',
- 'jsonld.LoadDocumentError',
- {'url': url},
- code='multiple context link headers')
- doc['contextUrl'] = linked_context['target']
+ if isinstance(linked_context, list):
+ raise JsonLdError(
+ 'URL could not be dereferenced, '
+ 'it has more than one '
+ 'associated HTTP Link Header.',
+ 'jsonld.LoadDocumentError',
+ {'url': url},
+ code='multiple context link headers')
+ doc['contextUrl'] = linked_context['target']
linked_alternate = parse_link_header(link_header).get('alternate')
# if not JSON-LD, alternate may point there
if (linked_alternate and
@@ -93,9 +92,8 @@ def requests_document_loader(secure=False, **kwargs):
not re.match(r'^application\/(\w*\+)?json$', content_type)):
doc['contentType'] = 'application/ld+json'
doc['documentUrl'] = prepend_base(url, linked_alternate['target'])
- if content_type not in headers['Accept']:
- # Original was not JSON/JSON-LD; fetch linked JSON-LD
- return loader(doc['documentUrl'], options=options)
+ return loader(doc['documentUrl'], options=options)
+ doc['document'] = response.json()
return doc
except JsonLdError as e:
raise e |
Hmm, that's a bit odd. Yes, Pyld adds if content_type not in headers['Accept']:
# Original was not JSON/JSON-LD; fetch linked JSON-LD
return loader(doc['documentUrl'], options=options) is important, it checks whether the server responded with |
Totally agree. But looks like https://schema.org/ does not have a
Which looks legit to me, even though I still didn't read and understood rfc8288 entierly.
IIRC It looks already covered by the:
check a few line before. |
Ah I see. Sorry it's been a while since I've looked at this. I think your version of the PR makes sense, and as long as both our tests pass, it gets my vote. |
I'd like to add both tests to the test suite, but I don't understand how to do so. If someone can explain before merging I'd gladly do so. |
@@ -12,8 +12,7 @@ | |||
import string | |||
import urllib.parse as urllib_parse | |||
|
|||
from pyld.jsonld import (JsonLdError, parse_link_header, LINK_HEADER_REL) | |||
|
|||
from pyld.jsonld import (JsonLdError, parse_link_header, prepend_base, LINK_HEADER_REL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from pyld.jsonld import (JsonLdError, parse_link_header, prepend_base, LINK_HEADER_REL) | |
from pyld.jsonld import JsonLdError, parse_link_header, prepend_base, LINK_HEADER_REL |
I believe the braces are redundant.
Ref #128
With the following test cases defined:
context.jsonld
manifest.jsonld
sample.jsonld
sample2.jsonld
output.jsonld
The tests both fail before the changes. The tests both passs after the changes. Since all the test cases of this repository are remote, I am not sure whether or where to contribute these test cases.
However, there is one regression, which I do not currently know how to resolve. This test does not have an error before the changes, and does have an error after the changes. The report is:
All other tests are unaffected. However, as mentioned in #128, running the tests as described with no changes elicits 5 failures, 2 errors, and 34 skipped tests.