yetanalytics / lrs

Protocols, specifications, and logic for building an xAPI Learning Record Store (LRS) in Clojure(Script).
https://www.yetanalytics.com/lrs
Apache License 2.0
4 stars 1 forks source link

LRS-68 Handle Async Multipart Errors #78

Closed milt closed 2 years ago

milt commented 2 years ago

LRS-68 Currently the async multipart body build fn simply omits nil attachments. It should instead make the multipart result invalid, since the status cannot be changed. It is also currently utilized for sync responses, which is extra overhead.

Separating the sync and async approaches to building multipart responses (and testing them for parity) will let us have different behaviors WRT errors in mid-multipart on async.

After more research, the universal (sync + async) use of a channel body is probably more desirable as it allows even sync implementations to return things like File for attachment content which are passed for direct handling by pedestal.

This leaves the problem with the current impl that it simply omits attachments when errors occur, rather than terminating the response (and thus returning an invalid multipart), which is the only indication a client will get that the response is invalid.

This PR modifies the two async body formation functions (json and multipart) to accept a ::lrsp/async-error keyword in lieu of a statement, attachment or header. Upon receiving it they will immediately close the stream.

In practice this has no effect on sync impls like lrsql where an eager result is provided.

See the multipart body tests for examples of early termination: https://github.com/yetanalytics/lrs/blob/bd55295491e33523dd0c83c5c86cedf6389c3025/src/test/com/yetanalytics/lrs/pedestal/interceptor/xapi/statements/attachment/response_test.cljc

kelvinqian00 commented 2 years ago

Also don't forget to describe this revised behavior in the documentation. Probably advice best suited for the application level (since that's where the user docs are written), but since this will now apply to all apps that use this lrs lib it might be a good idea to add a section in the README for "things to note" like this.