GrassBladeLRS throws "Unexpected end of JSON input" error

[brian-learningpool]

When launching a new attempt at a course with GlassBlade LRS, the call to /state/ returns a 200 OK but the state is an empty string, causing the JSON.parse() call on the following line to throw an error. https://github.com/adaptlearning/adapt-contrib-xapi/blob/master/js/adapt-contrib-xapi.js#L1051

[ht2]

I think this is because GrassBlade is not actually sending an "empty JSON string", but rather an empty string with no JSON.

e.g. Invalid

JSON.parse("")
VM159:1 Uncaught SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at <anonymous>:1:6
(anonymous) @ VM158:1

Valid

JSON.parse("\"\"")
""

This applies to non empty strings as well (assuming the application/json Content-Type is being sent). If GrassBlade actually wants to represent an empty JSON string, then it should be sending an escaped JSON version of that empty string.

e.g. Incorrect

JSON.parse("This is a string")
VM701:1 Uncaught SyntaxError: Unexpected token T in JSON at position 0
    at JSON.parse (<anonymous>)
    at <anonymous>:1:6

Correct

JSON.parse("\"This is a string\"")
"This is a string"

Alternatively, they could simply send a text/plain Content-Type and then Adapt shouldn't attempt to parse the content of the response as JSON (but presumably Adapt sent in the JSON in the first place)

[liveaspankaj]

Hi @ht2 @brian-learningpool

During a GET Request using State API, if a document is found, the Response header is same as the content type for that document (i.e. the Content Type header used when sending that document).

So, if you have a JSON document with stored content type: application/json . When fetching that document, the LRS will respond with application/json as Content Type. If you request Content Type: text/plain when the requested document is application/json, GrassBlade doesn't try to convert the document to text/plain. Nor is it a requirement in the spec, nor is is possible to convert types of contents.

https://screencast.com/t/0OZZGpCR

Now, when there is no content, the LRS responds with nothing in the content, which for convenience can be called an empty string. But it is not an empty JSON string. GrassBlade LRS doesn't add a response content type in such cases, however, in the browser I see the Response Content Type is: text/html; charset=UTF-8 which is probably the default content type for the browser.

Now, even if Adapt is requesting for application/json string, it should not blindly assume that the content is a JSON string, or an empty JSON string.

Converting data to any content type that is requested is beyond the scope of an LRS. Because tomorrow another application might ask for a binary content, or mp4 video content, and I do not know what is an empty video string.

Pankaj

[brian-learningpool]

@liveaspankaj, we could add a simple check to treat an empty string response as an empty state and skip the JSON.parse() in such instances.

Subsequent progress through the Adapt content will post JSON to /state. Will GrassBlade return those as JSON ok?

[ryasmi]

I'd probably just avoid JSON.parse if the content type isn't "application/json", that way if someone hijacks the state between requests you don't hit the same issues.

On Tue, 30 Jul 2019 at 11:39, Brian Quinn notifications@github.com wrote:

@liveaspankaj https://github.com/liveaspankaj, we could add a simple check to treat an empty string response as an empty state and skip the JSON.parse() in such instances.

Subsequent progress through the Adapt content will post JSON to /state. Will GrassBlade return those as JSON ok?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/adaptlearning/adapt-contrib-xapi/issues/69?email_source=notifications&email_token=AAXHRCOXATFI27XPBWVXXLTQCAK5BA5CNFSM4IHRAWHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DRXMA#issuecomment-516365232, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXHRCOTMN7NOW3JVL3NJZTQCAK5BANCNFSM4IHRAWHA .

-- https://www.ht2labs.com/ RYAN SMITH Email: ryan.smith@learningpool.com Phone: +44 (0)1865 873 862 Web: ht2labs.com https://www.ht2labs.com/

https://twitter.com/ht2labs https://www.linkedin.com/company/1219805/

[liveaspankaj]

@brian-learningpool sounds good. Yes, when you pull a stored JSON that was stored with Content Type application/json will return with the stored JSON with Content Type application/json . If there is an easy way to test, it is always best to confirm.

@ryansmith94 checking the content type makes sense, though I did not understand the state hijack part.

Alternative way could be to use try/catch, if you want to parse all json, and ignore the rest.

function parse_json(str) { try { return JSON.parse(str); } catch (e) { return false; } }

Pankaj

[ryasmi]

