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.

Sign up for GitHub

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

Jump to bottom

Unify how the GalaxyInteractorApi handles JSON requests #12152

Merged
merged 7 commits into from Jun 23, 2021
Merged

Unify how the GalaxyInteractorApi handles JSON requests #12152

merged 7 commits into from Jun 23, 2021

Conversation

davelopez
Copy link
Contributor

This is a follow up to #12136 (comment)

It basically allows the rest of the HTTP request methods in the GalaxyInteractorApi (that can contain a payload) to specify the json=True parameter. This will add the proper Content-Type: application/json to the header that is now explicitly required by FastAPI.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

@jmchilton
Copy link
Member

I'm sorry this is needed now - that is no fun. Should json=True be the default instead of json=False though?

@davelopez
Copy link
Contributor Author

No worries!
Yes, I guess it probably makes more sense to use json=True as default 😅 I will set it.

@davelopez davelopez force-pushed the unify_api_interactor branch 2 times, most recently from 884b537 to 379fbbc Compare June 18, 2021 13:39
@davelopez
Handle json requests for the rest of HTTP methods in the GalaxyIntera…
b7ab45d 
…ctorApi

The GalaxyInteractorApi was defining json payloads only for the POST method using the json boolean parameter. This unifies the rest of the HTTP methods that can accept a payload to specify a json payload.
@davelopez
Replace content-type hack in tests with the json=True parameter
f5c5f63
@davelopez
Add json parameter to BasePopulator methods
dda3841
@davelopez
Adapt DELETE and PUT methods in tests
bfde767
@davelopez
Handle optional payloads
cab9198 
If the dictionary is empty we should set the json content to None, otherwise, if the endpoint has an optional payload (it can be None) but the schema of the payload contains required fields, the validation will fail if we provide an empty JSON ({}) instead of None.
@davelopez
Remove unnecessary json.dumps
e6ba58b
@davelopez
Unify interactor and populators methods signature
398e849
@davelopez
Copy link
Contributor Author

Unfortunately, setting json=True as default in the interactor had more side-effects than expected so I had to revert it.

Unless I may have missed something when trying to make it the default, I guess the safest way to proceed is by checking each particular endpoint as they get migrated to FastAPI and to mark the corresponding _post, _put, _delete calls in the tests with json=True.
We can always set the default to True once we identify what is causing trouble with setting all payloads as JSON.

Any other ideas?

@davelopez davelopez marked this pull request as ready for review June 18, 2021 15:57
@github-actions github-actions bot added this to the 21.09 milestone Jun 18, 2021
@bgruening bgruening requested a review from jmchilton June 19, 2021 17:21
@davelopez davelopez mentioned this pull request Jun 21, 2021
Merged
4 tasks
@jmchilton jmchilton merged commit aab49e8 into galaxyproject:dev Jun 23, 2021
@jmchilton
Copy link
Member

This seems like a good step forward! Thanks @davelopez

@davelopez davelopez deleted the unify_api_interactor branch June 24, 2021 08:32
davelopez added a commit to davelopez/galaxy that referenced this pull request Jun 25, 2021
@davelopez
Fix API tests by explicitly setting content-type to JSON
1b47ec9 
See galaxyproject#12152
davelopez added a commit to davelopez/galaxy that referenced this pull request Aug 27, 2021
@davelopez
Adapt test to use JSON headers
21c373b 
Required by FastAPI, see comment galaxyproject#12152 (comment)
davelopez added a commit to davelopez/galaxy that referenced this pull request Aug 27, 2021
@davelopez
Adapt test to use JSON headers
ea12013 
Required by FastAPI, see comment galaxyproject#12152 (comment)
davelopez added a commit to davelopez/galaxy that referenced this pull request Sep 1, 2021
@davelopez
Adapt test to use JSON headers
8645f82 
Required by FastAPI, see comment galaxyproject#12152 (comment)
davelopez added a commit to davelopez/galaxy that referenced this pull request Sep 4, 2021
@davelopez
Adapt test to use JSON headers
5aae1b8 
Required by FastAPI, see comment galaxyproject#12152 (comment)
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt even more API tests around history contents to use JSON payload
bcfa30c 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt history contents API tests to use JSON payload
80ca9d5 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
2e6030b 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt even more API tests around history contents to use JSON payload
01ed324 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt history contents API tests to use JSON payload
4891836 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
13e58e8 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 19, 2021
@davelopez
Adapt even more API tests around history contents to use JSON payload
3426ac8 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 27, 2021
@davelopez
Adapt API tests to use JSON payload
fc6f5aa 
See galaxyproject#12152 for details.
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 28, 2021
@davelopez
Adapt API tests to use JSON payload
d401283 
See galaxyproject#12152 for details.
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 29, 2021
@davelopez
Adapt API tests to use JSON payload
fb02841 
See galaxyproject#12152 for details.
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 29, 2021
@davelopez
Adapt history contents API tests to use JSON payload
4b4f6bf 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Oct 29, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
e06fb45 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 1, 2021
@davelopez
Adapt history contents API tests to use JSON payload
5c99857 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 1, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
ffac0c4 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 2, 2021
@davelopez
Adapt history contents API tests to use JSON payload
a9336f8 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 2, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
371d4f3 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 3, 2021
@davelopez
Adapt history contents API tests to use JSON payload
42a7dc3 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 3, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
bc51bf2 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 4, 2021
@davelopez
Adapt history contents API tests to use JSON payload
009baf0 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 4, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
f928431 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 8, 2021
@davelopez
Adapt history contents API tests to use JSON payload
ad1545a 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 8, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
5646375 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 12, 2021
@davelopez
Adapt history contents API tests to use JSON payload
3466bc1 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 12, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
55ba1f4 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 19, 2021
@davelopez
Adapt history contents API tests to use JSON payload
1059c2f 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 19, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
41a494f 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 25, 2021
@davelopez
Adapt history contents API tests to use JSON payload
862f5d3 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Nov 25, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
9783464 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Dec 13, 2021
@davelopez
Adapt history contents API tests to use JSON payload
004a666 
See galaxyproject#12152 for details
davelopez added a commit to davelopez/galaxy that referenced this pull request Dec 13, 2021
@davelopez
Adapt more API tests around history contents to use JSON payload
85395d0 
See galaxyproject#12152 for details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmchilton jmchilton Awaiting requested review from jmchilton

Assignees
No one assigned
Labels
area/testing/api area/testing/integration area/testing area/tool-framework kind/enhancement
Projects
None yet
Milestone
21.09
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@davelopez @jmchilton