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

react-api: stats request uses POST for big queries/queryParameters #656

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

zbigg
Copy link
Contributor

@zbigg zbigg commented Apr 24, 2023

Description

Shortcut: https://app.shortcut.com/cartoteam/story/306016/support-for-long-queries-in-builder

@carto/react-api stats request suport for long queries or queryParameters by using POST is get URL exceeds certain limits.

Type of change

  • Feature

Acceptance

Please describe how to validate the feature or fix

  1. go to X
  2. test Y
  3. assert Z

If feature deals with theme / UI or internal elements used also in CARTO 3, please also add a note on how to do acceptance on that part.

Basic checklist

  • Good PR name
  • Shortcut link
  • Changelog entry
  • Just one issue per PR
  • GitHub labels
  • Proper status & reviewers
  • Tests
  • Documentation

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #306016: Support for long queries in Builder.

@coveralls
Copy link
Collaborator

coveralls commented Apr 24, 2023

Pull Request Test Coverage Report for Build 4809279272

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 71.465%

Totals Coverage Status
Change from base Build 4800692044: 0.1%
Covered Lines: 1969
Relevant Lines: 2532

💛 - Coveralls

@zbigg zbigg requested review from VictorVelarde, eamador and a team April 24, 2023 16:48
});

test('query source - big queryParameters, should return stats using POST', async () => {
const huugeQueryParameters = Array.from(Array(5000)).reduce((r, _, i) => {
Copy link
Contributor

@VictorVelarde VictorVelarde Apr 25, 2023

Choose a reason for hiding this comment

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

😄 niiiiiiiiice

expect(res).toEqual(QUERY_TEST.output);

expect(fetchMock).toBeCalledWith(QUERY_TEST.url, {
method: 'POST',
Copy link
Contributor

Choose a reason for hiding this comment

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

@zbigg Should we check the method: 'GET' in the previous tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GET is default in fetch api, and this assertion is exact match, so we should be safe

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM

@zbigg zbigg force-pushed the feature/sc-306016/support-for-long-queries-in-builder branch from 93a45a5 to 465c702 Compare April 25, 2023 14:45
@zbigg zbigg force-pushed the feature/sc-306016/support-for-long-queries-in-builder branch from 465c702 to 64feed9 Compare April 26, 2023 13:28
@zbigg zbigg merged commit 25471cb into master Apr 26, 2023
@zbigg zbigg mentioned this pull request Apr 26, 2023
@zbigg zbigg deleted the feature/sc-306016/support-for-long-queries-in-builder branch April 18, 2024 10: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.

3 participants