Rewrite LoadDocumentCallback #87

gkellogg · 2019-05-03T23:47:03Z

to include most HTML-specific and profile-specific processing, and use in expand and Context Processing.

For #85.

gkellogg · 2019-05-03T23:50:34Z

Per our meeting, I've updated the document loader section to consolidate HTML and profile processing. I'll give this until EOD Monday before pulling and releasing a new WD, pending feedback. As discussed today, we won't close this issue pending further review, but don't want to hold back a WD.

dlongley

Organizationally, this is a massive improvement, IMO -- a much better separation of concerns. Thanks, @gkellogg! I will only be able to offer more detailed feedback (if any) when we attempt implementation. There do seem to be more profile requirements than before (from the last time I reviewed) and I don't know what impact there will be on existing systems (hopefully none!).

index.html

gkellogg · 2019-05-04T20:27:25Z

Organizationally, this is a massive improvement, IMO -- a much better separation of concerns. Thanks, @gkellogg! I will only be able to offer more detailed feedback (if any) when we attempt implementation. There do seem to be more profile requirements than before (from the last time I reviewed) and I don't know what impact there will be on existing systems (hopefully none!).

The profile requirement is that systems that don't process HTML, which includes all 1.0 processors, as they will reject the mime type, should raise the same error they would have under 1.0. 1.1 full processors MUST handle the HTML content, and include text/html in the Accept header (or equivalent). We don't seem to have every said in 1.0 that the Accept header should include application/ld+json or application/json but it seems implicit.

The net is that to be a full processor you'll need to support HTML and a callback implementation must conform to the specified behavior. We do have tests to make sure that it handles the requests properly, but not for processor type, nor do we presently have a way to test the compliance of custom loaders, but I'm not sure how we would.

There is some cleanup of the test needed to separate out the HTML behavior into a different manifest.

…e-specific processing, and use in *expand* and *Context Processing*.

@dlongley

Committed @dlongley's suggestions. Co-Authored-By: gkellogg <[email protected]>

pchampin · 2019-05-06T11:42:23Z

index.html

+            </li>
+          </ol>
+        </li>
+        <li>Otherwise, if the retrieved document's <a>Content-Type</a> is neither


Same remark as above, this is an "else", not an "else if".
Suggested change (1/3)

Suggested change

<li>Otherwise, if the retrieved document's <a>Content-Type</a> is neither

<li>Otherwise, the retrieved document's <a>Content-Type</a> is neither

<code>text/html</code>,

pchampin · 2019-05-06T11:44:46Z

index.html

@@ -6014,6 +6084,8 @@ <h2>Changes since JSON-LD Community Group Final Report</h2>
      of type <code>application/ld+json;profile=http://www.w3.org/ns/json-ld#context</code>,
      or <code>application/ld+json</code> is used as the context for further processing.
      This allows a mechanism for documenting the content of a context using HTML.</li>
+    <li>Consolodate <a>RemoteDocument</a> processing into the <a>LoadDocumentCallback</a>


Suggested change

<li>Consolodate <a>RemoteDocument</a> processing into the <a>LoadDocumentCallback</a>

<li>Consolidate <a>RemoteDocument</a> processing into the <a>LoadDocumentCallback</a>

pchampin · 2019-05-06T11:47:17Z

index.html

          </li>
+          <li class="changed">If necessary, transform <a data-link-for="RemoteDocument">document</a>


"If necessary" is not very clear to me. I'm guessing it means "if 'document' is a neither a dictionary not an array". This should be made explicit, all the more than a plain string is listed as a valid "internal representation" (which makes sense as the JSON syntax is able to represent a single string).

I'll change, but even though a string is a valid JSON document, it's not a valid JSON-LD document, so the intention should be clear.

pchampin · 2019-05-06T11:49:26Z

index.html

+            the <var>promise</var> is rejected with a <a>JsonLdError</a> whose code is set to <a data-link-for="JsonLdErrorCode">multiple context link headers</a>
+            and processing is terminated.</p>
+          <p>Processors MAY transform <var>document</var> to the <a>internal representation</a>
+            using the <a href="#extract-script-content">Extract Script Content algorithm</a>.</p>


This is confusing: this algorithm is explicitly provided for processing HTML scripts, which is not what we have here. Furthermore, it may raise an 'invalid script element' error, which would be inappropriate here.

I suggest rephrasing this in the spirit of line 5116 (modulo the improvement that I suggested).

I'll just remove the bit about the Extract Script Content, as we don't really need to show how it should be transformed.

At this point, it's just the HTTP body, so it will always be in a string form. The contract allows an user-supplied implementation to do the transformation.

pchampin · 2019-05-06T11:51:49Z

index.html

+            <li>Otherwise, if <var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is not present, or <code>false</code>,
+              the <var>promise</var> is rejected with a <a>JsonLdError</a> whose code is set to <a data-link-for="JsonLdErrorCode">loading document failed</a>
+              and processing is terminated.</li>              
+            <li>Otherwise, if <var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>,


If I understand correctly, this "otherwise" is an "else", but it reads like an "else if", which might confuse the reader.
I suggest rephtrasing it (1/2)

Suggested change

<li>Otherwise, if <var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>,

<li>Otherwise,<var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>.

I added some deeper lists to clarify.

pchampin · 2019-05-06T11:53:43Z

index.html

+          <code>application/json</code>,
+          <code>application/ld+json</code>,
+          nor any other media type using a
+          <code>+json</code> suffix as defined in [[RFC6839]],


(cont 2/3)

Suggested change

<code>+json</code> suffix as defined in [[RFC6839]],

