+100 for not over-engineering. However either with the multipart or the Base64 approach, I believe it requires changes in core and of course in REST due to the nature of POST (using reflection instead of static setters).
Side note: this is something we are going to need at PIH shortly as well. I need to catch up on this thread, but I tagged RESTWS-297 as something we could potentially work on during the OMRS16 hackathon.
Take care, Mark
Would this break the following scenario?
- A complex obs is saved with another handler than
BinaryDataHandler
(outside of the REST API for example.) - The complex obs is then updated using the REST API to update
- Something unrelated to the complex data (such as the
comment
member.) - The complex data itself. Would
ObsResource
then understand that it needs to keep using the original handler and notBinaryDataHandler
?
Not all Obs use a binary data handler, so we can’t use a single handler to accomplish this, also note that Obs are immutable, it would be wrong to have to update them just to set the complex data since it would happen in a separate http resuest, one should be able to post an Obs along with its complex data in one rest call to avoid ‘updating’ it which otherwise would result in a void and create new operation, for this reason we need to update the core api for it to be possible
Thanks for raising this again @raff!
If I’m understanding you right, this is a no-go. Concepts are free to declare any type of handler, and the REST API needs to support all of them.
I would think we must support a POST of json-encoded binary data (since this is the “easiest” for a REST API client, right?), and we could also support multipart/form-data as a nice-to-have.
But the job of taking these posts, extracting the data, and streaming it to into the underlying Java API should be solvable purely in the REST module without needing to change the Java interfaces, right?
Also, creating or updating an obs with complex data should be doable as a single REST call that works atomically.
@darius or anyone who used handlers, would you care to explain what is the reasoning behind having different handlers in openmrs-api? Why do we need them? I don’t quite understand the concept behind them…
Here’s the only place where I found the usage of ImageHandler:
As it occurs to me in this example the only purpose of the handler is to indicate that it is an image. The methods implemented by the handler doesn’t seem to be used at all…
Anyway, I think I know how to handle it now and can do a first stub at implementing it. I don’t see a need to change openmrs-api at the moment to address the issue.
I think posting a file as multipart/form-data is easier at least from the javascript browser client. We can put json for obs metadata in the same request as multipart/form-data in the “obs” field to make the operation atomic.
Adding support for base64 json-encoded binary data should be easy as well, though it should be used for smaller files only as it increases bandwidth usage by ~30%.
Here is a simple example from VDUI.
When we save an image, although only one file is provided to the backend, we generate one more out of it as a compressed thumbnail file that is saved alongside the original image.
Displaying either the original image (for full screen view) or the thumbnail happens through using the ad-hoc view. Our custom handler knows how to deal with those views when getting the image. More importantly our custom handler is needed to generate the thumbnail in the first place.
But we needed another handler for cases where we don’t generate any thumbnails, the non-image cases.
You may also think of cases where one uses handlers to do something else than saving to disk or fetching from disk. Imagine a handler that posts to a third-party service and fetches from it.
@raff, you’re not wrong to say that the ComplexObsHandler interface is overengineered.
The key historical point is that Burke designed it long long ago (before 17 Aug 2010, when we switched from Ant to Maven). The intention was that intelligent handlers for different datatypes in the API could support multiple views in different UIs.
(It holds up pretty well for having been written >6 years ago.)
@raff, I cleaned up the Complex Obs Handler Technical Overview page. Hopefully, it’s a little clearer now.
My fix is at https://github.com/openmrs/openmrs-module-webservices.rest/commit/c01a314b14751f71eb0495b2d36e241e0c8d1845
It works for all handlers, which accept and set InputStream in ComplexData.data (at least all in openmrs-api do) when requesting RAW_VIEW.
Please see 2 tests presenting possible POSTs. One is for embedding base64 encoded data in json and the other is for uploading a file. You always fetch a file by requesting /obs/{uuid}/value.
Thanks @raff! We have a contractor working on some upload functionality that will will need this in the near future, so we will let you know how it goes!
Take care, Mark
Great @raff, I like a lot that you’ve gone through the two possibilities: (1) either everything embedded in the JSON or (2) any byte array added as a POST request file alongside the JSON.
In the latter, could you explain how this new upload(..)
method is triggered even though you simply save the complex obs through the usual URI?
Is it Spring that knows that if the input is a MultipartFile
then it has got to be the only possible route since that’s the only method having this type as an argument?
@mksd, its handled here: https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/controller/MainResourceController.java#L98 and it’s routed based on the “Content-Type=multipart/form-data” header.
I see, thanks for the pointer!
@raff, any chance to have this back ported to a (yet-to-be-created) 2.12.x branch? So on a next iteration 2.12.1-SNAPSHOT?
Was there a backwards-incompatible change sometime after 2.12 that means you can’t upgrade past 2.12 @mksd?
Mark
Actually I just realised that REST WS depends only on Core (I hope I’m correct here?), so upgrading could just work indeed.
@mksd, yes, you just need to upgrade RESTWS. The code is not released yet so you need to use 2.17-SNAPSHOT. Looking forward to your feedback.
Hey @mogoodrich (and @mseaton)
Have you ever had a chance to try out VDUI 1.1 then? This one is Core 1.10.2+ compatible and we would be really curious to know if PIH would find use in it, or what would be missing to it to make it more useable. Your feedback would be highly appreciated and valued.
I also intend to use @raff’s Uploadable
's pattern on the VisitDocumentResource
to get it closer to being 100% REST compliant.