informatics-isi-edu / hatrac

Simple object storage for collaborations
Apache License 2.0
3 stars 1 forks source link

Inconsistencies in implementation and api docs for arrays of resource names #41

Closed chiragsanghvi closed 7 years ago

chiragsanghvi commented 7 years ago

Revised summary

The service response with JSON arrays of object names is inconsistent as far as whether or not the service prefix (e.g. /hatrac) is present on each name, and this behavior differs from the REST-API documentation in this repo. We suspect that there are programmatic clients working with these implementation quirks now, so we need to think about the right fix and transition strategy.

Original issue description

The chunked job retrieval for a URL mentioned here returns a list of paths, but they don't have SERVER_NAME i.e. hatrac prepended to it.

REQUEST

https://dev.isrd.isi.edu/hatrac/5678/17255efa2361c4d3799ad5937909fa8a;upload

RESPONSE

["/5678/17255efa2361c4d3799ad5937909fa8a;upload/PFQ2HYG5MXG356W6ZLK36NGQCE"]

The actual response should have hatrac prepended

["/hatrac/5678/17255efa2361c4d3799ad5937909fa8a;upload/PFQ2HYG5MXG356W6ZLK36NGQCE"]
karlcz commented 7 years ago

Hmm, this is actually behaving consistently with the namespace listing API and behaving as documented in the REST-API docs. Both return lists of hatrac resource paths as a JSON array and the listed paths are always paths missing the /hatrac prefix. It seems strange to change this for one API and not both, but I suspect we'll have a backwards compatibility problem with existing clients if we change the namespace listing response.

Would you be able to negotiate and consume text/uri-list content type if I added that as an alternative response format? This could return proper URIs like /hatrac/5678/17255efa2361c4d3799ad5937909fa8a;upload/PFQ2HYG5MXG356W6ZLK36NGQCE while leaving the JSON response unchanged for legacy clients...

I had intended to support this content negotiation but never got around to it since nobody seemed to need it. You'd just need to send an Accept: text/uri-list header in your request.

karlcz commented 7 years ago

The new text/uri-list support is now pushed to master. It is available for both namespace and job listings but they still default to the existing JSON response format if the client does not explicitly request text/uri-list.

karlcz commented 7 years ago

@hongsudt I am not sure who should review this...

chiragsanghvi commented 7 years ago

@karlcz I am ok sending the header Accept: text/uri-list

karlcz commented 7 years ago

It turns out the service implementation is more inconsistent than I realized! The returned JSON result for namespace listings and object version listings actually includes the service prefix, contradicting the api docs.

I'd like to make the following cases all consistent:

Content-Type Namespace Listing Version Listing Upload Job Listing
json ["/hatrac/N/C1", ...] ["/hatrac/N/O:V1" ...] ["/hatrac/N/O;upload/J1" ...]
uri-list /hatrac/N/C1 ... /hatrac/N/Oj:V1 ... /hatrac/N/O;upload/J1 ...

This will mean changing the JSON response for upload jobs to include the service prefix and to update the API docs. For programmatic clients, the best way to implement a transition might be to start negotiating the text/uri-list which should always be correct (if available) and tolerating a JSON response to this request meaning that the server is running older code that hasn't been patched yet.

@mikedarcy @svoinea @robes @hongsudt

karlcz commented 7 years ago

The final round of code and doc fixes has been pushed to master. All of the URI lists now include the service prefix both in application/json and text/uri-list responses.

karlcz commented 7 years ago

Maybe @chiragsanghvi @mikedarcy and @svoinea can test their respective hatrac client codes against this version to ensure interop as a review task?

chiragsanghvi commented 7 years ago

@karlcz This works atleast for get upload job listings.

svoinea commented 7 years ago

I have tested on rbk-dev the Namespace Listing for the json content type, and it works as described above.