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.

Sign up for GitHub

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

Jump to bottom

Implementing $includes operation with paging. #4810

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Implementing $includes operation with paging. #4810

wants to merge 14 commits into from

Conversation

tarunmathew12
Copy link
Contributor

@tarunmathew12 tarunmathew12 commented Feb 3, 2025 •
edited
Loading

Description

The PR contains the implementation of the $includes operation (explained in the spec below) including UT/E2E tests.

[$includes operation spec]
https://microsoft.sharepoint.com/:w:/t/msh/EaYsMkWJsVxNpaHhokCQbQcBxHtWbL8u80sTsENnKYNnPA?e=O1gE7h

Related issues

Addresses [issue #138967].
User Story 138967: Implement a new $include operation.

Testing

Manually tested. Automated tests being added.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@tarunmathew12 tarunmathew12 added New Feature Label for a new feature in FHIR OSS Open source This change is only relevant to the OSS code or release. labels Feb 3, 2025
@tarunmathew12 tarunmathew12 added this to the 2Wk09 milestone Feb 3, 2025
@tarunmathew12 tarunmathew12 requested a review from a team as a code owner February 3, 2025 22:48
@tarunmathew12
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tarunmathew12
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tarunmathew12 tarunmathew12 changed the title [Do Not Review] Implementing $includes operation with paging. Implementing $includes operation with paging. Feb 13, 2025
@tarunmathew12
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tarunmathew12
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

feordin
feordin reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@feordin feordin left a comment

Choose a reason for hiding this comment

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

I have been testing the $include feature, and it is looking good! I have a few points to ask about:

  1. if we have more than 500 included items, then in the first Bundle of results we do return any of the included items. After testing, this seems weird, when I request 1 resource and all _revincludes, and the included items are many, but the matched results are one or few. My initial Bundle is very small. Could we not include the first page of "included" results in the remaining space of the bundle?
  2. _count does not apply to "included" resources. I believe that is correct, but wanted to double check. If I am asking for a default _count of 10, and I get 4 or 5 matches, but hundreds of included items, the resulting bundle will be very large, and the client may not be expecting that.
  3. We include the observation outcome of "Include result was truncated" even in the pages of the $includes link. Once I am following the $include link, it seem like I should page through that, and the existence of a "next" link would indicate that I am not finished paging. The operation outcome seems out of place at that point.

@tarunmathew12
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

feordin
feordin previously approved these changes Feb 21, 2025
View reviewed changes
brendankowitz
brendankowitz requested changes Feb 24, 2025
View reviewed changes
Copy link
Member

@brendankowitz brendankowitz left a comment

Choose a reason for hiding this comment

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

Lets hold off on merging until we discuss/resolve any breaking behaviors

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 25, 2025
View reviewed changes
src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs
EnsureArg.IsNotNull(coreFeaturesConfiguration?.Value, nameof(coreFeaturesConfiguration));

_mediator = mediator;
_coreFeaturesConfiguration = coreFeaturesConfiguration.Value;

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
coreFeaturesConfiguration
Loading
may be null at this access as suggested by
this
Loading
null check.

Copilot Autofix AI about 10 hours ago

To fix the problem, we need to ensure that _coreFeaturesConfiguration is not null before it is dereferenced. This can be done by adding a null check before accessing _coreFeaturesConfiguration.SupportsIncludes in the Search method. This will prevent a potential NullReferenceException if _coreFeaturesConfiguration is null.

Suggested changeset 1
src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs b/src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs
--- a/src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs
+++ b/src/Microsoft.Health.Fhir.Shared.Api/Controllers/IncludesController.cs
@@ -48,3 +48,3 @@
         {
-            if (!_coreFeaturesConfiguration.SupportsIncludes)
+            if (_coreFeaturesConfiguration == null || !_coreFeaturesConfiguration.SupportsIncludes)
             {
EOF
@@ -48,3 +48,3 @@
{
if (!_coreFeaturesConfiguration.SupportsIncludes)
if (_coreFeaturesConfiguration == null || !_coreFeaturesConfiguration.SupportsIncludes)
{
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LTA-Thinking LTA-Thinking LTA-Thinking left review comments

@brendankowitz brendankowitz brendankowitz requested changes

@feordin feordin feordin left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
New Feature Label for a new feature in FHIR OSS Open source This change is only relevant to the OSS code or release.
Projects
None yet
Milestone
2Wk09
Development

Successfully merging this pull request may close these issues.
4 participants
@tarunmathew12 @brendankowitz @feordin @LTA-Thinking