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

Fix query in GET requests #362

Merged
merged 2 commits into from
Apr 3, 2023
Merged

Fix query in GET requests #362

merged 2 commits into from
Apr 3, 2023

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Nov 28, 2022

Related Issue(s):

Description:

Fixes query in GET requests by stringifying the JSON dictionary. Note that (for the Planetary Computer at least) the query parameter cannot be url encoded. @philvarner do you think this is a server bug? If so I can open it on stac-fastapi.

PR Checklist:

  • Code is formatted
  • Tests pass
  • Changes are added to the CHANGELOG

@philvarner
Copy link
Collaborator

The query value should be url encoded, so it's a bug if stac-fastapi isn't handling that correctly.

@gadomski gadomski force-pushed the issues/355-query-in-get-request branch from b8f0b27 to 7972ba7 Compare November 28, 2022 21:54
@gadomski
Copy link
Member Author

The query value should be url encoded, so it's a bug if stac-fastapi isn't handling that correctly.

I've opened stac-utils/stac-fastapi#504 to fix, and added url encoding to this PR. However, this means that tests will be broken until the Planetary Computer picks up stac-utils/stac-fastapi#504. I think it's worth waiting to merge this PR until the Planetary Computer is fixed, since this isn't a critical fix.

@philvarner
Copy link
Collaborator

Agree with waiting -- I only found it because I had a get endpoint that was proxying through a stac api request, and I just wanted to pass the parameters through to pystac-client needing to parse them, but that was easy enough to do.

@gadomski gadomski marked this pull request as draft December 15, 2022 19:45
@gadomski gadomski self-assigned this Jan 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2023

Codecov Report

Base: 85.05% // Head: 85.12% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (375b97c) compared to base (00e4c98).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   85.05%   85.12%   +0.07%     
==========================================
  Files          11       11              
  Lines         823      827       +4     
==========================================
+ Hits          700      704       +4     
  Misses        123      123              
Impacted Files Coverage Δ
pystac_client/item_search.py 93.01% <100.00%> (+0.03%) ⬆️
pystac_client/stac_api_io.py 87.06% <100.00%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gadomski gadomski added this to the 0.7.0 milestone Mar 31, 2023
@gadomski gadomski force-pushed the issues/355-query-in-get-request branch from 375b97c to 202be62 Compare March 31, 2023 15:09
@gadomski
Copy link
Member Author

Okay, I'm marking this ready to review. Even if the Planetary Computer doesn't support quoted dictionaries in GET requests, users can always use POST.

@gadomski gadomski marked this pull request as ready for review March 31, 2023 15:11
@gadomski gadomski requested a review from pjhartzell March 31, 2023 15:11
@pjhartzell pjhartzell self-requested a review April 3, 2023 15:12
@gadomski gadomski force-pushed the issues/355-query-in-get-request branch from 2015e2a to 7591170 Compare April 3, 2023 17:26
@gadomski gadomski force-pushed the issues/355-query-in-get-request branch from 7591170 to 67c0f66 Compare April 3, 2023 17:28
@gadomski gadomski requested a review from pjhartzell April 3, 2023 17:28
@gadomski gadomski merged commit af8279d into main Apr 3, 2023
@gadomski gadomski deleted the issues/355-query-in-get-request branch April 3, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GET request with "query" parameter throws exception
4 participants