Skip to content

Fix exception when passing url/code combinations to an external terminology server #6984

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
Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Fix exception when passing url/code combinations to an external terminology server #6984

wants to merge 5 commits into from

Conversation

another-luis
Copy link

@another-luis another-luis commented May 23, 2025 •
edited by epeartree
Loading

What was done:
When an external terminology server is being used, we were incorrectly handling the codeableConcept and url/code combination of input parameters. We were only handling the Coding case. We now handle Coding and url/code, and fail gracefully when codeableConcept is used.

@another-luis another-luis marked this pull request as draft May 23, 2025 14:10
@another-luis another-luis self-assigned this May 23, 2025
@another-luis another-luis requested a review from epeartree May 23, 2025 14:10
@robogary
Copy link
Contributor

[spotless]: Formatting check succeeded!

@another-luis another-luis linked an issue May 23, 2025 that may be closed by this pull request
Passing url/code combinations to verify a code using an external terminology server produces an error #6987
Open
@robogary
Copy link
Contributor

[spotless]: Formatting check succeeded!

@codecov Codecov
Copy link

codecov bot commented May 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.41%. Comparing base (112cec3) to head (f2943b8).
Report is 16 commits behind head on master.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #6984      +/-   ##
============================================
- Coverage     83.41%   83.41%   -0.01%     
- Complexity    29193    29249      +56     
============================================
  Files          1843     1846       +3     
  Lines        113422   113529     +107     
  Branches      14251    14261      +10     
============================================
+ Hits          94608    94696      +88     
- Misses        12730    12736       +6     
- Partials       6084     6097      +13

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@another-luis
Merge remote-tracking branch 'origin/master' into 7432-NullPointerExc…
3fb7bb2 
…eption-when-executing-a-validate-code-operation
@robogary
Copy link
Contributor

[spotless]: Formatting check succeeded!

epeartree
epeartree reviewed May 27, 2025
View reviewed changes
@robogary
Copy link
Contributor

[spotless]: This Pull Request has failed the formatting check

Please run mvn spotless:apply or mvn clean install -DskipTests to fix the formatting issues.

You can automate this auto-formatting process to execute on the git pre-push hook, by installing pre-commit and then calling pre-commit install --hook-type pre-push. This will cause formatting to run automatically whenever you push.

tadgh
tadgh requested changes May 28, 2025
View reviewed changes
Copy link
Collaborator

@tadgh tadgh left a comment

Choose a reason for hiding this comment

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

Overall looks good, would prefer not to add a whole new mock test class for this tiny behaviour. Could this be tested in an existing test class like ValidateCodeWithRemoteTerminologyR4Test ?

...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/BaseJpaResourceProviderCodeSystem.java
@@ -39,6 +39,7 @@
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.instance.model.api.IPrimitiveType;
import org.jetbrains.annotations.Nullable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: jetbrains imports are not permitted. Use the jakartax libraries for this.

...ir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/ResourceProviderCodeSystemTest.java

assertThat(expectedUrl.getValue()).isEqualTo(myUrlCaptor.getValue());
assertThat(expectedCode.getValue()).isEqualTo(myCodeCaptor.getValue());
assertThat(expectedDisplay.getValue()).isEqualTo(myDisplayCaptor.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue(non-blocking): this mock test is very brittle, is there any way we could actually test this under one of the R4 Provider tests with an actual running set of providers?

@robogary
Copy link
Contributor

[spotless]: This Pull Request has failed the formatting check

Please run mvn spotless:apply or mvn clean install -DskipTests to fix the formatting issues.

You can automate this auto-formatting process to execute on the git pre-push hook, by installing pre-commit and then calling pre-commit install --hook-type pre-push. This will cause formatting to run automatically whenever you push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epeartree epeartree epeartree left review comments

@tadgh tadgh tadgh requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@another-luis another-luis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Passing url/code combinations to verify a code using an external terminology server produces an error
4 participants
@another-luis @robogary @tadgh @epeartree