Rest API plurial/singular

sebastienbeau commented 4 years ago

Hi all

context

In the Shopinvader API we have some endpoint in plurial and some in singular. I am going the initial reason of this choice and want to open a discussion about it. The aim it to have a clear guideline on this point.

why plurial and singular

The initial reason was to give the best experience to the web front developer. For this I try to stick to the locomotive/liquid tag and be compatible with the initial syntax.

For example if you want to show sale order you can do it with

{% paginate store.sales by 4 %}
    {% for sale in paginate.collection %}
       ...
    {% end_for %}
{% end_paginate %}

Paginate have a lot a great feature and a good API look at : https://doc.locomotivecms.com/docs/tags#properties-of-the-paginate-object

In the same way if you want to show the cart you can do

{{ store.cart }}

Later we have introduced the tag "ERP" example

{{ erp get "sale" as sale with {"per_page": 4, "page": 1} }}

This tag is more powerful (you can do post, get, put, delete...) but less integrated with standard syntax (you manage yourself the pagination for example)

Current stat of using store.something

store.addresses: 9 times
store.customer: 3 times
store.products: 17 times
store.cart: 23 times
store.quotations: 4 times
erp tag 1 time

So you have to keep in mind that in 99% of the case the frontend designer will use the simpler syntax "store.something" and not the erp tag

As I wanted to have a clean API, I thought that respecting plurial, singular is more explicit for the developer to understand the nature of the object manipuled

For example

store.cart => I have one record
store.sales => I have a list of sale

Having store.sale that return a list of sale will be weird for the front-end developer

Implementation

As I try to have less logic as possible on ruby size (only generic code). My first idea was to use plurial and singular in the REST API. So basically all service are in plurial and only some specific one are in singular like cart and customer

I see that we have some incoherence and sometime we have singular instead of plurial

delivery_carrier
delivery
invoice

Current issue

Using singular instead of plurial break "store.something", as we expect in the code a singleton see issue on template here : https://github.com/shopinvader/shopinvader-template/pull/34

Solution

I see four solution

always use singular and having a bad frontend naming ex: store.sale will return a list :'(
using singular and plurial correctly in the API (add Guideline to explain why)
always use singular in the API but add the logic on ruby side to map dynamically store.sales to sale API endpoint
using new syntax to handle paginate and scope for make simple call to odoo

What do you think?

@shopinvader/shopinvader-maintainers @lmignon @sbidoul @simahawk @thibaultrey @Cedric-Pigeon @rousseldenis

Note : We must keep a backward compatibility so if we change the API the syntax in locomotive, the old template should still work.

Note 2: we are thinking with Didier to add a mechanism in locomotive that track the usage of deprecated feature and show this information in the backend of locomotive. So user will be aware that they use deprecated feature and can migrate to newer syntax. Then want a site do not have anymore deprecated feature we know that we can remove deprecated feature.

Cedric-Pigeon commented 4 years ago

@sebastienbeau I totally agree with you and my favourite option would be to use singular and plurial correctly in the API. It will ensure consistency through all layers. We absolutely have to avoid introducing more stuff on Ruby side. Thanks for this reflexion

lmignon commented 4 years ago

I have to say that I'm quite annoyed to know that the way a service is named has an influence on the locomotive part. IMO, there is no reason to be forced to separate services providing access to collections on the one hand and to an instance of an object on the other. Personally I find that it makes the api more complex and I would prefer to have 1 service per concept handled (cart, delivery, ...) always named in the singular. IMO It is not acceptable that the locomotive syntax should impose rules on how to define our services, and I think it is important that this requirement should be dropped. I understand that pagination is something important, but can't we find another way to do it? (a method like ?????epr_call_with_paginate????) I think we need to remove as much of the implicit logic made by locomotive as possible. This is a barrier to flexibility and causes problems in many projects. I think about the difficulties encountered to add parameters from the template for product navigation, being unable to make a call for a specific instance of an object without the id being put into the session... To sum up, I personally find that the layer between locomotive and external systems is too often a barrier. For me it should play a role of pure proxy. That being said, nothing prevents us from paying attention to the naming of services. But once again, the distinction between singular and plural just to satisfy the locomotive part is not a valid argument for me and I would prefer to have a single entry point for the same concept.