To clarify the state hijacking. Adapt could send in state with a JSON content type, some other state user could modify the state with a new content type (hijacking the state), Adapt might then try to read in the state only to find the content type is no longer JSON as it expected.

[liveaspankaj]

@ryansmith94 If state hijacking to modify content type is possible by design, it might be more reliable to not rely at all on content type and use try/catch.

Pankaj

[ryasmi]

Yeah you're probably right.

On Tue, 30 Jul 2019 at 13:32, Pankaj Agrawal notifications@github.com wrote:

@ryansmith94 https://github.com/ryansmith94 If state hijacking to modify content type is possible by design, it might be more reliable to not rely at all on content type and use try/catch.

Pankaj

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adaptlearning/adapt-contrib-xapi/issues/69?email_source=notifications&email_token=AAXHRCKJ6ZXCEJKHIRIAAWLQCAYFNA5CNFSM4IHRAWHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DZ4IA#issuecomment-516398624, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXHRCJUNGXGK7R5Q4744ZTQCAYFNANCNFSM4IHRAWHA .

-- https://www.ht2labs.com/ RYAN SMITH Email: ryan.smith@learningpool.com Phone: +44 (0)1865 873 862 Web: ht2labs.com https://www.ht2labs.com/

https://twitter.com/ht2labs https://www.linkedin.com/company/1219805/

[ht2]

@liveaspankaj Is the empty string "" that Grassblade is returning because that's exactly what was sent by Adapt in the original POST/PATCH?

