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

Better explain 200 = ignored with Expect: 202-digest? #33

Closed
zimeon opened this issue Feb 1, 2017 · 23 comments
Closed

Better explain 200 = ignored with Expect: 202-digest? #33

zimeon opened this issue Feb 1, 2017 · 23 comments

Comments

@zimeon
Copy link
Contributor

zimeon commented Feb 1, 2017

Section 3.3.1 says:

A 200 OK status indicates that the server did not understand the request, and ignored the expectation.

Which on the face of it is very odd, and seems at odds with rfc2616 "A server that does not understand or is unable to comply with any of the expectation values in the Expect field of a request MUST respond with appropriate error status. The server MUST respond with a 417 (Expectation Failed) status if any of the expectations cannot be met or, if there are other problems with the request, some other 4xx status."

and then Section 3.3.2 says (non-normatively):

A client aware of 202-digest that receives a 200 should assume it is responsible for verifying fixity by downloading the resource.

which suggests a server implementation choice about supporting the fixity check.

A while ago I thought I understood this all to mean:

A 200 OK status indicates that the server did not understand or could not compute (or chose not to compute?) the 202-digest expectation, and thus ignored the expectation while otherwise successfully responding the HTTP request.

but as I have tried to write this up I find myself unsure again. I think they key question to clarify this is "Under what conditions is it OK for a server to give 200 OK in response to an Expect: 202-digestrequest?" My attempt to enumerate is:

  1. doesn't support 202-digest -- no, 417
  2. requires digest-param but none supplied -- no, 417
  3. unrecognized or unsupported instance-digest -- maybe?
  4. implementation choice -- maybe?
  5. HTTP/1.0 server (I mention only because HTTP/1.1 is mentioned) - yes, as would not look for Expect parameter, but since LDP is defined over HTTP/1.1 this seem irrelevant?
  6. other things I'm missing?

Sorry this is so long!

@ajs6f
Copy link
Contributor

ajs6f commented Feb 2, 2017

No reason to apologize-- this is exactly the kind of feedback we need!

For my money, 4 is no. It's not an impl choice to ignore the header. 5 is not relevant, for exactly the reason you give.

3 is the sticky bit. I think we settled on 412 for that, no? Is that not what we have written down now?

@barmintor
Copy link
Contributor

We went with 202 to make it as clear as possible that the server and client are acting according to the same expectations. Because of the particular nature of fixity checking, and because specifications are frequently incompletely implemented, I thought it should be explicitly reflected in the status.

@zimeon
Copy link
Contributor Author

zimeon commented Feb 3, 2017

@barmintor - I think 202 response for good fixity check is fine (just trying to work out when, if ever, a 200 response is appropriate for a 202-digest request)

@ajs6f - not sure whether 412 is appropriate for 3 (unrecognized or unsupported instance-digest) - that smells more like 400 to me. Either way, I think in the current text it isn't clear what code should be returned/expected for case 3 so should probably be made explicit.

But anyway, if case 3 is 4xx, case 4 does not apply, and case 5 is irrelevant, then I do not know when a client would ever get a 200 so I think mention of that should be removed?

@zimeon
Copy link
Contributor Author

zimeon commented Feb 3, 2017

Also, if support for 202-digest is required and there is no case when 200 is returned, I think section 3.3.2 Ignored Expectations and 200 (OK) should be deleted.

@barmintor
Copy link
Contributor

I think, for what it's worth, that 3.3.2 is not the problem (I think it's useful non-normative guidance), but that you are right about the parallel sentence in 3.3.1 (which is normative). Generally speaking, I think it is a mistake to write a spec like this as if it will be completely implemented- as far as I know, none of the LDP implementations pass the entire TC- and I would expect backfilling against one of those, or a mixed approach to be more common.

Let us say that a LDP implementation, we'll call it Marmothesis, decided to borrow the versioning spec. If the question were put to me, "Can I run Islandora or Hydra against Marmothesis?" I would like to be able to fall back on some guidance to know where my middleware needs to fill gaps. In many cases, this is detectable from response headers ("Is there a timemap?"). In the case of fixity checking generally, which we have feedback for that characterizes it as optional, and in this case, for which we introduce a new Expect header that we also have limited ability to forecast extensions of by different implementations (link values or hash algorithms), a reminder that HTTP 200 means the header was ignored strikes me as a useful caution.

