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

7.2.3 is unRESTful #706

Open
canweriotnow opened this issue Jun 21, 2015 · 9 comments
Open

7.2.3 is unRESTful #706

canweriotnow opened this issue Jun 21, 2015 · 9 comments
Labels

Comments

@canweriotnow
Copy link
Contributor

REST describes the retrieval of a resource from a URI. The use of query string parameters as a requirement in 7.2.3 violates the principles of REST by requiring a query string parameter for the retrieval of a single resource through the use of a query string when that resource could be retrieved by its identifier, i.e. https://lrs-provider.com/xapi/statements/some-statement-uuid instead of https://lrs-provider.com.xapi/statements?statementId=some-statement-uuid

Once again, as before, I direct the uninformed reader to R. Fielding's 2000 dissertation establishing REST, as well as the work done since in the arena, such as the JSON-API spec

@garemoko
Copy link
Contributor

Note: I believe this issue relates to GET Statements which is 7.2.3 in 1.0.2 (the current version at the time of writing).

This needs to be tagged as major as it's a breaking change.

My 2 cents: This change isn't worthwhile as the benefits (just theoretical purity from what I can see) outweigh the work required by adopters to make the change.

@akuckartz
Copy link

👍 for raising this issue.

@bscSCORM
Copy link
Contributor

re: @garemoko 's comment, we could provide an additional representation of the statement resource (rather than a replacement), in which case this wouldn't be a breaking change for clients. If we were to address this issue, that's certainly how I'd recommend doing it.

That said, it looks to me like the issue here is that this usage is not idiomatic REST. The key point about REST is the use of URLs to use as identifiers to find a representation of a resource. R. Fielding clearly talks about URLs being used as identifiers, not "the path component of a URL". If there is a particular part of this paper that supports the assertion that a resource identifier should not include a query string, I couldn't find it.

Of course, not being idiomatic, not looking right can cause developer confusion. Which is why an early draft of the spec did in fact use the syntax: https://lrs-provider.com/xapi/statements/some-statement-uuid . The feedback at that time from the group was that it was confusing to have statements identified only with the path portion of the URI but to have documents require the use of the query string (which was necessary since document IDs may be URIs, which are impractical to encode in the path portion of a URL). Based on that feedback, statement syntax was brought in line with document syntax. Around that time, the identifier for a single statement was collapsed into the syntax for a query in the documentation, since it looked confusing to have a resource .../statements?id=statementId that was a reference to a single statement and another resource .../statements that accepted query parameters, even though that's logically what is going on.

So, if we were to provide an alternate identifier for the statement resource that appears more restful we should do so with the understanding that we'll be back to having a statement identifier that is inconsistent with our document identifiers, and that we'll now have two ways to identify the same statement (unless we decide to break systems using the old identifier).

I don't really think it's worth doing, but if others think it's important and aren't bothered by the duplication and inconsistency I don't see any harm in it either.

@canweriotnow
Copy link
Contributor Author

@bscSCORM thanks for the historical background, I definitely lacked that.

I'm not sure I buy the distinction between "REST and idiomatic REST", but that's really a philosophical discussion. I do have the feeling that some LRS implementors are going to support the idomatic version whether it's required by the spec or not, and when we're looking at ease of adoption, the (imho) best REST client libs tend toward the idiomatic (e.g., Retrofit in Java), and make it much easier to request a singular resource by its URI rather than requiring the passing in of query params.

@garemoko I'm not sure this has to be "breaking" as it wouldn't remove existing functionality (I'm not suggesting we break the use of query params) but rather bring requirements into line with expected - sorry, idiomatic - behavior.

@canweriotnow
Copy link
Contributor Author

(which was necessary since document IDs may be URIs, which are impractical to encode in the path portion of a URL)

@bscSCORM You might think that, but JavaScript has this neat encodeURIComponent() function 😄

@bscSCORM
Copy link
Contributor

@canweriotnow I should have been more clear, it's possible to encode using encodeURIComponent, but Apache's default behavior is to helpfully pre-decode any encoded / , so by the time an application sees the URI there is no way to tell whether any / is part of an identifier or the path leading up to that identifier. It seemed impractical to use an encoding that Apache would mangle by default.

@canweriotnow
Copy link
Contributor Author

@bscSCORM Oh dear god, I haven't seen that... granted I've been using nginx almost exclusively since about 2011 or so, and only client-side routing for UI paths for the past couple of years, but I can see how that would be a problem...

The 'almost' was because I had to use mod_shib to support AD/Shibboleth some years ago and Apache was the only option... put it this way, I hate writing C and I was ecstatic when I figured out a way to hook straight into PAM for auth and got to murder that Apache monstrosity.

But yeah, that does make sense. I think I need to do another read-through and mentally decomplect "paths that could get sent to the LRS" and "paths that could get sent to a client application"

@canweriotnow
Copy link
Contributor Author

@akuckartz thanks, I try to help maintain theoretical purity wherever I can. I'm like theoretical-purity Batman except without the cave. Or the money. And my utility belt is basically the Unix shell. So not much like Batman at all. But lots of theoretical purity.

@canweriotnow
Copy link
Contributor Author

@bscSCORM -

helpfully pre-decode any encoded /

I just noticed the "helpfully" - I assume that was sarcasm? 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants