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

Updating language for POSTing to an LDPCv #239

Merged
merged 3 commits into from
Oct 20, 2017
Merged

Conversation

dannylamb
Copy link
Contributor

Resolves #215

I kept the SHOULDs to maintain the assertiveness of the original language, but could be compelled to upgrade to MUST, though I admit I'm not sure of the depth of the ramifications that would incur.

awoods
awoods previously approved these changes Oct 4, 2017
escowles
escowles previously approved these changes Oct 11, 2017
@zimeon
Copy link
Contributor

zimeon commented Oct 11, 2017

I don't understand the motivation for this paragraph:

If an LDPCv supports POST, a POST without a request body but with a Memento-Datetime header SHOULD be understood to create a new LDPRm contained by the LDPCv, reflecting the state of an empty LDPRv at the time of the Memento-Datetime request header.

it seems to say make a zero size memento which seems not very useful. But anyway, I think this is just a special case of POST with Memento-Datetime where the body is stored as the memento, so I think this paragraph is unnecessary.

I think the logic is really:

  • POST has Memento-Datetime?
    • yes - make memento with given body (size >= 0) and that timestamp
    • no - body is zero size?
      • yes - make memento of current state with LDPRv
      • no - error, body must be zero size

@escowles
Copy link
Contributor

I agree with @zimeon that POSTing with a Memento-Datetime header and an empty body is a special case of doing a POST with a Memento-Datetime header. So I'd be fine with rolling that into one paragraph instead of calling it out separately. That should help reduce the duplicate language and make the spec more readable.

@dannylamb dannylamb dismissed stale reviews from escowles and awoods via 6903a7b October 11, 2017 14:57
@dannylamb
Copy link
Contributor Author

@escowles @zimeon I've updated the language collapse the paragraphs for non Memento-Datetime POSTs.

index.html Outdated
If an <a>LDPCv</a> does accept <code>POST</code> with a request body, it SHOULD respect a
<code>Memento-Datetime</code> request header for the created <a>LDPRm</a>. Absent this header, it MUST use
the current time.
If an <a>LDPCv</a> supports <code>POST</code>, a <code>POST</code> with both a request body and a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove "both a request body and".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

index.html Outdated
<p>
If an implementation does not support <code>POST</code> with a request body, the <code>Accept-Post</code>
header of any response from the <a>LDPCv</a> SHOULD indicate that no request body is accepted via the form
<code>Accept-Post: */*; p=0.0</code>, and that implementation MUST respond to any body-containing
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted by @bseeger in #228 p=0.0 should be q=0.0. It may also be imperiled.

index.html Outdated
<code>Memento-Datetime</code> header SHOULD be understood to create a new <a>LDPRm</a> contained by the
<a>LDPCv</a>, reflecting the state of the <a>LDPRv</a> at the time of the <code>POST</code>.
Implementations MUST fail such requests that also contain a non-empty body by responding
with a 400 Bad Request.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I agree with the non-empty body. If it's a dumb implementation, that may be how the versions are created. If the impl doesn't support request entities on version creation via POST, that should be indicated somehow (as in the next 2 paragraphs).

Copy link
Contributor

Choose a reason for hiding this comment

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

That is to say that a combination of Accept: from the server and a 400 response should tell the client that the operation was illegal - I don't think we should specify anything else here.

@dannylamb
Copy link
Contributor Author

@zimeon @escowles @barmintor I've tried my best to incorporate your feedback. Seeing where we've landed now, it almost feels we could just as easily say that, if absent, the Memento-Datetime header has a default value of the time of the request. Am I reducing too much or missing a corner case? I may be able to trim some more language if that holds water.

Copy link
Contributor

@barmintor barmintor left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@awoods awoods merged commit f05fe23 into fcrepo:master Oct 20, 2017
barmintor pushed a commit to barmintor/fcrepo-specification that referenced this pull request May 22, 2018
* Updating language for POSTing to an LDPCv

Resolves: fcrepo#215
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.

5 participants