Skip to content

Add validation on resource id for import. #4977

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
Merged
merged 3 commits into from
May 12, 2025
Merged

Add validation on resource id for import. #4977

merged 3 commits into from
May 12, 2025

Conversation

tarunmathew12
Copy link
Contributor

Description

A validation is added on the resource id of resources in the import data.

Related issues

Addresses [issue #147537].
Bug 147537: Import should validate IDs

Testing

Tested by adding some unit and e2e tests.

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.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@tarunmathew12 tarunmathew12 added this to the CY25Q2/2Wk03 milestone May 8, 2025
@tarunmathew12 tarunmathew12 requested a review from a team as a code owner May 8, 2025 18:50
@tarunmathew12 tarunmathew12 added Bug Bug bug bug. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Open source This change is only relevant to the OSS code or release. labels May 8, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems May 8, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems May 12, 2025
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
{ "?badresource&id", false },
{ "abc.123.ABC", true },
{ string.Empty, false },
{ Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },

Check notice

Code scanning / CodeQL

Redundant ToString() call Note test

Redundant call to 'ToString' on a String object.

Copilot Autofix

AI 10 days ago

To fix the issue, we will remove the redundant ToString() call on line 1758. The Guid.NewGuid().ToString() method already returns a string, so there is no need to call ToString() on its result. This change will simplify the code without altering its functionality.

Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.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/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
@@ -1757,3 +1757,3 @@
                 { string.Empty, false },
-                { Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },
+                { Guid.NewGuid() + Guid.NewGuid().ToString(), false },
             };
EOF
@@ -1757,3 +1757,3 @@
{ string.Empty, false },
{ Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },
{ Guid.NewGuid() + Guid.NewGuid().ToString(), false },
};
Copilot is powered by AI and may make mistakes. Always verify output.
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
{ "?badresource&id", false },
{ "abc.123.ABC", true },
{ string.Empty, false },
{ Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },

Check notice

Code scanning / CodeQL

Redundant ToString() call Note test

Redundant call to 'ToString' on a String object.

Copilot Autofix

AI 10 days ago

To fix the issue, we will remove the redundant ToString() calls on the Guid.NewGuid() objects in line 1758. The + operator will implicitly convert the GUIDs to strings during concatenation, so the explicit calls are not needed. This change will make the code cleaner and more concise while maintaining the same functionality.

Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.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/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs
@@ -1757,3 +1757,3 @@
                 { string.Empty, false },
-                { Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },
+                { Guid.NewGuid() + Guid.NewGuid(), false },
             };
EOF
@@ -1757,3 +1757,3 @@
{ string.Empty, false },
{ Guid.NewGuid().ToString() + Guid.NewGuid().ToString(), false },
{ Guid.NewGuid() + Guid.NewGuid(), false },
};
Copilot is powered by AI and may make mistakes. Always verify output.
@tarunmathew12 tarunmathew12 enabled auto-merge (squash) May 12, 2025 20:04
@tarunmathew12 tarunmathew12 merged commit b044a92 into main May 12, 2025
47 checks passed
@tarunmathew12 tarunmathew12 deleted the personal/v-tmathew/import-validate-rid branch May 12, 2025 21:00
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 approved these changes

Assignees
No one assigned
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Bug Bug bug bug. Open source This change is only relevant to the OSS code or release.
Projects
None yet
Milestone
CY25Q2/2Wk03
Development

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