I'm willing to be overruled, but the lack of expected scenarios in which a conformant implementation returns a 200 is in this case the reason I think it should be noted.

@barmintor
Copy link
Contributor

Support for 202-digest is a SHOULD in section 1.5.2.1

@ajs6f
Copy link
Contributor

ajs6f commented Feb 6, 2017

Yes, and I think that is causing a lot of confusion. I'm sympathetic to your points about the partial fulfillment of specs, @barmintor, but I think this SHOULD errs too much in one direction. We have to assume that people will make a certain level of effort to impl or we make the spec effort radically less useful.

@barmintor
Copy link
Contributor

I don't think the confusion of this ticket is changed according to MUST/SHOULD of 202-Digest: If I am a client, and I send a 202-digest expectation, and I get a 200, what does it mean? It means that, whatever the reason, the client should not expect the server to have verified the digest. If our attempts to clarify that in a non-normative note are counter-productive, we should drop the note.

I should add that we had (I thought) consensus that fixity was a required feature, but that the minimum server features to support it were only Want-Digest, support for which is indicated as a MUST in section 1.5.2. This matches some of the reported fixity verification use cases (a client system stores original/expected digests, and compares them to the reports from Want-Digest). 202-digest supports more specialized use cases.

@zimeon
Copy link
Contributor Author

zimeon commented Feb 6, 2017

If we establish that a conforming implementation will never respond to a 202-digest request with a 200, then I think it is confusing to ever mention what what 200 means (as opposed to 201 or 204 or 299) -- all are simply non-conforming responses and should be understood as not a 202, which has defined meaning.

@ajs6f
Copy link
Contributor

ajs6f commented Feb 7, 2017

@barmintor No, that was not my understanding of our decision. I very much thought that 202-digest was to be understood as a MUST.

@ajs6f
Copy link
Contributor

ajs6f commented Feb 7, 2017

@zimeon I think that exactly hinges on whether 202-digest is optional. If it is, there must be some way for servers that fail to support it to admit the failing. If not, it is, as you say, merely confusing to talk about it here.

@barmintor
Copy link
Contributor

Well, as I said: I am sympathetic to dropping the note, or at least changing it to reaffirm that non-202 responses signal non-conformance. It might be worth noting the HTTP/1.1 spec's position on missing support for other expectations does not require a 417, only defines its meaning:

A server that receives an Expect field-value other than 100-continue
MAY respond with a 417 (Expectation Failed) status code to indicate
that the unexpected expectation cannot be met.

As a client, I only find the MUST designation useful when the lack of a behavior cannot be detected or worked around. In this case you can reliably infer that from non-202 responses, so I think a note is valuable, even if singling 200 out is not (my PR included the note, but different strokes). I don't personally see what the promotion of 202-digest support from SHOULD to MUST would accomplish, and I still don't know how it bears on the situation here. If we continue to have implementations that redirect LDP-NR content requests, I think it would be courteous to make a particular claim about other 2XX statuses, but again: this might just be me.

@ajs6f
Copy link
Contributor

ajs6f commented Feb 7, 2017

The "promotion of 202-digest support from SHOULD to MUST would accomplish" that any Fedora impl will satisfy the exact use cases for which my users are clamoring now. That is not to say that those use cases could never be satisfied in any other way. But if the spec doesn't actually make its best effort to ensure that I can switch impls... then I'm not sure why I am spending so much time on it.

@awoods
Copy link
Collaborator

awoods commented Feb 15, 2017

My suggestion here is to modify section 3.3.2 as follows:

3.3.2 Ignored Expectations on non-202 (Accepted)
This section is non-normative.
A client aware of 202-digest that receives a non-202 response should assume it is responsible for verifying fixity by downloading the resource.

Given that we state that support for Want-Digest is a MUST, I suggest we keep section 1.5.2.1 as-is.

@ajs6f
Copy link
Contributor

ajs6f commented Feb 15, 2017