<code>+json</code> suffix as defined in [[RFC6839]].

pchampin · 2019-05-06T11:54:16Z

index.html

+          <code>application/ld+json</code>,
+          nor any other media type using a
+          <code>+json</code> suffix as defined in [[RFC6839]],
+          reject the <var>promise</var> passing a <a data-link-for="JsonLdErrorCode">loading document failed</a> error.</li>


(cont. 3/3)

Suggested change

reject the <var>promise</var> passing a <a data-link-for="JsonLdErrorCode">loading document failed</a> error.</li>

Reject the <var>promise</var> passing a <a data-link-for="JsonLdErrorCode">loading document failed</a> error.</li>

pchampin · 2019-05-06T11:56:09Z

index.html

+              the <var>promise</var> is rejected with a <a>JsonLdError</a> whose code is set to <a data-link-for="JsonLdErrorCode">loading document failed</a>
+              and processing is terminated.</li>              
+            <li>Otherwise, if <var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>,
+              set <var>document</var> to a new empty <a>array</a>.


(cont 2/2)

Suggested change

set <var>document</var> to a new empty <a>array</a>.

Set <var>document</var> to a new empty <a>array</a>.

pchampin · 2019-05-06T11:58:08Z

index.html

+                or the located element is not a <a>JSON-LD script element</a>,
+                the <var>promise</var> is rejected with a <a>JsonLdError</a> whose code is set to <a data-link-for="JsonLdErrorCode">loading document failed</a>
+                and processing is terminated.</p></li>
+            <li>If <var>source</var> is defined and the,


Something is missing between "and the" and the trailing comma.

This should simply remove the "and the".

gkellogg · 2019-05-06T21:21:51Z

Thanks @pchampin, the algorithm had an odd branch logic because of some fall-through behavior. With some extra structure based on your suggestions, I think it reads more clearly.

gkellogg requested review from BigBlueHat, dlongley and pchampin May 3, 2019 23:47

dlongley approved these changes May 4, 2019

View reviewed changes

index.html Show resolved Hide resolved

index.html Show resolved Hide resolved

gkellogg and others added 4 commits May 4, 2019 14:22

Rewrite LoadDocumentCallback to include most HTML-specific and profil…

3ecb229

…e-specific processing, and use in *expand* and *Context Processing*.

Apply suggestions from code review

1a4bf00

Committed @dlongley's suggestions. Co-Authored-By: gkellogg <[email protected]>

Cleanup some promises language.

9f11d92

Fix HTML markup issues.

fb1e0ce

gkellogg force-pushed the issue-85-comprehensive-document-loader branch from bb0d61a to fb1e0ce Compare May 4, 2019 21:23

pchampin reviewed May 6, 2019

View reviewed changes

Address @pchampin's concerns and fix some other spelling issues.

27f650f

gkellogg merged commit c4c6dd0 into master May 6, 2019

gkellogg deleted the issue-85-comprehensive-document-loader branch May 6, 2019 21:44

This was referenced Jun 14, 2019

More compact @prefix #76

Open

compaction changes the meaning of absolute IRIs in some cases #104

Closed

gkellogg mentioned this pull request Jun 20, 2019

Update documentLoader to return parsed document #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite LoadDocumentCallback #87

Rewrite LoadDocumentCallback #87

gkellogg commented May 3, 2019 •

edited by pr-preview bot

Loading

gkellogg commented May 3, 2019

dlongley left a comment

gkellogg commented May 4, 2019

pchampin May 6, 2019

pchampin May 6, 2019

pchampin May 6, 2019

gkellogg May 6, 2019

pchampin May 6, 2019

gkellogg May 6, 2019

gkellogg May 6, 2019

pchampin May 6, 2019

gkellogg May 6, 2019

pchampin May 6, 2019

pchampin May 6, 2019

pchampin May 6, 2019

pchampin May 6, 2019

gkellogg May 6, 2019

gkellogg commented May 6, 2019

	<li>Otherwise, if the retrieved document's <a>Content-Type</a> is neither
	<li>Otherwise, the retrieved document's <a>Content-Type</a> is neither
	<code>text/html</code>,

	<li>Consolodate <a>RemoteDocument</a> processing into the <a>LoadDocumentCallback</a>
	<li>Consolidate <a>RemoteDocument</a> processing into the <a>LoadDocumentCallback</a>

		</li>
		<li class="changed">If necessary, transform <a data-link-for="RemoteDocument">document</a>

	<li>Otherwise, if <var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>,
	<li>Otherwise,<var>source</var> is undefined and the <a data-link-for="LoadDocumentOptions">extractAllScripts</a> option is <code>true</code>.

	<code>+json</code> suffix as defined in [[RFC6839]],
	<code>+json</code> suffix as defined in [[RFC6839]].

	reject the <var>promise</var> passing a <a data-link-for="JsonLdErrorCode">loading document failed</a> error.</li>
	Reject the <var>promise</var> passing a <a data-link-for="JsonLdErrorCode">loading document failed</a> error.</li>

	set <var>document</var> to a new empty <a>array</a>.
	Set <var>document</var> to a new empty <a>array</a>.

Rewrite LoadDocumentCallback #87

Rewrite LoadDocumentCallback #87

Conversation

gkellogg commented May 3, 2019 • edited by pr-preview bot Loading

gkellogg commented May 3, 2019

dlongley left a comment

Choose a reason for hiding this comment

gkellogg commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkellogg commented May 6, 2019

gkellogg commented May 3, 2019 •

edited by pr-preview bot

Loading