REST principles in OLMIS

[mkwiatkowski]

Hi everyone,

I was reviewing our codebase in terms of RESTfull principals that are in (http://docs.openlmis.org/en/latest/conventions/codeStyleguide.html#restful-interface-design-documentation).

I've run into some parts that are still not RESTfull, problems that occured after refactoring our API or some RESTfull patterns still don't have implementation in our services.

#1 We still have a lot of /search endpoints with POST method (i.e. /api/orders/search, /api/geographicZones/search). Those should be merged with GET /api/{resource}/ endpoints.

#2 Requisition resource is still using actions like submit/approve/skip witch is against RESTfull principles. I cannot find a discussion on this topic in our dev forum but I'm pretty much sure that is was discussed already and I think that proposed solution was that those should be to incorporated into /api/requisitions/{id} PUT method. There are few possible problems with that solutions like PUT method in controller could have a lot of logic etc. but I assume most of that logic should be moved to Requisition class if possible.

#3 Parameters are parsed into map on searching endpoints instead of having a class with all legal parameters. Pawel Albecki already pointed that out, here is an example: https://review.openlmis.org/cru/FEOLMIS-2150#CFR-31234)

#4 Endpoint that are retrieving resources by list of ids (facilities and users) are omitting other search parameters. I don't think that it is a good pattern, it could introduce some confusion for implementers if part of valid parameters are not taken into consideration. This change would not require a lot of work probably. If we are going not to change that I would say we should at least describe it clearly in RAML.

#5 We don't have pattern for limiting fields in returned resource: (http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#limiting-fields) . Instead we have i.e. /api/facilities/minimal endpoint which is not the best practice.

#6 In UserController and RightController in ReferenceData service we have only one endpoint for updating/creating user which accepts PUT requests. Those should be separated.

#7 Swagger have problems with endpoints that can return or accept multiple types of data (i.e. /api/catalogItems can return application/json and text/csv content types). This is documented well in RAML file, but does not work in Swagger. We have tickets for that already.

#8 We don't have a expanded resource representation pattern implemented, but we have ticket for that already in progress.

#9 In Requisition service we have api/requisitions/forConvert /api/requisitions/submitted endpoints and retrieving those requisitions should probably be done using GET /api/requisition/ endpoint. Problem here is that we are also checking user rights for those requisitions 

#10 The one of the biggest problems is searching by extra data field. Now we are using POST /search endpoints for that, which probably needs to change. I don't have a good pattern for this that would not require some parsing on client and/or server side. Maybe doing similar thing to expanded pattern with dedicated parameter and coma separated 'key:value' inside could work here.

I will create a page with checklist that includes above problems and places where those occur (some of them are already in https://openlmis.atlassian.net/wiki/spaces/OP/pages/101580897/Technical+Debt+for+v3).

I would appreciate some feedback on those and if we want to create some tickets for them. Please feel free to pinpoint other problems with REST principals in OpenLMIS.

Regards,

Mateusz

[Nick Reid]

Innocent Question: How often are we actually checking extra data fields, and where?

I'm curious because cleaning that up might be over-engineering....

Nick Reid | nick...@villagereach.org
Software Developer, Information Systems Group

VillageReach Starting at the Last Mile
2900 Eastlake Ave. E, Suite 230, Seattle, WA 98102, USA
CELL: +1.510.410.0020
SKYPE: nickdotreid
www.villagereach.org

---

From: openlm...@googlegroups.com <openlm...@googlegroups.com> on behalf of mkwiatkowski <mkwiat...@soldevelo.com>
Sent: Thursday, November 23, 2017 8:13:56 AM
To: OpenLMIS Dev
Subject: [openlmis-dev] REST principles in OLMIS

 

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

--
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...@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/d64a74a8-27a2-40f3-8d75-416d1d0c656e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Josh Zamor]

Thanks Mateusz for digging into this.

Some quick thoughts by #:

1.  Search methods by POST are actually not that far from "best practice".  I like this Stack Overflow response.  Moreover I wouldn't want to see our POST search merged with our GET if that meant the issues that come with URL encoding complex search parameters.  I do have some other concerns:
1. Search resources returning a different representation than our GET - this level of inconsistency seems unneeded.
2. Search resources with odd mix and match of required parameters.  If this isn't apparent I can find an example.
2. Agreed.  In the past I've put forth the idea that a status change could be it's own sub-resource.  I think that's how we ended up with our requisition/{id}/submit, however I think we're passing around the Requisition resource, and not a true sub-resource (i.e. with an id).
3. Not that I disagree this is an issue, however it's not exactly an issue with our REST resources.  Sounds like an implementation issue in our Java controllers, right?
4. Yup, this is what I was thinking in regards to 1.2 (above)
5. Yes!  I'd like to move on this ASAP, and I hope that the work Sebastian is doing for our Expanded pattern will be re-usable/close enough.
   
6. This may not be an issue.  At least I don't put much stock in it.  I think the concern is the belief that a PUT should only be used to update, and a POST to create?  That's not the case, PUT may be used for both create and update.
7. I'm open to replacing Swagger UI with something more native to Swagger.  We only went with a conversion of RAML to Swagger UI as it was felt then that Swagger UI was nicer than something like API Console.  Perhaps it's time to re-evaluate if RAML native tools haven't caught up yet.
8. +1
9. Agreed, from a RESTful perspective these resources shouldn't exist.  I think this issue should be fully handled inside the service's implementation.  And for that we need someone able to come up with a good Java pattern.  Some advanced version of the Strategy pattern perhaps?
10. I'm not so sure this is an issue.  Again based on search endpoints being a pragmatic response for RESTful resources.

This is a good list.  I'm looking forward to the discussion tomorrow.

Best,
Josh

[Josh Zamor]

Typo in my #7.  I meant replace Swagger UI with a RAML native alternative, such as API Console (or a better one if anyone has one to suggest).

Also I realized I didn’t link the SO article for #1.  And of course now I can no longer find it.  I’ll look some more for it, but the basics for using POST to do a more RESTful query is this:

• A POST can be made to a resource for searching.  In our API we’ve ended up with /api/resource/search.  It’s a little odd, but I’d argue most people aren’t confused by this.
• The body of the POST has your search criteria (this would not be idempotent)
• The returned status is a 200 with a body that has an ID.  This ID is the ID of the search you just created.
• You’d then use a GET, with the ID just returned, to retrieve the results.  The results should be idempotent - i.e. stored somewhere forever to be retrieved exactly as retrieved the first time, until a DELETE is made. 

That’s the basics of turning a POST for a query to be more RESTful.  We’re not doing that as today our POST doesn’t create anything.  Personally I’d like to be more RESTful here, however the ROI seems low compared to some of the other things on this list.

Lastly, for #9 I forgot to link this post which I think has a good description of where we want to end up (we’ve started the move in this direction) for requisition status changes:  https://www.thoughtworks.com/insights/blog/rest-api-design-resource-modeling

-- 
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...@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/40a3a88d-e3e7-4554-a175-0f03bb6d1d12%40googlegroups.com.

[Mateusz Kwiatkowski]

Hi,

responding to Nicks question: we are only searching by extraData between backend services, there is no such example on UI, maybe in the future we will be searching by vvmStatus in stock? but as Josh said using /search endpoints it is not the best REST practice but it is acceptable so probably there is no reason to do this over-engineering just to make rid of those 2 endpoints.

Thanks Josh got your opinion on those bullet-points:

#1 and #10 Yes, I should write above that using /search endpoints is acceptable by REST as said here: http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#restful and introducing some kind of complex logic is not worth it probably. My only concern is that we will be having 2 endpoints doing the same search except searching by extraData (both should be using the same code placed in service probably)

#3 Yea, this is not problem with REST, just a thing that I've found going through /search endpoints

#6 I'm aware that PUT can be used for both update and create, but those 2 places are just inconsistent with other controllers

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 openlmis-dev@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/40a3a88d-e3e7-4554-a175-0f03bb6d1d12%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
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/F122633C-F02A-4E93-B030-62D60FEF7B27%40villagereach.org.

For more options, visit https://groups.google.com/d/optout.

[Paweł Albecki]

Something about search resources: https://stackoverflow.com/a/926793 (see comments too).

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAHq-FDPw0FsSUUjxnbpCZuS-PLA5mo2EL6egnsyuqJS0nKXNAw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

[Mateusz Kwiatkowski]

Hi everyone,

here is REST design issues page where we can keep track on those. List is ordered by priority so the things to be fixed first are on the top. I've created tickets for most of them.

Best Regards,

Mateusz

[Mateusz Kwiatkowski]

Hi everyone,

based on some comments made by Paweł and Łukasz I've added some issues/bullet points to REST design issues confluence page:

- our PUT /api/{resource}/{id} endpoints do not allow creating objects with specified 'id'

- we have /api/facilities/byBoundary endpoint,

- we are using /print endpoints

- search parameters in some places are confuisng i.e. we have 'facility' instead of 'facilityId' or 'facilityCode'

If you have something to add please place it on REST design issues

Regards,
Mateusz