-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve HTTP API #370
Improve HTTP API #370
Conversation
4850aa7
to
9105797
Compare
2cd181a
to
41ed013
Compare
I think that you've accidentally checked in |
db0dc01
to
f6f8a2f
Compare
Thanks @sevein: removed those |
cffa4d5
to
ae871b7
Compare
ae871b7
to
2c4f950
Compare
Hi @jrwdunham. We have spent some time going through this and our overall impression is positive. We like the approach and were able to understand how you had done things and why. Using clientbuilder as a stopgap makes sense. Hopefully swagger support isn’t too far away. We noticed this: https://github.com/openapitools/openapi-generator. Our understanding is a bit abstract at this stage - we’ve only eyeballed the code. We will be able to give more detailed feedback after using it. |
path_dict[http_method][attr] = val | ||
paths[path] = path_dict | ||
|
||
def _get_search_request_body_examples(self, resource_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specific to the Archivematica Storage Service so it should be removed or moved outside of Remple. Futhermore, it seems that it is not even being used.
rsrc_collection_name): | ||
for action in ('search_post', 'new_search'): | ||
http_method = {'search_post': 'post', 'new_search': 'get'}.get( | ||
action, 'search') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document or remove the implicit usage here of the non-standard HTTP method SEARCH
.
path_dict[http_method]['requestBody'] = request_body | ||
path_dict[http_method]['responses'] = getattr( | ||
self, responses_meth)(resource_name) | ||
path_dict[http_method]['tags'] = [rsrc_collection_name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to remove some of the duplication here with _set_crud_paths
.
""" | ||
return [ | ||
OrderedDict([ | ||
('url', self.get_dflt_server_path()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: should we be able to use info in django_settings
to specify a true URL here instead of just a path?
schema is constructed by introspecting the Django model, while the | ||
create and update schemata are constructed by introspecting the relevant | ||
Formencode schemata attached as class attributes on the resource class. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: it seems that OpenAPI's readOnly and writeOnly boolean property attributes could be used here. See https://swagger.io/docs/specification/data-models/data-types/, in particular the Read-Only and Write-Only Properties
section.
input-named resource. | ||
|
||
TODO: should every parameter in an update request be optional, given | ||
that the resource being updated is presumably valid? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This comment is misleading. Update via Remple has the semantics of PUT
not PATCH
. A full representation of the post-update state must be sent in the update request.
"""Return a read schema for the resource named ``resource_name``. | ||
|
||
The read schema describes what is returned by the server as a | ||
representation of an instance of the input-named resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring should indicate that this is done by inspecting the Django model.
non_empty_ptr = [p for p in parts_to_right if int(p)] | ||
if non_empty_ptr: | ||
new_parts.append(part) | ||
return 'v{}'.format('_'.join(new_parts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is only needed if we want to support semver-compliant API versions, which may not be a good idea.
return getattr(field, 'default', NOT_PROVIDED) | ||
|
||
|
||
def get_required(field, default): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix function name since it returns a boolean indicating whether a field is required and whether it is null(able).
This is a huge step forward in making the Storage Service API consistent, predictable and documented. Thank you! I haven't had a chance to spin this up locally yet, but I'm planning to do that in the next week or so and may have more specific feedback after that. I am not a real developer, and have a very rudimentary knowledge of the Archivematica codebase, however from my perspective I'm not sure it makes sense to implement a custom REST framework. It seems like maintenance for Archivematica is already challenging enough that alleviating some of that burden would outweigh the benefits of OpenAPI 3.0 compliance, especially since as you note the tooling isn't really there yet. But perhaps I don't fully understand the benefits of OpenAPI 2.x versus 3.0. |
Connected to #369
Summary
GET locations/{pk}/browse/
using Python data structures.This PR adds a new set of versioned API endpoints (under the
beta
namespace) which address #369 by being:/api/beta/
—and a Swagger UI interface—see/api/beta/doc/
—to same;/api/beta/client/
—which is generated by clientbuilder.py; andHow to Understand OpenAPI 3.0
OpenAPI suffers from too much documentation. I have found this one to be good:
How to Understand this PR
resources
dict that tells Remple which resources with which endpoints to expose.browse
resources
config dict and can return a list of Djangourl()
instances that can beinclude
d inurlpatterns
.Questions