[Issue #1467] Add order_by support for more fields #1481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…4-update-api-schema
…4-update-api-schema
…4-update-api-schema
chouinar
requested review from
acouch,
aplybeah and
SammySteiner
as code owners
coilysiren
approved these changes
Mar 14, 2024
| # means we can't determine how to sort / need to correct | ||
| # the mismatch. | ||
| msg = f"Unconfigured sort_by parameter {pagination.order_by} provided, cannot determine how to sort." | ||
| raise Exception(msg) |
There was a problem hiding this comment.
This seems like is should raise a HTTP 400, not a HTTP 500? Do you have a class for that?
There was a problem hiding this comment.
I wanted to return a 500 here, if this happens, it is because of a bug in our code. This means our API allows you to pass in a field to sort by that we haven't implemented.
jamesbursa
approved these changes
Mar 15, 2024
| # This determines whether we use ascending or descending when building the query | ||
| sort_fn = asc if pagination.is_ascending else desc | ||
|
|
||
| match pagination.order_by: |
There was a problem hiding this comment.
Neat to see match being used, I haven't used it yet.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1467
Time to review: 10 mins
Changes proposed
Added the ability to order_by several more fields
Updates to test structure, continuing a pattern from the parent PR
Context for reviewers
Except for relevancy (which is its own separate ticket if we do it prior to a search index), this should cover all the sorting needs for search in the near term.
One callout is that sorting always shoves null values to the end now, regardless of whether they are ascending or descending (by default, Postgres considers null to be the largest value).
Additional information
Did some local testing of performance with the queries we actually use, and sorting by any of these fields has effectively the same performance cost, and adding an index to the un-indexed ones didn't seem to change that.