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 request body compression #4263

Merged
merged 2 commits into from
May 23, 2022

Conversation

Luke-Sikina
Copy link
Member

Describe changes proposed in this pull request:

  • Url params are now omitted from the url passed to the api client
  • This was breaking some startsWith logic for requests that we compress
    based on both url params and the uri
  • To fix this, we now match against both the url string and the param
    object

@Luke-Sikina Luke-Sikina force-pushed the fix-request-body-compression branch from 30962be to d69ce5d Compare May 19, 2022 17:21
- Url params are now omitted from the url passed to the api client
- This was breaking some startsWith logic for requests that we compress
based on both url params and the uri
- To fix this, we now match against both the url string and the param
object
@Luke-Sikina Luke-Sikina force-pushed the fix-request-body-compression branch from d69ce5d to 91e9246 Compare May 19, 2022 17:25
@Luke-Sikina Luke-Sikina requested a review from pvannierop May 19, 2022 18:11
Copy link
Contributor

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for solving this!

Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

There are some broken localdb tests but it doesn't seem to be specific to this PR? Bit tricky to verify with junit test reports broken

@inodb inodb merged commit 3fa3ad1 into cBioPortal:master May 23, 2022
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.

3 participants