dsp-api

dsp-api copied to clipboard

Currently, there are a few update routes in the Admin-API like changeProject which expect a JSON object in the body of a certain structure and allow to change more than one property at a time. I'm not very happy about how I implemented it. It is a bit obscure, in the sense, that it is not immediately apparent from just looking at the definition of the payload what is needed or what is going to happen. There is too much logic in the code.

For example, the changeProject route expects a ChangeProjectApiRequestADM payload in the body:

https://github.com/dasch-swiss/knora-api/blob/9b1bbe9349eb5cc1d247198d0b298d1853ae8617/webapi/src/main/scala/org/knora/webapi/messages/admin/responder/projectsmessages/ProjectsMessagesADM.scala#L78-L84

The route needs to handle a few use-cases:

1. Only update some of the properties (hence all are optional)
2. Although all are optional, it could be the case that some are still required.
3. Allow deleting a property, i.e., an empty list, in the case of description, means delete what is there. Not sending the property means leave it be.

To entangle these cases, I would propose to generally add a route per property. Then it should be fairly clear, what a route does.

Staying with the example of the changeProject route, it would be broken down into seven routes:

PUT admin/projects/IRI/Shortname
PUT admin/projects/IRI/Longname
PUT admin/projects/IRI/Description
PUT admin/projects/IRI/Keywords
PUT admin/projects/IRI/Logo
PUT admin/projects/IRI/Status
PUT admin/projects/IRI/Selfjoin

The payload in the first route would be something like:

PAYLOAD: {"shortname": "thenewshortname"}

The capitalized property name is just a recommended convention that I read somewhere.

This would also simplify the client library since it would not need to replicate the somewhat obscure logic implemented in the Admin-API.

On the client-side (GUI), changes to a form where multiple values are allowed to be changed could then be serialized into several requests.

@benjamingeer @tobiasschweizer @flavens @kilchenmann Any suggestions or objections?

I think thats a good proposal which makes things a lot clearer! I would adapt it ASAP in knora-py..

Sent from my iPad

On 10 Nov 2019, at 12:50, Ivan Subotic [email protected] wrote:



Currently, there are a few update routes in the Admin-API like changeProject which expect a JSON object in the body of a certain structure and allow to change more than one property at a time. I'm not very happy about how I implemented it. It is a bit obscure, in the sense, that it is not immediately apparent from just looking at the definition of the payload what is needed or what is going to happen. There is too much logic in the code.

For example, the changeProject route expects a ChangeProjectApiRequestADM payload in the body:

https://github.com/dasch-swiss/knora-api/blob/9b1bbe9349eb5cc1d247198d0b298d1853ae8617/webapi/src/main/scala/org/knora/webapi/messages/admin/responder/projectsmessages/ProjectsMessagesADM.scala#L78-L84

The route needs to handle a few use-cases:

1. Only update some of the properties (hence all are optional)
2. Although all are optional, it could be the case that some are still required.
3. Allow deleting a property, i.e., an empty list, in the case of description, means delete what is there. Not sending the property means leave it be.

To entangle these cases, I would propose to generally add a route per property. Then it should be fairly clear, what a route does.

Staying with the example of the changeProject route, it would be broken down into seven routes:

PUT admin/projects/IRI/Shortname PUT admin/projects/IRI/Longname PUT admin/projects/IRI/Description PUT admin/projects/IRI/Keywords PUT admin/projects/IRI/Logo PUT admin/projects/IRI/Status PUT admin/projects/IRI/Selfjoin

The payload in the first route would be something like:

PAYLOAD: {"shortname": "thenewshortname"}

The capitalized property name is just a recommended convention that I read somewhere.

This would also simplify the client library since it would not need to replicate the somewhat obscure logic implemented in the Admin-API.

On the client-side (GUI), changes to a form where multiple values are allowed to be changed could then be serialized into several requests.

@benjamingeerhttps://github.com/benjamingeer @tobiasschweizerhttps://github.com/tobiasschweizer @flavenshttps://github.com/flavens @kilchenmannhttps://github.com/kilchenmann Any suggestions or objections?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/dasch-swiss/knora-api/issues/1510?email_source=notifications&email_token=ABJX3TA3AH66QVXL24NRXU3QS7YQNA5CNFSM4JLMBBW2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HYHSVAQ, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABJX3THUFIYGHQPFL4NDTBLQS7YQNANCNFSM4JLMBBWQ.

Sounds good to me!

On the client-side (GUI), changes to a form where multiple values are allowed to be changed could then be serialized into several requests.

But then we have to know which values have changed. Should we do it by comparing the old with the new values before sending multiple requests? It will generate more traffic? If it's just for the project, then okay. But to be consistent, the user route and the other admin routes would also have to be modified. I'm not sure now with this solution.

Yes, we can change the existing project form, that a admin can change only one value at once. But it's not user-friendly... hm, yes it depends. I'll think about it.

@subotic Your proposal is good but what if the user wants to change multiple properties at once, like changing labels+comments+name of a list, or two of them? I would suggest giving all possibilities, that means:

1. a route to update the list with a payload where multiple optional properties are given.
2. separate routes to update properties individually as you suggested. For the lists, they would be

• PUT admin/lists/<listIRI>/label which accepts payload that contains

  {"labels": [{ "value": "a new label", "language": "en"}]}`
  

• PUT admin/lists/<listIRI>/name

• PUT admin/lists/<listIRI>/comment

Yes, this is the design proposed at the top of this issue, so I agree 😀

ok, know that I read your comment again, I don‘t agree with 1. because this causes problems which 2. attempts solve.

ok, know that I read your comment again, I don‘t agree with 1. because this causes problems which 2. attempts to solve.

the first option gives users the possibility to modify multiple properties at once. If we omit it and have only the second option, then for changing every property user would need to make a separate request. That means if the user wants to change all basic info of a list, she has to make 3 requests, one for each property. That seems cumbersome to me. If we have both options 1. and 2., depending on how many properties the user wants to change, she can choose the proper route.

There are instances where the first option simply does not work. I see that you are not convinced. Maybe we should have an online meeting about this?

I understand that it can be cumbersome to send multiple requests, but this could be solved by implementing a helper-function in DSP-JS-LIB or a helper function in the component that deals with this data.