xAPI-Spec icon indicating copy to clipboard operation
xAPI-Spec copied to clipboard

PUT to Profiles without ETags

Open ryasmi opened this issue 7 years ago • 10 comments

Hey all, just looking for clarification with the expected behaviour of the LRS when receiving a PUT request without ETag headers (If-Match and If-None-Match).

The client requirements seem to suggest that the LRS should just return a 400.

A Client making a PUT request to either the Agent Profile Resource or Activity Profile Resource MUST include the "If-Match" header or the If-None-Match header.

However, the LRS requirements seem to suggest that the LRS should allow the client to supply neither and throw a 409 when the resource already exists and I guess implies a 204 otherwise.

If a PUT request is received without either header for a resource that already exists, the LRS:

  • MUST return HTTP status 409 Conflict.
  • MUST return a response explaining that the Learning Record Provider SHOULD
    • check the current state of the resource.
    • set the "If-Match" header with the current ETag to resolve the conflict.
  • MUST NOT make a modification to the resource.

@garemoko @fugu13

ryasmi avatar Aug 22 '17 14:08 ryasmi

@ryansmith94 the MUST only relates to the Agent Profile Resource or Activity Profile not the State.

garemoko avatar Aug 22 '17 17:08 garemoko

Yeah I understood that, I was referring to Agent Profiles and Activity Profiles when I said Profiles.

ryasmi avatar Aug 22 '17 18:08 ryasmi

Sorry, I should have read the whole section more carefully. Reading it all again, I can see the arguments for the LRS returning 409 or 400 in the case of a PUT to the Agent Profiles and Activity Profiles. I think I'd fall on the side of 409 though.

The position of the note about the state resource under the 'Client Requirements' heading is also confusing. Does it mean that the LRS requirements also apply to the state? I don't think they do, but it could be read that way.

image

garemoko avatar Aug 22 '17 19:08 garemoko

No worries. Ok could we please add something to the spec that says a "If a PUT request is received without either header for a resource that doesn't already exists, the LRS MUST return HTTP status 204 No Content"?

Also, you're right the comment about State is confusing and perhaps should be updated too.

ryasmi avatar Aug 22 '17 19:08 ryasmi

204 is definitely wrong, since it's a bad request, the LRS should return 400 in that case.

I was reflecting on this last night some more. In the case of a request without headers where the resource already exists is both a bad request and a conflict, so really the LRS can justify either error code depending on the order it does its validation. I lean towards 409 as a preference since that is a specific requirement. Searching through the spec, we don't actually specifically mention error code 400 in the case of missing headers (only extra unrecognized ones). The closest I could find is The LRS SHOULD enforce rules regarding structure.

garemoko avatar Aug 23 '17 09:08 garemoko

409 is the only correct LRS response to a PUT request missing both headers for an existing Profile resource. I went back to 0.95 to verify the text supports this. Here's the 0.95 text:

XAPI consumers should use these headers to avoid concurrency problems. The State API will permit PUT statements without concurrency headers, since state conflicts are unlikely. For other APIs that use PUT (Actor and Activity Profile), the headers are required. If a PUT request is received without either header for a resource that already exists, the LRS must return HTTP status 409 "Conflict", and return a plain text body explaining that the consumer must check the current state of the resource and set the “If-Match” header with the current ETag to resolve the conflict. In this case, the LRS must make no modification to the resource.

The requirements are right next to each other, so it is much clearer that the intent is for the LRS to use 409 here when the client doesn't follow the requirement.

This makes sense when you consider how the if-match and if-none-match headers work in HTTP generally, and that we tried to make xAPI reasonably well-behaved as a web API (definitely some work to do in future versions, but we did pretty well considering the other concerns being balanced).

409 is definitely not justifiable for a PUT request missing both headers to a Profile resource that does not exist; it would violate the meaning of a 409 response.

I think the only reasonable response by an LRS is a 400 in that case. The requirement for clients is clear, and the default assumption (that we should have been clearer about) is that the LRS is required to enforce (enforceable) client requirements, as without that we basically don't have interoperability.

fugu13 avatar Aug 23 '17 21:08 fugu13

I will say there's a bit of tension between the present/not present split, but I think that tension can be resolved. In both cases the choice is to use the most specific compatible error that gives information about the concurrency control needed.

fugu13 avatar Aug 23 '17 21:08 fugu13

Ok, I agree with both of you @garemoko @fugu13 and appreciate you taking the time to give your input. I think perhaps we should have some clarification in the spec that a 400 is expected for a PUT request missing both headers to a Profile resource that does not exist.

ryasmi avatar Aug 23 '17 21:08 ryasmi

Verify the behavior we are testing in the test suite, since ETag issues were discussed there. Make changes to align.

creighton avatar Sep 20 '17 18:09 creighton

I thought we discussed this case in the context of the tests, but it looks like we just discussed other ETag related issues.

@fugu13's take seems reasonable, and we should make sure not to miss that 204 is valid for a PUT to a state resource without either header.

bscSCORM avatar Sep 20 '17 19:09 bscSCORM