-1 to leaving support for 202-digest optional.

@awoods
Copy link
Collaborator

awoods commented Feb 15, 2017

@barmintor ? @zimeon ? @acoburn ? @birkland ? @ruebot ? @no-reply ?
Can you offer input on the the suggestion above?

@birkland
Copy link
Contributor

Reading the spec after having read the comments on this issue, I actually think it makes more sense to remove section 1.5.2.1, as it seems redundant to section 3. To me, it reads better when the concerns are orthogonal (resource management vs fixity).

I think 3.3.2 can be eliminated as well; just move the non-normative text to 3.3.1. In that case, the narrative is simple: a 202 means the repository was able to verify fixity. Anything else means that it did not. That includes the 3xx for GET requests to message/external-body resources as described in 1.5.2.

@zimeon
Copy link
Contributor Author

zimeon commented Feb 16, 2017

The thing there seems to be some agreement on is that it is unhelpful to mention 200. Thus to make some progress I think we should look for minimal changes to remove mention of 200. I like @birkland's suggestion to remove 3.3.2 and merge in to the 3.3.1 statement to clarify that any response other than 202 means the repository did not successfully verify fixity. I'll try a PR for this...

I'd suggest keeping section 1.5.2.1 for now, if only to avoid expanding the scope of this hard-to-resolve issue. If there is more debate there then perhaps new issue?

I think the underling BIG ISSUE for much of the difficulty here is that there is not agreement on how prescriptive and how testable this spec should be. That will need to be resolved in order for a good outcome overall.

@awoods
Copy link
Collaborator

awoods commented Feb 16, 2017

Thanks, @zimeon #56 is a constructive, incremental step forward.

@ruebot
Copy link
Contributor

ruebot commented Feb 16, 2017

Partially addressed with: 8223e7c

@zimeon
Copy link
Contributor Author

zimeon commented Apr 6, 2017

Transmission fixity is consistently (internally and with LDP & RFC3230) described in the POST LDP-NR and PUT LDP-NR sections where server responses tell a client:

  • 409 => fixity error
  • 400 => digest type not supported
  • 201/201/204 => success (though digest might have been ignored as allowed by SHOULD in definition of 400 response and RFC3230; no way to tell if supported)

Persistence fixity is defined in terms of the 202-digest expectation where server responses tell a client:

  • 417 => unsupported:
    1. server does not support 202-digest expectation (i.e. no support for persistence fixity)
    2. does not support 202-digest without a supplied digest (i.e. no support for intenal check)
    3. server does not support one of the 202-digest expectations (mentioned only in 3.5.2.1 Expect: 202-digest)
  • 412 => fixity error
  • 3xx => if external-body (from 3.5.2.1 Expect: 202-digest)
  • 202 => successful fixity check
  • 200 => no fixity check was made in case of 202-digest without a digest-param (servers only SHOULD compare digest!)

I note that there is not agreement to make support for any particular digest type(s) mandatory (see also #32).

I think that there are two changes that would tighten up persistence fixity to be clear:

  1. Change "SHOULD compare digest" to "MUST compare digest" for the case that a server supports the case of 202-digest without a digest-param. Why would it make sense to "support" that and then not actually do the check? Servers that don't support 202-digest without a digest-param can just do 417. I will suggest a PR for this.

  2. Move or repeat some of the language in 3.5.2.1 Expect: 202-digest in Persistence fixity. First, 3.5.2.1 applies only to GET whereas we want to language about "Implementations that do not support one of these expectations must reject 202-digest requests for the expectation with a 417 Expectation Failed. If the LDP-NR is of Content-Type: message/external-body, the server must respond with a 3xx." to apply to HEAD too. Second, it is easy to miss if you looking for information about fixity in the Binary Resource Fixity section.

@ruebot
Copy link
Contributor

ruebot commented Apr 7, 2017

Partially resolved with: fa627da

@zimeon
Copy link
Contributor Author

zimeon commented Apr 7, 2017

Bullet 1 addressed by fa627da , have created #86 to deal with bullet 2 as setting in this thread seems confusing now. Propose close.

@zimeon zimeon closed this as completed Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants