adlnet / xAPI-Spec

The xAPI Specification describes communication about learner activity and experiences between technologies.
https://adlnet.gov/projects/xapi/
905 stars 405 forks source link

Incoming statement with "stored" property #931

Closed cr8onski closed 8 years ago

cr8onski commented 8 years ago

The spec is clear that the lrs is to set the stored property. v1.0.2 is unclear what to do when receiving a statement with a stored property. v1.0.3 in part2.2.4.8.b (https://github.com/adlnet/xAPI-Spec/blob/1.0.2/xAPI.md#stored) says: The stored property MUST be set by the LRS; An LRS MUST validate and then overwrite any value currently in the stored property of a Statement it receives. It seems such a statement is to be accepted if the stored property is valid, and then overwrite the stored property. I suggest that validation be removed since the lrs is overwriting that property with a correctly formatted timestamp of then the statement is stored.

garemoko commented 8 years ago

See comment from @fugu13 here for discussion of why this was added.

creighton commented 8 years ago

Sorry about another post asking for clarification, we're just trying to get this straight for our LRS Test work...

This behavior seems contradictory to the behavior being discussed in the dup property discussion #924 . In this case the LRS MUST validate a part of the Statement it is also required to overwrite. Yet in 924 it seems that the LRS does not have to validate duplicate properties, since it will likely be sorted out by a JSON library.

Is there a general rule, guidance or best practice that's informing these decisions?

garemoko commented 8 years ago

In this case it's not a duplicate property - its a single property that the LRS overwrites.

The general principal we've been following is to have the LRS validate as much as possible. Why doesn't that seem to be applying to #924? That's a question to ask there :-)

fugu13 commented 8 years ago

Having a stored property that is set but doesn't validate is often a warning sign of code that is basically working but there's something going wrong some of the time that needs to be fixed. Strong guidance from the LRS will be an important debugging assist and an early warning sign.

Having doubled up properties is a sign of either someone manually typing statements and messing it up or code that's so far gone it virtually certainly is messing up ten billion things, neither of which I think is especially important for the LRS to provide strong guidance to. Heck, I'm honestly having a hard time figuring out how it'd happen with code without really trying, since almost no JSON libraries support it.

creighton commented 8 years ago

Still I'm hung up on behavior and impact.

In the first case (aka stored), we know that the LRS will overwrite the value with something that follows the rules in the spec: a 8601 time stamp. So impact is minimal, no matter how bad the value is messed up, as long as it's still valid JSON the LRS will always overwrite it the bad value with a valid value, as defined in the spec.

Yet in the case of the crazy, and agreed, very unlikely multiple property statement, the behavior is undefined - I mean the best we can say is that the JSON library that your software uses will decide for you. Granted most seem to take the last value for the property, but as far as I can tell that isn't actually specified anywhere. So the result that is actually stored in the LRS is unknown. That is what bothers me. We are kind of hoping that everything works out and we don't have to deal with it.

I don't want extra processing and arbitrary rules put in the spec. But I do want well defined behaviors and consistent treatment of data. Clearly I don't have a good answer and for that I'm sorry. Thanks for all your input and discussion.

garemoko commented 8 years ago

From June 15 call

Change "An LRS MUST validate and then overwrite any value currently in the" to "An LRS SHOULD validate and then overwrite any value currently in the "

Overwriting is a must, validating is a should.

See https://github.com/adlnet/xAPI-Spec/blob/1.0.3/xAPI-Data.md#248-stored