Regards,

lmi

On Tue, May 5, 2020 at 7:16 AM Cédric Pigeon (ACSONE) < notifications@github.com> wrote:

@sebastienbeau https://github.com/sebastienbeau I totally agree with you and my favourite option would be to use singular and plurial correctly in the API. It will ensure consistency through all layers. We absolutely have to avoid introducing more stuff on Ruby side. Thanks for this reflexion

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/shopinvader/odoo-shopinvader/issues/626#issuecomment-623860703, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEE2WUDE57YGHVRSKSKA7DRP6OMTANCNFSM4MXGWU6A .

sebastienbeau commented 4 years ago

IMO It is not acceptable that the locomotive syntax should impose rules on how to define our services, and I think it is important that this requirement should be dropped.

I also agree that the way we use the API on locomotive should not impact the design of the API. The API must be clean and usable, understandable without locomotive.

I understand that pagination is something important, but can't we find another way to do it? (a method like ?????epr_call_with_paginate????)

The initial idea was to stick as much as possible near from locomotive syntax and make it compatible with paginate, scope syntax. So user was using the same syntax for getting sales (come from odoo), product (from elastic or algolia), or content entries (come from locomotive). But maybe we can provide an alternative syntax only for Odoo. I am going to think more about this point and see if we can find a syntax that make @thibaultrey happy.

In any case I am 100% agree with this

But once again, the distinction between singular and plural just to satisfy the locomotive part is not a valid argument for me and I would prefer to have a single entry point for the same concept.

So my first question is for an API, does it make sense to have plurial and singular. If not => we can already exclude some solution :)

sbidoul commented 4 years ago

I'm not sure I get all the subtleties here. So a few things that are clear to me:

always use singular and having a bad frontend naming ex: store.sale will return a list :'(

-1

using singular and plurial correctly in the API (add Guideline to explain why)

+1

Many REST API in the wild tend to use plural for everything. For instance it would make sense to me to see /invoices/{id} returning one or /invoices/ returning something that you can iterate. See for instance the https://api.github.com. Not sure if that creates problems in base_rest?

always use singular in the API but add the logic on ruby side to map dynamically store.sales to sale API endpoint

-10, too magical to my taste

using new syntax to handle paginate and scope for make simple call to odoo

Not sure why a new syntax would be needed? Could the template engine detect if the service being called supports iteration or pagination by introspecting the return type, or the schema? But I understand there is currently / would be code that works differently if there is a s at the end of a service name? That sounds fragile to me.

sebastienbeau commented 4 years ago

But I understand there is currently / would be code that works differently if there is a s at the end of a service name? That sounds fragile to me.

Yes currently we do not handle in the same way a call with a singular endpoint and plural endpoint. The code is here : https://github.com/shopinvader/locomotive-shopinvader/blob/v4.0.x/lib/shop_invader/liquid/drops/store.rb#L30

Basically if I do store.sales or store.cart

In the first case I expect to work on a normal API (with get, search, update...)
In the second case I expect to work on a "singleton API" that always work on one item (We have two singleton API cart and customer, all the other are normal API)

I do not use the same code use exactly the same code and test if is a plurial or singular is done with

        def is_plural?(value)
          value.singularize != value
        end

An other approach will to just introspect the result.

In anycase, let's focus on the first main point, and then come back to ruby part.

My initial idea was

always use plural in the API
only use singular for special API that work on singleton (cart and customer)

Does it make sense? If yes let's write guideline, if not let's go for full singular of full plural

sbidoul commented 4 years ago

My initial idea was
* always use plural in the API

