Contributing batch requisition endpoints to core
Sebastian Brudziński
Hello tech committee!
During the next couple of days, the Malawi team is planning to
work on contributing the batch requisition endpoints from our
service
(https://github.com/OpenLMIS-Malawi/mw-requisition-expansion) to
the core requisition service. We would therefore like to confirm
technical details for this change and a necessary refactor of the
error handling in requisitions.
The RAML has already been reviewed on May, during the first
attempt to contribute those, although some things have changed
since then, so I have uploaded it for review again. Please do take
a look:
https://github.com/OpenLMIS/openlmis-requisition/commit/8b31d522d46897fe55c020e32a7f4582bbdb733c
Specifically:
- minified DTOs are used in all of those endpoints and they
contain only the data necessary for the batch approval page.
Working with several requisitions, each having a few hundred
products made requests and responses really huge and so it proved
necessary to satisfy slow connections
- a new endpoint has been introduced to also retrieve
requisitions in batch (next to save & approve). It takes an
array of UUIDs and will ignore UUIDs that do not exist and UUIDs
of requisitions the user does not have access to
- partial success (minimum 1 requisition processed with
validation errors) returns error code 400
Since the batch endpoints need to always process all passed
requisitions, a refactor of error handling is necessary in the
requisition service. Currently, most validation checks, including
permission verification, rely on unchecked exceptions. Throwing an
unchecked exception, however, ceases the execution and would
therefore lead to the batch endpoints not processing all passed
requisitions upon a validation error. Catching unchecked exception
was not accepted as viable option by the technical committee and a
refactor of error handling was suggested.
As a part of this contribution we are therefore planning to
re-work the validation checks in the requisition service, such
that they do no longer immediately answer with an exception, but
rather return an instance of "ValidationResult" class. This new
class would store an array of error Message objects and would
contain some utility methods to retrieve the errors and to check
if the validation passed. It will be up to an endpoint then what
to do with that error. Current requisition endpoints will
translate the validation messages to exceptions, while the new
batch endpoints will store the errors and keep on processing all
requisitions.
Does all of this sound/look good to you? If you have any further
questions or concerns, please let me know as well.
Best regards,
Sebastian.
Sebastian
Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com
SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41
Josh Zamor
At a high level the work on validations, not immediately throwing an unchecked exception, etc all sound familiar and on the right course.
If I'm reading this RAML right, these two statements are true:
- Batch saving doesn't take in the request a minified Requisition
- Batch approve takes in a set of UUIDs (POSTed) and returns a minifed Requisition
I spent a few minutes tracking down some of these older conversations, so I'll link them here for reference:
- Technical Committee meeting on batch: https://openlmis.atlassian.net/wiki/x/rwq-Bg
- Dev forum post on batch (and 400 response to partial success): https://groups.google.com/forum/#!topic/openlmis-dev/day87LiGZro
Sebastian Brudziński
--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/34d2dc9d-0372-430b-8bbf-462389afac3f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Josh Zamor
Lets update the RAML, that seems like a simple step.
I think everything here works, though I'd like us to take a step back and look at these endpoints from the REST perspective. Overall I feel like we've been slipping in building a RESTful interface (especially when it comes to performance hacks and finding things), and when I look at the endpoints the various services provide, I'm growing concerned.
For Batch Requisitions, The RESTful representation for /requisitions/batch/save, /requisitions/batch/approve, etc just doesn't feel very RESTful. They look more like method calls. More-over when we say "batch" here, we seem to mean "batch approval" - not just batch. Overall I'm wondering if creating a new approval resource would help keep the boundary between "requisition" and "approval" clearer. After all the two modes are quite distinct: I can't change approval fields when it's not in the approval phase, nor can I change requisition fields when it is in the approval phase. Sure the screen needs to display most of these fields, but that doesn't mean that it's the same RESTful resource. Longer term could you see us splitting out approval as a separate (sub)resource?
There is an immediate improvement that could be made to these proposed endpoints, before that's done, based on this stack overflow answer:
- I don't see any reason for POST /api/requisitions/batch/approve to exist. In fact it highlights a problem with our current approach which looks more like method calls. We currently have POST /api/requisitions/{id}/approve, and /api/requisitions/{id}/authorize, and /api/requisitions/{id}/reject, etc. If we had instead supported POST /api/requisitions?markAsApproved&id=A&id=B&id=... (or have ids in POST body) it would have been easy to support the batch approval feature (it would already exist) and we'd have fewer verbs in our URI (meaning fewer endpoints that look like method calls).
I think it'd be worth a bit of inconsistency in the short term if we could adopt this for the batch approval need, rather than introduce another approve endpoint (/api/requisitions/batch/approve).
Sebastian Brudziński
Yes, I think treating the requisition ready for approval as a
separate (sub)resource makes sense from the system's perspective.
It could also simplify the validation checks that we currently
have in place and that make sure that only specific fields are
changed during approval. When treating them as separate resources
we could just remove all those checks, as no other fields than
those that can be edited, would be passed and accepted by the
endpoint.
As for your proposed endpoint design - would you like to follow the same approach for the remaining "batch approval" endpoints for now? Like "/api/requisitions?retrieve&id=1&id=2...". I don't have a problem with that, although I think it looks a little more messy than the originally proposed design (even if it wasn't very RESTful). I can go ahead and code them either way though.
Regards,
Sebastian.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/c3360e42-8e8f-4655-919e-5752a85a08ea%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Sebastian
Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com