-
Notifications
You must be signed in to change notification settings - Fork 15
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
specify use of message/external-body for referenced content #121
Conversation
I'll have limited ability to respond to this thread this week, please feel free to add commits as necessary. |
<code>Content-Type: message/external-body; access-type=URL; URL=\"http://www.example.com/file\"</code>, | ||
as defined in [[!RFC2017]]. | ||
<p class='informative'> | ||
Implementations may support <code>Content-Type: message/external-body</code> extensions for |
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.
may
=> MAY
?
index.html
Outdated
<h3>External LDP-NR Content</h3> | ||
<p class='informative'> | ||
Variability among client types and locations may mean that LDP-NR | ||
content is addressed in ways that are external to LDP-S but not resolvable |
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.
Where is the def'n for LDP-S
? What is it?
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.
I am guessing LDP-S
is intended to mean LDP Server
... which should probably be the Fedora server
...as the server
is probably not exactly accurate in this situation.
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.
I agree: s/LDP-S/the Fedora server/
index.html
Outdated
content is addressed in ways that are external to LDP-S but not resolvable | ||
by all clients. This specification describes the use of <code>message/external-body</code> | ||
Content-Type value to signal, on POST or PUT, that the LDP-S should should not consider the | ||
request entity to be the LDP-NR's content, but that a Content-Type value will signal a |
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.
Content-Type
=> <code>Content-Type</code>
. There are several other missing <code>
tags.
index.html
Outdated
</section> | ||
<section id='external-content-content-location'> | ||
<p> | ||
LDP-NR GET and HEAD responses SHOULD include a Content-Location: header with a URI |
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.
Doesn't this destroy the "proxying for authZ" use case?
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.
Only if the URIs can be resolved by the client, and also doesn't enforce it's own access control. If either or both of things aren't true, then the server can safely expose the content location URI to the client, even when proxying for authZ.
When not proxying for authZ, exposing the content location URI allows the client to disintermediate the server if it wishes for performance or other reasons.
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.
Exactly-- the server is forced to trust the client (trust that the client won't resolve the URI or that the client will enforce the same authZ), so the use case is unsupported if this SHOULD
is executed. (It's unusual, to say the least, to make authZ depend on a trustworthy client.) It's not of great interest to me, because my institution does not require this use case, and we will be "implementing" by returning 415s in all cases, so I won't pursue it further.
index.html
Outdated
<h3>External LDP-NR Content</h3> | ||
<p class='informative'> | ||
Variability among client types and locations may mean that LDP-NR | ||
content is addressed in ways that are external to LDP-S but not resolvable |
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.
I am guessing LDP-S
is intended to mean LDP Server
... which should probably be the Fedora server
...as the server
is probably not exactly accurate in this situation.
index.html
Outdated
</section> | ||
<section id='external-content-expires'> | ||
<p> | ||
Per [[!RFC2017]], all <code>message/external-body</code> Content-Type values |
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.
I am seeing no mention of expires
or expiration
in RFC2017... but do in RFC1521
index.html
Outdated
may include an <code>expires</code> parameter. LDP-S receiving requests that | ||
would create or update a LDP-NR with a <code>message/external-body</code> content | ||
type SHOULD respect the <code>expires</code> parameter, if present, by copying | ||
content. If the expires parameter cannot be accomodated, the request MUST be rejected |
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.
typo: accomodated
should be accommodated
index.html
Outdated
would create or update a LDP-NR with a <code>message/external-body</code> content | ||
type SHOULD respect the <code>expires</code> parameter, if present, by copying | ||
content. If the expires parameter cannot be accomodated, the request MUST be rejected | ||
with a 4xx or 5xx status code. |
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.
A constrainedby
link header would also be useful here.
index.html
Outdated
This specification assumes that clients interested in resolving a <code>type='url'</code> | ||
or other value without the LDP-S as an intermediary (effectively, redirection as opposed | ||
to proxying) will be able to negotiate the <code>Content-Location</code> response header | ||
value from a HEAD request. |
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.
Not necessarily a question for the spec, but what would the expected response body be on a GET request of a redirect
LDP-NR? ... if we are expecting the client to figure out that an implicit redirect is available via inspection of the Content-Location
header.
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.
I read this paragraph as specifying a way for a client (at its option) to use HEAD
to decide whether it can go fetch remote content from the Content-Location
itself, avoid a default proxy action on GET
. It seems that from https://tools.ietf.org/html/rfc7231#section-3.1.4.2 the response to GET
should be either:
- 200,
Content-Location
and content in body to proxy - 3xx,
Content-Location
andLocation
to same URI to redirect (no body)
This PR seems to be heading in the right direction, it would be really helpful to have a pass that fixes the minor formatting issues so that we can focus on the more substantive ones |
index.html
Outdated
Per [[!RFC1521]], all <code>Content-Type: message/external-body</code> values | ||
MAY include an <code>expiration</code> parameter. Fedora servers receiving requests that | ||
would create or update a LDP-NR with a <code>message/external-body</code> content | ||
type SHOULD respect the <code>expiration</code> parameter, if present, by copying |
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.
Is the presence of expiration
intended to be a mechanism by which a client can instruct the repository to durably persist ephemeral content (see fedora-tech thread?
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.
Sorry, I simply find this section hard to understand what this section means from a high level. It suggests that a server SHOULD
copy content if an expiration parameter is present, but it could do something else, Including (as the above thread suggests) "expire the resource", which I understand as making the LDP-NR content unavailable once the expiration date is met. So is it consistent with the spirit of this section that one implementation might interpret expiration
as "make this ephemeral content durable", and another implementation might interpret it as "expire the content at the appropriate time"? It may be a stupid question, but I'd really appreciate some help from the editors to help understand how to interpret this section.
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 the MODE implementation, the answer to your first comment is, yes.
If there is no expectation that this be implemented in any other way, I would suggest we tighten the language to more clearly define what the expiration
parameter means/requires.
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.
Great, I believe some tightening and/or non-normative text is necessary if that's the case
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.
Should I create a new issue after this is merged to address this?
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.
Yes, please open that ticket.
index.html
Outdated
@@ -889,6 +884,79 @@ <h2 class='informative'>Vary</h2> | |||
</section> | |||
</section> | |||
|
|||
<section id='external-content'> |
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.
Besides the boilerplate text (Conformance, Terminology, X-Considerations, Acknowledgements, References) the top-level specification elements are the core Fedora capabilities:
- Resource Management
- Resource Versioning
- Binary Resource Fixity
- Resource Authorization
- Notifications
With that in mind, I would expect this new section (External LDP-NR Content) to be a sub-element under Resource Management, instead of a new top-level specification element.
index.html
Outdated
@@ -889,6 +884,79 @@ <h2 class='informative'>Vary</h2> | |||
</section> | |||
</section> | |||
|
|||
<section id='external-content'> | |||
<h3>External LDP-NR Content</h3> | |||
<p class='informative'> |
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.
This informative
annotation does not appear to have a visual effect in the rendered spec.
index.html
Outdated
as defined in [[!RFC2017]]. | ||
<p class='informative'> | ||
Implementations may support <code>Content-Type: message/external-body</code> extensions for | ||
request bodies for HTTP <code>POST</code> or <code>PUT</code> that would create <a>LDP-NR</a>s. |
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.
Do we need to have both verbs here? Can we just have only POST in the POST section and PUT in the PUT section?
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.
If this prevents you from approving, I can get to it sometime this weekend.
@barmintor I can pull on pull if you're strapped for time. |
I wouldn't worry about it, it's the commit-merge-squash-travis dance that
I'll need to wait to do.
…On Fri, Jun 9, 2017 at 1:29 PM, dannylamb ***@***.***> wrote:
@barmintor <https://github.com/barmintor> I can pull on pull if you're
strapped for time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHUNPuSzNTCxcCsWJoknmdX_eLkqv2mks5sCYD_gaJpZM4Now7t>
.
|
@barmintor ok, nbd |
fixes fcrepo#118 - fix formatting and terminology issues in discussion - Move 'External Binary Content' section into Section#3 'Resource Management'
fixes fcrepo#118 - fix formatting and terminology issues in discussion - Move 'External Binary Content' section into Section#3 'Resource Management'
fixes #118