* only use singular for special API that work on singleton (cart and customer)
Does it make sense? If yes let's write guideline, if not let's go for full singular of full plural

It makes sense to me.

lmignon commented 4 years ago

It makes sense but that means that the "deliveries" service must remain with "s" and we should not have other services. That also means that it would be possible to makes an HTTP GET with an ID into the path.... My concern is that we have some logic into the proxy that relies on the name of the called service.... lmi

On Fri, May 8, 2020 at 11:58 AM Stéphane Bidoul notifications@github.com wrote:

My initial idea was

always use plural in the API

only use singular for special API that work on singleton (cart and customer)

Does it make sense? If yes let's write guideline, if not let's go for full singular of full plural

It makes sense to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/shopinvader/odoo-shopinvader/issues/626#issuecomment-625739688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEE2WVKEC6KJI2BW7F33K3RQPJUDANCNFSM4MXGWU6A .

lmignon commented 4 years ago

TO be more specific on my previous response.

IMO:

the name of the called service should not be interpreted by the proxy
an analysis of the result could be used to support the pagination.
the list of singleton services should be limited to 'customer' and 'cart'
a guideline should explain that service name should use the plural form except for singleton service.

The last months I've improved a lot the api of base_rest https://github.com/OCA/rest-framework/pull/48. IMO, the proposed improvements will help to define a cleaner API and make the code cleaner. I can give you a presentation of the new concepts to see what you think about. We (@acsone) should be able to do some real integration tests on V13 for one of our new projects into the next days/weeks.

Regards,

lmi

On Fri, May 8, 2020 at 12:13 PM Mignon, Laurent laurent.mignon@acsone.eu wrote:

It makes sense but that means that the "deliveries" service must remain with "s" and we should not have other services. That also means that it would be possible to makes an HTTP GET with an ID into the path.... My concern is that we have some logic into the proxy that relies on the name of the called service.... lmi

On Fri, May 8, 2020 at 11:58 AM Stéphane Bidoul notifications@github.com wrote:

My initial idea was

always use plural in the API

only use singular for special API that work on singleton (cart and customer)

Does it make sense? If yes let's write guideline, if not let's go for full singular of full plural

It makes sense to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/shopinvader/odoo-shopinvader/issues/626#issuecomment-625739688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEE2WVKEC6KJI2BW7F33K3RQPJUDANCNFSM4MXGWU6A .

sebastienbeau commented 4 years ago

Ok so it's seem that now we share the same vision ! \o/

The first most important step is to

explain in the guideline that we should always use plural and only use singular for really rare special case (cart, customer)
fix the existing API to put plural
- invoice => invoices : https://github.com/shopinvader/odoo-shopinvader/blob/12.0/shopinvader/services/invoice.py#L21
- delivery => deliveries : https://github.com/shopinvader/odoo-shopinvader/blob/12.0/shopinvader_delivery_carrier/services/delivery_carrier.py#L13
- lead => leads : https://github.com/shopinvader/odoo-shopinvader/blob/12.0/shopinvader_lead/services/lead.py#L13
- I will review more if there is other case

Thanks for your constructive and smart feedback !

sebastienbeau commented 4 years ago

Regarding the fact of fixing the API, on v10 and v12 I propose to declare two service

one for the plural
one for the singular (that will inherit the first one) but deprecated in order to avoid breaking existing customer production.

simahawk commented 4 years ago

wow, I missed a very long thread :sweat_smile: FWIW I share the same remarks as Laurent and I agree on making it consistent w/ plurals :+1: Here you have another one https://github.com/shopinvader/odoo-shopinvader/blob/12.0/shopinvader_wishlist/services/wishlist.py#L15 ;)

sbidoul commented 3 years ago

Here are guidelines for designing REST API that I found quite good: https://www.gcloud.belgium.be/rest. It includes recommendations about Resource URI, versioning and backward compatibility, error handling and more.

github-actions[bot] commented 1 year ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

shopinvader / odoo-shopinvader