search

IIIF / trc

Technical Review Committee issue review
Apache License 2.0
1 stars 1 forks source link

Recipe #466: Sharing a link to open a Manifest in a specific viewer. #125

Open glenrobson opened 5 months ago

glenrobson commented 5 months ago

Links

Background and Summary

This recipe is the first one that covers the IIIF Content State API. This is the most basic version of the use of Content state which opens up a Manifest in a specific viewer. Once the basic one is complete hopefully we can move on to opening a specific canvas in a viewer and other recipes.

Voting and changes

We welcome comments on the recipe and as well as voting +1, confused face or -1 feel free to add comments to this issue. If this issue is approved then the author will take account of the comments before we merge the branch in to the master cookbook branch.

If the recipe is rejected by the TRC then we will make the changes requested and resubmit it to a future TRC meeting. If you feel that your comments are substantial enough that the recipe should be looked at again by the TRC after the changes have been made please vote -1 (thumbs down). A confused face is treated as abstaining.

Changes to the recipe will only be made after the TRC voting process has concluded.

nfreire commented 5 months ago

I approved the recipe, but I have a comment about some of the URLs. The values of URL parameters should be URL encoded (using UTF-8), otherwise they are violating the URL syntax.

For example: https://projectmirador.org/embed/?iiif-content=https://iiif.io/api/cookbook/recipe/0001-mvm-image/manifest.json should be: https://projectmirador.org/embed/?iiif-content=https%3A%2F%2Fiiif.io%2Fapi%2Fcookbook%2Frecipe%2F0001-mvm-image%2Fmanifest.json

The URL syntax may be consulted in rfc 1808, section 2.2.

If the values of URL parameters are not encoded, in some cases there are characters that will "confuse" URL parsing. For example, if a manifest was available at https://example.org/getManifest?id=12345, when it is passed in the iiif-content parameter, the final URL, after parsing would end up with two parameters. For example: https://projectmirador.org/embed/?iiif-content=https://example.org/getManifest?id=12345 after parsing would result in these parameters:

zimeon commented 5 months ago

There are some missing back ticks for quoted literals in the recipe, e.g.:

I think the current sentence is confusing:

Note when the content state is a plain URI, rather than a JSON object, it must not be content-state-encoded.

in that we are talking only about about the URI reference case and don't otherwise mention JSON objects. Also, the "it" (the manifest URI) is not very clear. It probably is good to mention that the Content State spec provides for more complex reference to state using JSON objects rather than just omitting. Perhaps something like:

Note that the manifest URI might need to be URL encoded if it contains characters that conflict with URI query component syntax.

The Context State API also provides mechanisms to convey more detail about which part of a resource should be shown. This is achieved by passing an encoded JSON object instead of the manifest URL, and is described in other recipes.

I do not think that it is necessary to encode the : and / characters in @nfreire's example above (though it is certainly OK to do so), but it would be necessary to encode other query parameters (RFC1808 was obsoleted by RFC3986)

azaroth42 commented 5 months ago

Thanks for calling out the encoding issue @nfreire! Per the spec, for HTTP GET requests:

the GET request parameter must be content-state-encoded as described in Section 6 below

So the links provided in the recipe aren't correct as written, they should be encoded as base64.

So :-1: from me until that gets fixed.

glenrobson commented 5 months ago

Thanks for calling out the encoding issue @nfreire! Per the spec, for HTTP GET requests:

the GET request parameter must be content-state-encoded as described in Section 6 below

So the links provided in the recipe aren't correct as written, they should be encoded as base64.

So 👎 from me until that gets fixed.

I'm not sure it should be base64 encoded. In section 6 it says:

"When the content state is a plain URI, rather than a JSON object, it must not be content-state-encoded."

https://iiif.io/api/content-state/1.0/#62-content-state-encoding-and-uri-requirements

also in the following section:

https://iiif.io/api/content-state/1.0/#initialization-mechanisms-link

The simple URL example is not encoded but the JSON version explicitly mentions it should be content-state-encoded.

azaroth42 commented 5 months ago

Yes, you're right @glenrobson. I think that's a bug in the spec :( It should have some sort of explicit encoding requirement, otherwise we'll end up applying the wrong number of encoding/decoding pairs.

At which point the recipe conforms to the spec ... but I think the spec needs to be changed! So I'm overall -1 on publishing it, through no fault of the recipe authors.

@tomcrane?

stephenwf commented 5 months ago

It feels like a step back not to be able to do: iiif-content=https://example.org/manifest on viewers. Prevents any sort of manual crafting for the average user

azaroth42 commented 5 months ago

The value of notes :)

The encoding of URIs issue: https://github.com/IIIF/discovery/issues/90 Which caused concern in the TRC first time round when URIs were encoded.

Then: https://docs.google.com/document/d/1YZtE7OCGD5bEO52Xa_o-aE40Z9JkJ8awE1j43-rGZuU/edit documents the proposed response:

Proposed change: to clarify that simple URIs can be plain, or URI encoded, but must not be explicitly content-state encoded.

I guess I shouldn't have missed that meeting!

Currently it's undetermined how many times iiif-content=https://example.org/manifest/%2550 should be decoded... is it %50 on the end or P ?

--> https://github.com/IIIF/api/issues/2292

zimeon commented 5 months ago

Wrt. encoding I think both the spec and the recipe are OK, but could be improved by some additional mention. To get really into the weeds, RFC3986 URI syntax says the following about what characters can appear in the query component of the URI:

query = *( pchar / "/" / "?" )

where

pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

So, the query component can explicitly include “/”, “:”, "-" and a number of other characters that a brute-force URL encoding routine may encode. The URI spec also notes that as “one frequently used value is a reference to another URI, it is sometimes better for usability to avoid percent-encoding those characters.”

I thus maintain that the two examples in the recipe:

are best left as-is, without any additional encoding (and we note they do also work fine, which is reassuring).

stephenwf commented 5 months ago

if ?iiif-content=${encodeURIComponent(url)} is used then it should match exactly, regardless of which is used.

const url1 = "https://example.org/manifest/%2550";
const url2 = "https://example.org/manifest/%50";
const encoded1 = encodeURIComponent(url1);
const encoded2 = encodeURIComponent(url2);

const searchParams = new URLSearchParams(`a1=${encoded1}&a2=${encoded2}`);
const out1 = searchParams.get("a1");
const out2 = searchParams.get("a2");

console.log(out1 === url1); // true
console.log(out2 === url2); // true
nfreire commented 5 months ago

@zimeon I double checked the RFCs and I agree with your assessment.

azaroth42 commented 5 months ago

Agreed as well.

glenrobson commented 4 months ago

Issue 125 (Recipe #466: Sharing a link to open a Manifest in a specific viewer.)

+1: 13 [JulieWinchester akrishnan15 cubap glenrobson julsraemy markpatton mikeapp nfreire regisrob rentonsa robcast tpendragon zimeon] 0: 0 [] -1: 0 [] Not TRC: 1 [ioanrichards] Ineligible: 2 [azaroth42 omaeazusa]

Result: 13 / 13 = 1.00

Super majority is in favor, issue is approved