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 case sensitive array query parameters #8988

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

jascks
Copy link
Contributor

@jascks jascks commented Aug 9, 2024

Description:
My first attempt (#8852) at fixing this issue did not accommodate the scenario where a query parameter is provided more than once. When that happens, the Express query parser aggregates the values for the key into an array. Thus, when the order=ASC parameter is specified a single time, the value is simply a string. However order=ASC&order=ASC results in the single order key, but this time with a value of [ ASC, ASC].

The solution is to accommodate the possibility of the array, and in that case to canonicalize each element individually.

Related issue(s):

Fixes #8790

Notes for reviewer:
Beyond the fact this scenario was not tested for previously, my assumption was that the process of merging multiple values within requestQueryParser() was handling this possibility. And, it was in the sense that the original code could handle whether the request parameter was a single value or an array, and that it was also merging to support case-insensitive keys.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…roperly canonicalize that case.

Signed-off-by: Jeff Schmidt <jeffrey.schmidt@swirldslabs.com>
@jascks jascks added bug Type: Something isn't working rest Area: REST API labels Aug 9, 2024
@jascks jascks added this to the 0.112.0 milestone Aug 9, 2024
@jascks jascks self-assigned this Aug 9, 2024
@jascks jascks requested a review from a team August 9, 2024 16:55
Copy link

sonarcloud bot commented Aug 9, 2024

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.34%. Comparing base (96acb4c) to head (85162c3).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8988      +/-   ##
============================================
- Coverage     92.34%   92.34%   -0.01%     
  Complexity     7515     7515              
============================================
  Files           909      909              
  Lines         30233    30237       +4     
  Branches       3698     3699       +1     
============================================
+ Hits          27920    27921       +1     
- Misses         1491     1494       +3     
  Partials        822      822              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@xin-hedera xin-hedera left a comment

Choose a reason for hiding this comment

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

LGTM

@jascks jascks merged commit b9e2f50 into main Aug 9, 2024
29 checks passed
@jascks jascks deleted the 8790-fix-case-sensitive-query-parameters branch August 9, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Something isn't working rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some rest api endpoints return 500 with order=ASC / order=DESC
3 participants