Skip to content

made unknown code system error severity config work #7194

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrdnctrk
Copy link
Collaborator

@mrdnctrk mrdnctrk commented Aug 13, 2025

#closes #7196

Description

This PR addresses a bug where validating a resource with an unknown CodeSystem fails to produce any validation issues, even when the UnknownCodeSystem issue severity is set to ERROR.


Root Cause

The main issue was that none of ValidationSupport classes were returning a validation result for unknown code systems. The UnknownCodeSystemWarningValidationSupport class, whose purpose is to generate these issues, was skipping the creation of an issue if the severity was set to ERROR, but would correctly produce an issue if the severity was set to WARNING.

It appears this feature may have never worked as intended. The existing validation tests were also not expecting any ERROR level issues and were, in fact, incorrectly asserting that no issues would be produced when the severity was set to ERROR.


Changes in this PR

This PR makes two key improvements: it corrects the behavior of UnknownCodeSystemWarningValidationSupport to properly handle error cases, and it refines the ValidationSupport interface to align with the unique role of UnknownCodeSystemWarningValidationSupport.

1. The UnknownCodeSystemWarningValidationSupport class now produces a validation issue with the configured severity if there's an unknown code system

  • The class will no longer skip generating an issue when the severity is set to ERROR.
  • This also changes the behavior for the INFORMATION severity setting. Instead of attempting to suppress validation issues (the previous behavior), it will now correctly generate INFORMATION level issues as configured. If the goal is to suppress issues, a mechanism like ValidationMessageSuppressingInterceptor.java would be a more appropriate approach, or at least we can introduce a separate config or a config value for suppressing the issues.

2. A new method, canGenerateValidationResultForCodeSystem, has been added to the ValidationSupport interface.

  • The UnknownCodeSystemWarningValidationSupport class is not a true ValidationSupport class as it cannot actually validate a code system. However, to ensure it was called at the end of the validation chain, it was "abusing" the ValidationSupport interface by returning true for isSupportedCodeSystem, which would mislead the core validator.

  • This had the unintended effect of causing the core library to follow a different code path for the same resource when validated through the CLI versus HAPI-FHIR because core validator also calls isSupportedCodeSystem itself in several places through context.supportsSystem, see for example src1, src2

  • To improve this discrepancy, the new canGenerateValidationResultForCodeSystem method is now called in the validation chain. UnknownCodeSystemWarningValidationSupport will return true for this new method (since it can generate a result) but will now correctly return false for isSupportedCodeSystem. This ensures the core validator is not misled while still allowing the UnknownCodeSystemWarningValidationSupport to be properly invoked. For all the other existing ValidationSupport classes canGenerateValidationResultForCodeSystem behaves the same as isSupportedCodeSystem.


Outstanding Issue

Even though this PR attempts to fix the issue on the HAPI-FHIR side, this is not enough to generate error level issues in all cases. There is still an outstanding question for the core validator. The core validator has its own logic (src) for determining the severity of an issue for an unknown CodeSystem, which is based on the binding strength. It will ignore the error severity set by HAPI-FHIR. It produces errors only when binding strength is REQUIRED. For any other binding strength, it assigns a warning severity be default. See the issue on the core validator: hapifhir/org.hl7.fhir.core#2129. So currently we are able to produce an error level issue only for a REQUIRED binding.


Future Work

I believe a better long-term solution would be to move the logic of UnknownCodeSystemWarningValidationSupport somewhere eles, maybe into the validation chain or to the wrapper, rather than trying to make it conform to the ValidationSupport interface. However, since this would require a larger refactoring, I am leaving that for a future PR.

@robogary
Copy link
Contributor

Formatting check succeeded!

@mrdnctrk mrdnctrk requested a review from Copilot August 14, 2025 16:17
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements configurable severity for unknown code system validation errors. Previously, unknown code systems always generated errors or warnings with fixed severity levels. This change allows the severity to be configured through settings, enabling teams to control whether unknown code systems should result in errors, warnings, or informational messages.

Key Changes

  • Added configuration for unknown code system error severity in JpaStorageSettings and InMemoryTerminologyServerValidationSupport
  • Introduced canGenerateValidationResultForCodeSystem method to replace isCodeSystemSupported in validation chains
  • Updated validation support chain to use the new method for determining if validation results can be generated for unknown code systems

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ValidationSupportChain.java Replaced isCodeSystemSupported with canGenerateValidationResultForCodeSystem in validation logic
UnknownCodeSystemWarningValidationSupport.java Refactored to use new method and removed complex conditional logic
InMemoryTerminologyServerValidationSupport.java Added configurable severity setting for unknown code systems
JpaStorageSettings.java Added configuration property for unknown code system issue severity
IValidationSupport.java Added new interface method canGenerateValidationResultForCodeSystem
Test files Updated mock calls and assertions to use new method names and expected behaviors

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mrdnctrk mrdnctrk force-pushed the emre-unknown-sys-validation-clean-rebased-to-master branch from 70227fa to e7180f2 Compare August 14, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validation setNonExistentCodeSystemSeverity is not working as expected when set to error
2 participants