-
Notifications
You must be signed in to change notification settings - Fork 568
Fix search with include and sort #5242
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
base: main
Are you sure you want to change the base?
Conversation
mikaelweave
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions
...lth.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
Show resolved
Hide resolved
| if (_tokens.Length == 5 | ||
| && short.TryParse(_tokens[3]?.ToString(), out tid) | ||
| && long.TryParse(_tokens[4]?.ToString(), out sid0)) | ||
| if (_tokens.Length >= 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know a lot this code was existing but I'm seeing a lot of magic numbers in this block. Any chance you can add some comments to improve readability for those new to this area?
| if (includesContinuationToken == null) | ||
| { | ||
| _logger.LogWarning("Bad Request (InvalidIncludesContinuationToken)"); | ||
| throw new BadRequestException(Resources.InvalidIncludesContinuationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a BadRequestException correct here? If the token is malformed is it an issue from the client or in our code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the safer expectation is that the client passed in the wrong value for the CT. It is possible for us to have a bug in the CT generation code, but I don't think that is the right assumption to take with a user provided value.
…t the tests are correct
|
There are still more edge cases to fix here. The behavior when using a second phase includes continuation token after the second page of first phase includes results isn't working correctly. |
Description
When using
_sortand_include/_revincludetogether there are issues when there are enough included resources to create an include continuation token.This PR fixes all of these issues.
Related issues
Addresses Bug 177120
Testing
Manual testing and new E2E tests.
FHIR Team Checklist
Semver Change (docs)
Patch