adlnet / xAPI-Spec

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

Statement Resource Multiple Return Types #1030

Open joshrussell opened 7 years ago

joshrussell commented 7 years ago

We've been attempting to document our LRS implementation using Swagger. We quickly found that Swagger doesn't support multiple response types per code. This lead to the question of why a GET on the Statement Resource returns a single Statement object or a StatementResult object. This specification not only makes it (currently) impossible to document the endpoint with a tool like Swagger, it also seems like it makes both the LRS implementation code and the client code more complex in that the LRS has to determine whether it should send a single Statement or a StatementResult and the calling code has to determine whether it has received a single Statement or a StatementResult. I'm curious why the decision to unwrap the StatementResult if a statementId or voidedStatementId is passed instead of just always returning a StatementResult?

brianjmiller commented 7 years ago

FWIW: https://github.com/TryxAPI/xapi-swagger

brianjmiller commented 7 years ago

I suspect @bscSCORM is really the only one that can answer this, and some of that answer may just be the age of the spec and/or an arbitrary decision at the time. To improve this you'd have to add 2 resources which expands the overall face of the API. Ultimately this is subjective, and Swagger is known to be pretty opinionated.

joshrussell commented 7 years ago

That actually illustrates the issue I encountered perfectly @brianjmiller. Take a look at https://github.com/TryxAPI/xapi-swagger/blob/master/xapi-swagger.yaml#L669. Only one possible response is documented, not both.

joshrussell commented 7 years ago

Yes I agree that this is a very subjective topic. There's nothing technically wrong with the design of the spec as it exists. It just seems to me to make implementation (server and client) and documentation a little more complex than a single possible return type. Definitely interested in further discussion of the topic though.

garemoko commented 7 years ago

There's not a situation where the client has to figure out if what is received is a single statement or statement result object as this is determined by the parameters passed by the client. So the only complexity is on the server side and arguably adding an additional resource for single vs. multiple statement GET would be equally as complex.

joshrussell commented 7 years ago

I agree the behavior is deterministic so I suppose it comes down to the client side implementation as to whether or not you would type check. But since it's the same endpoint, I could reasonably see an encapsulation that takes parameters or not and then has to inspect the response to know what it got back.

I'm not sure I follow though on adding an additional resource. The endpoint is the same; I'm just suggesting that the Statement resource always return a StatementResult, whether the statements array contains 1 or 300. If I pass statementId or voidedStatementId, I get back a StatementResult with a single statement, the same as I would if I passed no parameters and there is only 1 statement in the database. That's all I'm suggesting, for what it's worth.

bscSCORM commented 7 years ago

@joshrussell a big 👍 to this from me. If I had it to do over again, the .../statements resource would always return a list, even if it was a list of one.

What probably happened here is that in early drafts of the spec, there was a /statements/{statementId} resource, which returned just a statement, not a StatementResult, as you'd expect. And in fact, I think it would be even better to just go back to that, rather than treat statementId as a filter. Now, this will seem unrelated but we also had ActivityId as a query string parameter to the "Activities" resource because ActivityId's are URIs and therefore can have a / character, and: https://github.com/adlnet/xAPI-Spec/issues/706#issuecomment-114196430. At the time, the group felt that sometimes fetching a sub-resource using a query string parameter and sometimes adding it to the path was confusing. In retrospect, I wish I had pushed back against that harder. At the time I thought something like: "that doesn't seem better, but it will work either way, so OK". So, things that should logically be their own resource (like particular statements) got collapsed back into a small set of top level resources. We should have changed the expected response at that time, but didn't.

That all said, I think we're just stuck with this until 2.0, because we will have clients expecting that single-statement result coming back. I suppose we could do a 1.1 that added the option for a client to pass a header requesting consistent response formats for each resource + content-type, in which case the LRS would have to use a StatementResult even when StatementId is specified. That could at least make it easier to automatically generate clients.

joshrussell commented 7 years ago

That's some good history @bscSCORM. Thanks for the reply. While a typical REST pattern would indeed have the statement id as part of the path, I can understand not doing that for conformity across endpoints as you describe. I actually have no quarrel with that decision in this case. Since my main argument is simplicity, I'm not sure implementing a consistent response header would be worth the effort. It would lead to even more complexity in the implementation. If the answer is we have to wait until 2.0 before we can discuss returning a StatementResult regardless of the query params, that is a valid response. I certainly understand not introducing major breaking changes within the same major version. But I definitely think the best future path is a consist return type for a given endpoint/method combination regardless of the parameters passed. Just my opinion of course. But like I said, I welcome any discussion on the topic.

creighton commented 4 years ago

@andyjohnson is this going to be discussed in the IEEE group?