Trying to establish if Adapt is sending blank strings as "\"\"" as expected (and if it isn't, why that is allowed into the LRS as it would not be valid JSON).

[brian-learningpool]

In this instance, no content has been sent from the Adapt content. This is the first call to GET /state.

On Tue, 30 Jul 2019 at 15:40, James Mullaney notifications@github.com wrote:

@liveaspankaj https://github.com/liveaspankaj Is the empty string "" that Grassblade is returning because that's exactly what was sent by Adapt in the original POST/PATCH?

Trying to establish if Adapt is sending blank strings as "\"\" as expected (and if it isn't, why that is allowed into the LRS as it would not be valid JSON).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adaptlearning/adapt-contrib-xapi/issues/69?email_source=notifications&email_token=AAKEC36W5OOHQ3UG6Z6UM4TQCBHHPA5CNFSM4IHRAWHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EGJJQ#issuecomment-516449446, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKEC363X3AWMEPRRDZS5MDQCBHHPANCNFSM4IHRAWHA .

-- BRIAN QUINN Senior Software developer Email: brian@learningpool.com Phone: +44 (0) 2871 223 687 Web: learningpool.com https://www.facebook.com/LearningPool/ https://twitter.com/LearningPool https://www.linkedin.com/company/learning-pool-ltd

http://www.learningpool.com

[liveaspankaj]

@ht2 like @brian-learningpool said, Adapt content is trying to "GET" what does not exist yet.

Pankaj

[ht2]

@ryansmith94 Wouldn't we expect a 404 in that scenario, if the state doesn't exist?

[ht2]

After doing a little more digging, here is the line in the xAPI spec that I would expect to be followed by the LRS: https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#user-content-3.2.s2.b4

404 Not Found - Indicates the requested resource was not found. May be returned by any method that returns a uniquely identified resource, for instance, any State, Agent Profile, or Activity Profile Resource request targeting a specific document, or the method to retrieve a single Statement.

I would expect that Grassblade should return a 404 if that State does not exist - returning a blank string (especially one that isn't JSON) is defintely invalid and as we've seen here will probably cause content problems.

[liveaspankaj]

@ht2 I agree 404 would be the right way to go from the LRS standpoint. However, some of the initial days contents gave errors saying LRS is 404 when they received 404 on State API calls.

And since then we decided to use 200 OK. We might change that, but it there is a potential risk and very low priority of this change.

The xAPI Spec is also undecided on it. The xAPI Spec explaining State API asks to send 200 OK. And the section of error codes says: "May be" returned by any method. The explanation complicates it further by limiting it to: "specific document" and "single Statement", that means if multiple documents or multiple statements are possible you send back 200.

So, 404 is clearly not something you can rely upon.

This point however brought another issue to my notice. That State API calls are not using registration value. This will cause a problem when the registration changes, state data will still return from previous registration and cause the attempt with new registration to look like already attempted.

Pankaj

[ryasmi]

However, some of the initial days contents gave errors saying LRS is 404 when they received 404 on State API calls.

@liveaspankaj do you remember which content authoring tools did this? I've been on the Learning Locker team for five years and never seen that.

The xAPI Spec is also undecided on it.

Whilst the conformance tests don't currently test this for state, they do test it for statements. Is there are any discussion on the spec's Github repository to suggest indecision besides the wording of the spec?

This point however brought another issue to my notice. That State API calls are not using registration value. This will cause a problem when the registration changes, state data will still return from previous registration and cause the attempt with new registration to look like already attempted.

When you tested this was the registration query parameter used in the URL? My understanding is that Adapt uses the xAPI wrapper which should probably be using the registration query parameter if it's provided.

[brian-learningpool]

@liveaspankaj, regarding registration , please see the discussion on the following PR https://github.com/adaptlearning/adapt-contrib-xapi/pull/62.

This is something we could add as a new configuration option, however, persisting the state between attempts is more akin to how Adapt/SCORM packages are typically used.

[liveaspankaj]

@ryansmith94

@liveaspankaj do you remember which content authoring tools did this? I've been on the Learning Locker team for five years and never seen that.

I could be the Tin Can v0.9 days around 2013-2014. Most likely the Articulate. I will have to test some content, but if you did not face the issue in last 5 years, I believe it could just be one or two cases. Gives me some confidence that adding 404 is possible.

Whilst the conformance tests don't currently test this for state, they do test it for statements. Is there are any discussion on the spec's Github repository to suggest indecision besides the wording of the spec?

I do not see any discussion regarding State API. I do see one regarding Statements, and you should get an idea why 404 is not very reliable. https://github.com/adlnet/xAPI-Spec/issues/499

When you tested this was the registration query parameter used in the URL? My understanding is that Adapt uses the xAPI wrapper which should probably be using the registration query parameter if it's provided.

It is possible that xAPI Wrapper adds registration parameter, and that is why I see it in Statement API queries. However, I do not see it for state api queries.

@brian-learningpool

@liveaspankaj, regarding registration , please see the discussion on the following PR #62.

This is something we could add as a new configuration option, however, persisting the state between attempts is more akin to how Adapt/SCORM packages are typically used.

We should not confused between attempts and registration. Though it can be a topic of debate because there is no standard involved, two are very different things, at least based on how most of the content work currently.

Attempts are mostly number of times a learner comes back to same content during same enrolment.

So, for example, taking a course on Sexual Harassment is a requirement for Mr A. He is enrolled into the course. He comes and takes 50% of the course today, and then comes back tomorrow, he restarts from where he left and completes the rest. This is possible because you are storing and reading the bookmark data from State API.

Now, next year, he has to retake the Sexual Harassment course, he is re-enrolled into the same course. He comes to the content, and what will happen now? In current Adapt scenario, I have not tested but most likely he will start from the end.

A commonly used method is to use registration as enrolment, so first year you get one registration, all state data is assigned to that registration. So, all attempts for that registration allow you to resume from where you left.

Next year the registration value changes, and you start afresh, but when you attempt again against same registration you are able to resume from where you left.

Those who want the state to persist always, can either not use any registration at all, or use one fixed registration value.

Use of registration depends on the authoring tool, however, a common practice is to use it with State API as well as Statement API. So that the enrolment use case can work.

Pankaj

[ryasmi]

I could be the Tin Can v0.9 days around 2013-2014. Most likely the Articulate. I will have to test some content, but if you did not face the issue in last 5 years, I believe it could just be one or two cases. Gives me some confidence that adding 404 is possible.

@liveaspankaj given the number of installations of Learning Locker (5k+ in the last 12 months) and the fact that we've always returned 404 in Learning Locker I'm very confident that adding 404 is possible.

I do not see any discussion regarding State API. I do see one regarding Statements, and you should get an idea why 404 is not very reliable. adlnet/xAPI-Spec#499

As you can see, I was actually involved in that discussion. It was also not a discussion about returning 404s for retrieving missing identified resources, however, Edoardo does say "It's ok responding 404 Not Found (identified resource)". Given my extensive experience with other APIs and the fact that Learning Locker has always returned a 404 in this case, I disagree that returning "404 is not very reliable", in fact this issue seems to highlight the fact that not returning a 404 is unreliable.

[brian-learningpool]

404 responses on calls to get state have been handled in this plugin for some time.

However there is a quick patch for this issue in https://github.com/adaptlearning/adapt-contrib-xapi/pull/71. Any reviews would be appreciated.