Skip to content
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

Send URIs to unindex or remove via form data if more than one was provided #29

Merged
merged 5 commits into from
Sep 30, 2021

Conversation

Enet4
Copy link
Collaborator

@Enet4 Enet4 commented Sep 24, 2021

Resolves #28.

- if uri parameter has more than one item,
  send content as URL-encoded body
- add tests covering unindexing and removing
  more than one item
- also add "file:" scheme on some examples of URI
  (they are more common than not)
- change how the service mock validates some inputs
   - plugins and provider names: \w+
   - URIs via dedicated function
callback should always be specified,
although it may be undefined.
This signature is better at preventing issues.
- accept dangling commas, which are not necessarily bad
@Enet4 Enet4 added the bug label Sep 24, 2021
@Enet4 Enet4 requested a review from bastiao September 24, 2021 17:22
@coveralls
Copy link

coveralls commented Sep 24, 2021

Coverage Status

Coverage increased (+0.08%) to 95.987% when pulling a608a32 on bug/unindex-remove-form-data into f54c893 on master.

@Enet4 Enet4 marked this pull request as draft September 28, 2021 13:49
- form data request body is already parsed,
  use it directly
- retweak URI validation function to no longer use URL.parse
@Enet4 Enet4 marked this pull request as ready for review September 28, 2021 14:01
@bastiao
Copy link
Member

bastiao commented Sep 29, 2021

Tested with Dicoogle 3 and works fine.
It seems that if the array of URIs contains one element it uses one implementation, otherwise, it will send by POST. Why shouldn't we use only one method?

@Enet4
Copy link
Collaborator Author

Enet4 commented Sep 29, 2021

It seems that if the array of URIs contains one element it uses one implementation, otherwise, it will send by POST. Why shouldn't we use only one method?

No strong opinion on this, I felt it just made the request simpler when it didn't need to have a response body. On the other hand, it would be simpler for the client to just provide these entries in a uniform fashion. I can change this to always send the URIs via form data.

In the meantime, I noticed that the OpenAPI specification of the web API still refers to this property as a query property, so the spec should be updated accordingly. I filed bioinformatics-ua/dicoogle#512 for this.

@bastiao
Copy link
Member

bastiao commented Sep 29, 2021

I'm fine with any option.
You can merge.

optional parameter in impl signature of unindex
@Enet4 Enet4 merged commit dae180e into master Sep 30, 2021
@Enet4 Enet4 deleted the bug/unindex-remove-form-data branch September 30, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client should send URIs to unindex and remove as form data
3 participants