Design/Implementation Comments

[fenekku]

I went through and wrote (or tried to) a dummy version of our Deposit API with what is on master. I know there are issues that tackle some of the elements I raise here, but probably not for all of them. If these issues are solved in pending PRs great!

Endpoints implemented:

• post/get/put draft,
• publish draft,
• get published records

I have NOT tried the 2nd use case: be an application developer that configures the created API. Nevertheless I listed some issues related to that.

• Resource().create and other methods are missing *args, **kwargs and/or other parameter (see later). This was mentioned by Lars.

• As an API creator I need to provide a Config separately --> little strange --> little confusing bc now there will be the library's Config class and the application's Config class --> For the Deposit API, with 5 endpoints, I had to create 6 classes. With 2 classes per action endpoint...

• (2nd use case) To be able to configure the Deposit API by creating another Config class, Resource should check somewhere for my configured class TODO: Resource should check global config to override the used ResourceConfig This is the hook to let others configure this API, right?

• create() method must return a serializable object : must be indicated

• Would be nice if create() method defaulted to 201 status code (nice-to-have)

• We should just follow the body,status,headers return of Flask view if my memory serves. I need to double-check that.

• Naming: For post, I need to implement "create" method... why not have to implement 'post_item', get_list... methods? i.e. verbs that match the HTTP verbs with _item or _list appended

• deserialization is magic from the API creation point of view:

  • "Where do parameters to create/read... come from?" I ask myself as a developer
  • adding <id> in the route and in the read(self, id) method just works... until it doesn't
  • id is a built-in python method that we should not override
  • With item_route = "/records/<id>/draft", def update(self, id, data) breaks (should be reverse)
  • With item_route = "/records/<data>/draft", def update(self, id, data) breaks because of name clash

• default to json is a little bit magic

• DepositResource is created once only as opposed to the MethodView approach. Little wrinkle there.

• if an HTTP verb corresponding method (read, create....) is not defined, accessing the endpoint should return 405. It doesn't do that for update for instance, because load_request is attempted first which may result in 400 even if the method is not defined at all. try-except must be done twice and checked to solve this. We can't rely on pre-running the method because it may have side-effects.

• Error order should be 404 if no route at all (first), then 405 if route but no method, then 400 if bad request. I think in this precedence order.

• Being able to attach actions to same resource would be much better UX than what we have. As mentioned we need 2 classes per 1 action endpoint.

  • thinking about using decorators but it might get messy too:

@action(route="", parser="...")
def actionA(self, *Args, **kwargs)
    pass

• having to define init to pass configuration is heavy

  class DepositActionResource(SingletonResource):
  default_config = ResourceConfig

  seems lighter

• list_route for SingletonResource is confusing and in fact an item-level action such as publish is not possible right now

• Why is CollectionResource different than Resource? Could we combine both into 1? Unimplemented route should return appropriate HTTP code anyway

• collections.py L40 search is missing request_context -> goes back to the discussion about deserialization

• if the deserializer for an Accept MIMEtype is not defined, we get data of value None rather than a 415 HttpException

• decorators on MethodView apply to all view methods, so we don't want that if possible (and it is possible: flask-restful does it)

• We talked about pagination, error cases, Etags and even sign posting when it comes to building the Response. Rate limiting is another scenario not to forget.

these are the tests I've been playing with: https://github.com/inveniosoftware/flask-resources/pull/27

[ppanero]

Answering in-line... Mostly with issues or PR links. Also shortening the question/point made, the full one is in the issue description anyway.

• Resource().create and other methods are missing *args, **kwargs and/or other parameter (see later). This was mentioned by Lars.

Fixed in https://github.com/inveniosoftware/flask-resources/pull/26

• As an API creator I need to provide a Config separately

Opened https://github.com/inveniosoftware/flask-resources/issues/33

• create() method must return a serializable object : must be indicated

Since this only affects "Deposit/Draft", "Record" and "Tombstone" objects (may be others), which are Invenio specific the SerialiableMixin was moved to invenio-resources. Opened issue https://github.com/inveniosoftware/invenio-resources/issues/9 In addition, the SerialiableMixin is not implemented by any default class of Flask-Resources nor (needed), which led to the thought that it did not belong there.

• Would be nice if create() method defaulted to 201 status code (nice-to-have)

Opened https://github.com/inveniosoftware/flask-resources/issues/34

• We should just follow the body,status,headers return of Flask view if my memory serves. I need to double-check that.

This is already the case. See here. Or do you mean do a simple return and let the view handle the make_response?

• Naming: For post, I need to implement "create" method... why not have to implement 'post_item', get_list... methods?...

I sort of disagree on this point, because it breaks the whole point of having a Resource. If we are gonna have duplicated names, it is not worth to do the implementation of the ResourceView... IMHO. Let's put it in the agenda for next discussions.

• deserialization is magic from the API creation point of view:

Yes. Namings are not the best. There is also object in some places which is a reserved word too. Needs refactoring. Opened https://github.com/inveniosoftware/flask-resources/issues/35

• DepositResource is created once only as opposed to the MethodView approach. Little wrinkle there.

If I understand this correctly, it is actually what we want to achieve. To have it centralized, independently of how it is represented (CollectionView, SingletonView, etc.)

• if an HTTP verb corresponding method (read, create....) is not defined...

Opened https://github.com/inveniosoftware/flask-resources/issues/36

• Error order should be 404 if no route at all (first), then 405 if route but no method, ...

Opened https://github.com/inveniosoftware/flask-resources/issues/36

• Being able to attach actions to same resource would be much better UX...

Opened https://github.com/inveniosoftware/flask-resources/issues/37

• having to define init to pass configuration is heavy

Related to: https://github.com/inveniosoftware/flask-resources/issues/33

• list_route for SingletonResource is confusing and in fact an item-level action such as publish is not possible right now

• Why is CollectionResource different than Resource? ...

IMO, Resource != CollectionResource != SingletonResource. They are different in representation. Someone might want an item representation without the list view part.

• collections.py L40 search is missing request_context -> goes back to the discussion about deserialization

Fixed in https://github.com/inveniosoftware/flask-resources/pull/31

• if the deserializer for an Accept MIMEtype is not defined, we get data of value None rather than a 415 HttpException

This is because right now content-type is hardcoded... https://github.com/inveniosoftware/flask-resources/blob/master/flask_resources/content_negotiation.py#L93 Should be treated at https://github.com/inveniosoftware/flask-resources/issues/14

• decorators on MethodView apply to all view methods, so we don't want that if possible (and it is possible: flask-restful does it)

Don't we? Some decorators are applied to every single one of the methods... It is true that there are some issues with the current approach (see more in comment in https://github.com/inveniosoftware/flask-resources/pull/26)

• We talked about pagination, error cases, Etags and even sign posting when it comes to building the Response. Rate limiting is another scenario not to forget.

True. Although most of this will come into invenio-resources

[fenekku]

Most of these have been addressed. New issues will be opened for any outstanding ones.