Validating documents in the LRS already doesn't make sense.

[garemoko]

xAPI

https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#requirements-8 If the document being posted or any existing document does not have a Content-Type of application/json, or if either document cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

Test Suite

Rejects a JSON update document when the original documents content-type is not 'application/json'

Rejects a JSON update document when the original documents content-type is 'application/json' but the original document is not valid json (XAPI-00281)

The problem

There's a problem with the logic of this requirement which means these tests are invalid. If the first request is bad, then the LRS is required to not update the target document and reject the request. This means that when the second (good) request is made, there is no original document to check the content or content type of. To put it another way: there's no point validating documents already in the LRS because they have already passed validation on their way in.

I'll cross post this as a test suite issue: https://github.com/adlnet/grail-issue-tracker/issues/50

[garemoko]

I propose we change the requirement to:

If the document being posted does not have a Content-Type of application/json, or cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

[garemoko]

Or possibly the intended meaning was: If the document being posted or any existing document does not have a Content-Type of application/json but the other does, or if either document cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request. If neither document has a Content-Type of application/json the LRS MUST replace the whole document as it would for a PUT request.

I don't think that's what the spec says as it stands though.

[andyjohnson]

I think this is a case where the comma killed us. I'm going to echo what @garemoko is saying:

I believe what is being said is: "If is is the case that the document being posted or any existing document does not have a Content-Type of application/json the LRS MUST..... OR if it is the case that if either document cannot be parsed as a JSON Object, the LRS MUST...."

What was intended was: "If the document being posted or any existing document a) does not have a Content-Type of application/json or b) if either document cannot be parsed as a JSON Object, the LRS MUST....

The OR is intended to give two choices, not be two requirements. All compound requirements were intentionally separated in 1.0.3. This was intended to be a single requirement with two options. The proposed change above is correct.

[garemoko]

I'm not sure whether or not we're on the same page. It's such a long sentence it's hard enough to decipher my own summaries let alone yours. On that basis I wonder if we should split it up as separate requirements, even if there is a lot of duplication between the two.

So with the current wording I think we have four requirements:

1. If the document being posted does not have a Content-Type of application/json the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

2. If the existing document does not have a Content-Type of application/json the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

3. If the document being posted cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

4. If the existing document cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

But 2 and 4 don't make sense because an existing document will never not be pars-able JSON because requirements 1 and 3 ensure they are rejected and not stored.

What I think the intended meaning might have been is more like:

1. If the document being posted does not have a Content-Type of application/json the existing document does, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

2. If the existing document does not have a Content-Type of application/json but the document being posted does the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

3. If the document being posted has a content type of Content-Type of application/json but cannot be parsed as a JSON Object, the LRS MUST respond with HTTP status code 400 Bad Request, and MUST NOT update the target document as a result of the request.

Again, the scenario where the stored document is application/json but cannot be parsed would not happen because of the validation when the document comes in required be 3.

Essentially the difference between what I think the spec says and what I think was intended is whether or not the LRP is allowed to POST non-json documents or whether it has to PUT them.

[oconnetf]

Per today's call, this strikes me as related to https://github.com/adlnet/xAPI-Spec/issues/1015. Part of what prompted me to open that was the confusing semantics of PUT/POST and whether a document was JSON or not.

[andyjohnson]

Quick notes per the Jan 4th, 2017 call - PUT was intended for store/update of JSON or non-JSON. POST was later allowed for JSON-only updates. Requirement of content-type is not sufficient for behavior, need to actually parse.

[garemoko]

@bscSCORM commented on the call that an invalid json document or non-json document could be stored using PUT and then a POST could be used to try to update it. So it does make sense to check the original document. Closing.