Hi all, I wanted to gather some opinions about the mentioned route. We've made sure all routes have the additionalProperties: false setting, and some routes, like this one, don't have it. In the task to add it we noticed: 1. Theroute doesn't work as expected: the documented attributes don't match the coding guidelines (i.e. terminology hasn't been adjusted on the spec) but it did in the code, it just works because we don't set additionalProperties: false. TL;DR no one following the spec can use it. 2. The route seems to be wrong regarding how our RESTful implementation works in the rest of the routes: PUT should get a Koha::Hold object for updating it. Instead, it only 'accepts': 'priority', 'pickup_library_id' and 'suspended_until'. If we wanted to allow to selectively change those attributes, we should do a PATCH instead. 3. We already have individual routes for changing those things on the hold: PUT /holds/:hold_id/pickup_location PUT /holds/:hold_id/priority POST /holds/:hold_id/suspension The route itself doesn't work now. And any change will break things for people 'using' it. And tests pass only because the spec is loose. The options are: - 'Fix' the route so the attributes match the code, i.e. change the spec as bug 20006 intended to do. - Leave it as-is (probably documenting it should not be used). - Remove the route until we really need to implement that PUT for something. - Make it a PATCH route, in which we could add new attributes without breaking anyone's dev (besides the verb change, that should be advertised) - Fully implement a PUT, making it mandatory to send all the attributes (this would be an important breaking change). I would vote removing it, or making it a PATCH route. But I still don't see the use case in the UI, for the latter. Most places in Koha do atomic updates for priority, pickup location or hold suspension for which we clearly have routes already. Looking forward to your comments. -- Tomás Cohen Arazi Theke Solutions (http://theke.io) ✆ +54 9351 3513384 GPG: B2F3C15F
I’m not sure that I have much to contribute. One comment I would have is… what is the threshold for creating a V2 of the REST API? And maybe a breaking change for the API is OK for a major version change in Koha but just not a minor version change? (That said, I’ve encountered breaking changes in PostgreSQL on minor version updates. They’re annoying, but they are a fact of life I suppose.) David Cook Software Engineer Prosentient Systems Suite 7.03 6a Glen St Milsons Point NSW 2061 Australia Office: 02 9212 0899 Online: 02 8005 0595 From: Koha-devel <koha-devel-bounces@lists.koha-community.org> On Behalf Of Tomas Cohen Arazi Sent: Tuesday, 18 May 2021 7:05 AM To: koha-devel <koha-devel@lists.koha-community.org> Subject: [Koha-devel] PUT /holds/:hold_id Hi all, I wanted to gather some opinions about the mentioned route. We've made sure all routes have the additionalProperties: false setting, and some routes, like this one, don't have it. In the task to add it we noticed: 1. Theroute doesn't work as expected: the documented attributes don't match the coding guidelines (i.e. terminology hasn't been adjusted on the spec) but it did in the code, it just works because we don't set additionalProperties: false. TL;DR no one following the spec can use it. 2. The route seems to be wrong regarding how our RESTful implementation works in the rest of the routes: PUT should get a Koha::Hold object for updating it. Instead, it only 'accepts': 'priority', 'pickup_library_id' and 'suspended_until'. If we wanted to allow to selectively change those attributes, we should do a PATCH instead. 3. We already have individual routes for changing those things on the hold: PUT /holds/:hold_id/pickup_location PUT /holds/:hold_id/priority POST /holds/:hold_id/suspension The route itself doesn't work now. And any change will break things for people 'using' it. And tests pass only because the spec is loose. The options are: - 'Fix' the route so the attributes match the code, i.e. change the spec as bug 20006 intended to do. - Leave it as-is (probably documenting it should not be used). - Remove the route until we really need to implement that PUT for something. - Make it a PATCH route, in which we could add new attributes without breaking anyone's dev (besides the verb change, that should be advertised) - Fully implement a PUT, making it mandatory to send all the attributes (this would be an important breaking change). I would vote removing it, or making it a PATCH route. But I still don't see the use case in the UI, for the latter. Most places in Koha do atomic updates for priority, pickup location or hold suspension for which we clearly have routes already. Looking forward to your comments. -- Tomás Cohen Arazi Theke Solutions (http://theke.io <http://theke.io/> ) ✆ +54 9351 3513384 GPG: B2F3C15F
Hi Thomas, David, I vote for making it a PATCH route and either document the PUT method as "do-not-use" or making it a real "PUT". In my opinion making the api a v2 is going to make things confusing for consumers as there are already several web-services available for Koha (ILS-DI, OAI-PMH, SRU, Rest...). For each API version, consumer app must develop a new connector and a mechanism to select which connector is appropriate to use, this is going to be quite confusing + added complexity for api consumers. Unnecessary complexity in my opinion. Arthur Suzuki, BibLibre Support Developper On 18/05/2021 05:31, dcook@prosentient.com.au wrote:
I’m not sure that I have much to contribute.
One comment I would have is… what is the threshold for creating a V2 of the REST API?
And maybe a breaking change for the API is OK for a major version change in Koha but just not a minor version change? (That said, I’ve encountered breaking changes in PostgreSQL on minor version updates. They’re annoying, but they are a fact of life I suppose.)
David Cook
Software Engineer
Prosentient Systems
Suite 7.03
6a Glen St
Milsons Point NSW 2061
Australia
Office: 02 9212 0899
Online: 02 8005 0595
*From:*Koha-devel <koha-devel-bounces@lists.koha-community.org> *On Behalf Of *Tomas Cohen Arazi *Sent:* Tuesday, 18 May 2021 7:05 AM *To:* koha-devel <koha-devel@lists.koha-community.org> *Subject:* [Koha-devel] PUT /holds/:hold_id
Hi all, I wanted to gather some opinions about the mentioned route.
We've made sure all routes have the additionalProperties: false setting, and some routes, like this one, don't have it. In the task to add it we noticed:
1. Theroute doesn't work as expected: the documented attributes don't match the coding guidelines (i.e. terminology hasn't been adjusted on the spec) but it did in the code, it just works because we don't set additionalProperties: false. TL;DR no one following the spec can use it. 2. The route seems to be wrong regarding how our RESTful implementation works in the rest of the routes: PUT should get a Koha::Hold object for updating it. Instead, it only 'accepts': 'priority', 'pickup_library_id' and 'suspended_until'. If we wanted to allow to selectively change those attributes, we should do a PATCH instead. 3. We already have individual routes for changing those things on the hold: PUT /holds/:hold_id/pickup_location PUT /holds/:hold_id/priority POST /holds/:hold_id/suspension
The route itself doesn't work now. And any change will break things for people 'using' it. And tests pass only because the spec is loose. The options are:
- 'Fix' the route so the attributes match the code, i.e. change the spec as bug 20006 intended to do.
- Leave it as-is (probably documenting it should not be used).
- Remove the route until we really need to implement that PUT for something.
- Make it a PATCH route, in which we could add new attributes without breaking anyone's dev (besides the verb change, that should be advertised)
- Fully implement a PUT, making it mandatory to send all the attributes (this would be an important breaking change).
I would vote removing it, or making it a PATCH route. But I still don't see the use case in the UI, for the latter. Most places in Koha do atomic updates for priority, pickup location or hold suspension for which we clearly have routes already.
Looking forward to your comments.
--
Tomás Cohen Arazi
Theke Solutions (http://theke.io <http://theke.io/>) ✆+54 9351 3513384 GPG: B2F3C15F
_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
Hi, Looks like bug 28369 is moving in the right direction. Whatever is eventually done, it's important to not just remove or change existing API functionality in the current API version. An API version should remain back-compatible. Functionality can be deprecated, but it should not be removed between Koha versions. I think Innovative has found an acceptable balance with their Sierra REST api versioning and deprecation policy. I'm not sure of all the details, but basicly they've added a new version whenever there's a backwards-incompatible change and kept the older API versions available for a few versions. Then the oldest one gets deprecated, and in the next major version or so it is removed. They also indicate which one is the recommended API version to target. A further note about the holds API endpoints: I can see the reasoning behind the different endpoints for suspension, priority etc. but from an API user's endpoint it's really convoluted to use. I'd really like a PATCH method that would allow one to update priority, suspension and it's end date and pickup location in a single request. --Ere Arthur kirjoitti 18.5.2021 klo 12.58:
Hi Thomas, David,
I vote for making it a PATCH route and either document the PUT method as "do-not-use" or making it a real "PUT".
In my opinion making the api a v2 is going to make things confusing for consumers as there are already several web-services available for Koha (ILS-DI, OAI-PMH, SRU, Rest...).
For each API version, consumer app must develop a new connector and a mechanism to select which connector is appropriate to use, this is going to be quite confusing + added complexity for api consumers.
Unnecessary complexity in my opinion.
Arthur Suzuki,
BibLibre Support Developper
On 18/05/2021 05:31, dcook@prosentient.com.au wrote:
I’m not sure that I have much to contribute.
One comment I would have is… what is the threshold for creating a V2 of the REST API?
And maybe a breaking change for the API is OK for a major version change in Koha but just not a minor version change? (That said, I’ve encountered breaking changes in PostgreSQL on minor version updates. They’re annoying, but they are a fact of life I suppose.)
David Cook
Software Engineer
Prosentient Systems
Suite 7.03
6a Glen St
Milsons Point NSW 2061
Australia
Office: 02 9212 0899
Online: 02 8005 0595
*From:*Koha-devel <koha-devel-bounces@lists.koha-community.org> *On Behalf Of *Tomas Cohen Arazi *Sent:* Tuesday, 18 May 2021 7:05 AM *To:* koha-devel <koha-devel@lists.koha-community.org> *Subject:* [Koha-devel] PUT /holds/:hold_id
Hi all, I wanted to gather some opinions about the mentioned route.
We've made sure all routes have the additionalProperties: false setting, and some routes, like this one, don't have it. In the task to add it we noticed:
1. Theroute doesn't work as expected: the documented attributes don't match the coding guidelines (i.e. terminology hasn't been adjusted on the spec) but it did in the code, it just works because we don't set additionalProperties: false. TL;DR no one following the spec can use it. 2. The route seems to be wrong regarding how our RESTful implementation works in the rest of the routes: PUT should get a Koha::Hold object for updating it. Instead, it only 'accepts': 'priority', 'pickup_library_id' and 'suspended_until'. If we wanted to allow to selectively change those attributes, we should do a PATCH instead. 3. We already have individual routes for changing those things on the hold: PUT /holds/:hold_id/pickup_location PUT /holds/:hold_id/priority POST /holds/:hold_id/suspension
The route itself doesn't work now. And any change will break things for people 'using' it. And tests pass only because the spec is loose. The options are:
- 'Fix' the route so the attributes match the code, i.e. change the spec as bug 20006 intended to do.
- Leave it as-is (probably documenting it should not be used).
- Remove the route until we really need to implement that PUT for something.
- Make it a PATCH route, in which we could add new attributes without breaking anyone's dev (besides the verb change, that should be advertised)
- Fully implement a PUT, making it mandatory to send all the attributes (this would be an important breaking change).
I would vote removing it, or making it a PATCH route. But I still don't see the use case in the UI, for the latter. Most places in Koha do atomic updates for priority, pickup location or hold suspension for which we clearly have routes already.
Looking forward to your comments.
--
Tomás Cohen Arazi
Theke Solutions (http://theke.io <http://theke.io/>) ✆+54 9351 3513384 GPG: B2F3C15F
_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website :https://www.koha-community.org/ git :https://git.koha-community.org/ bugs :https://bugs.koha-community.org/
_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
-- Ere Maijala Kansalliskirjasto / The National Library of Finland
participants (4)
-
Arthur -
dcook＠prosentient.com.au -
Ere Maijala -
Tomas Cohen Arazi