WIP: FI-3556: Migrate MustSupport logic into core #584

dehall · 2025-01-03T21:56:28Z

Summary

This PR migrates the logic from the MustSupport test in the US Core test kit into an evaluator Rule. For the most part, files have been copied over and changed as little as possible, since the logic has been working well in that test kit for a while.

The idea in the test kit is to parse the StructureDefinitions into metadata, apply custom logic to add/modify/remove things in that metadata, and then the MustSupportTest runs based on that metadata. (This is mostly because of things that are defined in narrative rather than in a structured way, for example US Core v3.1.1 Patient "previous name" is MS only in narrative, and "choices" are defined only in narrative)
To maintain that flexibility as we move this capability into core, the new AllMustSupportsPresent rule first generates the metadata, but then offers a block for callers to apply custom logic to the metadata, or offers an alternative entrypoint where the metadata may be provided directly. There's currently no way to apply custom logic via the CLI but we could explore that if we want to.

This Rule is the first of several that depends on associating resources in the dataset with profiles in the IG. It's impossible to determine intent programmatically, so I've added a "ProfileConformanceHelper" that offers a few approaches to make a determination, based on meta.profile, validator conformance (the skeleton is there but not yet implemented), a block to allow for custom logic like matching codes, or just fall back to resource type.

Notes on Changed Files

lib/inferno/dsl/fhir_evaluation/rules/all_must_supports_present.rb
- Mostly copied from https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/must_support_test.rb but added the check method.
spec/inferno/dsl/fhir_evaluation/rules/all_must_supports_present_spec.rb
- Mostly copied from https://github.com/inferno-framework/us-core-test-kit/blob/main/spec/us_core/must_support_test_spec.rb but reworked pretty significantly to use metadata fixtures instead of test classes, and added a couple tests for the check method. See notes on metadata below.
lib/inferno/entities/ig.rb
- Refactored to bring in logic from the IgResources class https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/generator/ig_resources.rb

The following files were copied much more cleanly from the US Core Test Kit. The main changes to these were to change the modules from UsCoreTestKit --> Inferno::DSL and address Rubocop issues. Most issues were autocorrected (rubocop -A) but for "cyclomatic complexity too high" I broke some nested logic out into new methods. (Some of the logic was tough to figure out what it's actually doing, so hopefully the names are accurate and meaningful) The original method signatures for the "main entrypoints" to these classes/modules should never change.

lib/inferno/dsl/fhir_resource_navigation.rb
lib/inferno/dsl/must_support_metadata_extractor.rb
lib/inferno/dsl/primitive_type.rb
lib/inferno/dsl/value_extractor.rb
- Also fixed a typo here, the class was called ValueExactor
spec/fixtures/QuestionnaireResponse.json
spec/inferno/dsl/must_support_metadata_extractor_spec.rb

Metadata files were copied from various lib/us_core_test_kit/generated/[...]/metadata.yml and renamed to a more specific name. I copied these since the existing spec tests for the MustSupport logic used them and a lot of the MS metadata turns out to be custom. There should still be a good mix of tests that use the metadata construction path and tests that use a directly passed metadata.

Testing Guidance

Execute the CLI with bundle exec inferno evaluate ig [--data_path path_to_data_files], for example:

# evaluate the examples in the US Core 3.1.1 IG
bundle exec inferno evaluate ./spec/fixtures/uscore311.tgz

# evaluate data in ~/sample_data/ against the US Core 3.1.1 IG
bundle exec inferno evaluate ./spec/fixtures/uscore311.tgz --data_path ~/sample_data/

Please test against some other IGs as well. Frankly I'm less concerned about the correctness of other IGs because the logic should be appropriate across all IGs, but I am concerned about brittleness - does the code make assumptions about things that aren't guaranteed? In particular things like the presence of certain fields. I've tested against most of the IGs for which we have test kits and made a few "null check" type changes to prevent exceptions.

Another important question for reviewers: do you have any concerns about potentially plugging our test kits that use generators into this?

codecov · 2025-01-03T21:59:12Z

Codecov Report

Attention: Patch coverage is 88.73239% with 64 lines in your changes missing coverage. Please review.

Project coverage is 84.59%. Comparing base (e3f14cf) to head (978b3cb).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/inferno/dsl/must_support_metadata_extractor.rb	87.17%	20 Missing ⚠️
...fhir_evaluation/rules/all_must_supports_present.rb	90.34%	17 Missing ⚠️
lib/inferno/dsl/fhir_resource_navigation.rb	87.96%	13 Missing ⚠️
lib/inferno/dsl/value_extractor.rb	85.91%	10 Missing ⚠️
lib/inferno/dsl/fhir_evaluation/evaluator.rb	60.00%	2 Missing ⚠️
lib/inferno/entities/ig.rb	91.66%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (88.73%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
+ Coverage   84.28%   84.59%   +0.30%     
==========================================
  Files         274      280       +6     
  Lines       11601    12151     +550     
  Branches     1291     1429     +138     
==========================================
+ Hits         9778    10279     +501     
- Misses       1815     1864      +49     
  Partials        8        8

Flag	Coverage Δ
backend	`92.46% <88.73%> (-0.17%)`	⬇️
frontend	`79.11% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

dehall · 2025-01-14T18:03:11Z

lib/inferno/dsl/fhir_resource_navigation.rb

+      DAR_EXTENSION_URL = 'http://hl7.org/fhir/StructureDefinition/data-absent-reason'.freeze
+      PRIMITIVE_DATA_TYPES = FHIR::PRIMITIVES.keys
+
+      def resolve_path(elements, path)


Capturing a comment made offline -- we need good documentation for all of these new functions and classes

Jammjammjamm · 2025-01-15T13:31:12Z

lib/inferno/dsl/fhir_evaluation/rules/all_must_supports_present.rb

+          # Customizing the metadata may add, modify, or remove items.
+          # For instance, US Core 3.1.1 Patient "Previous Name" is defined as MS only in narrative.
+          # Choices are also defined only in narrative.
+          def perform_must_support_test(profile, resources, ig) # rubocop:disable Naming/MethodParameterName


The Naming/MethodParameterName rule should have an AllowedNames configuration option that we could use to globally allow ig as a parameter name rather than having to override this rule. https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Naming/MethodParameterName

Jammjammjamm · 2025-01-15T13:33:42Z

lib/inferno/dsl/fhir_evaluation/rules/all_must_supports_present.rb

+
+          def missing_elements(resources = [])
+            @missing_elements ||= find_missing_elements(resources, must_support_elements)
+            @missing_elements


This line isn't doing anything.

Jammjammjamm · 2025-01-15T13:37:27Z

lib/inferno/dsl/must_support_metadata_extractor.rb

+        }
+      end
+
+      def uscdi_requirement_element?(element)


This is specific to US Core, so it feels a bit strange to include here.

Changed this to be a more generic by_requirement_extension_only? and the constructor now takes an optional extension url. Alternatively we could just remove this concept altogether but then that means we'll have to leave a lot of this logic duplicated in the us core test kit

dehall · 2025-01-15T16:15:10Z

lib/inferno/dsl/fhir_evaluation/rules/all_must_supports_present.rb

+          # Choices are also defined only in narrative.
+          def perform_must_support_test(profile, resources, ig) # rubocop:disable Naming/MethodParameterName
+            profile_metadata = extract_metadata(profile, ig)
+            yield profile_metadata if block_given?


Another comment from offline - this test could be painful to debug if you can't easily inspect the metadata that it's using behind the scenes. Consider having some way to write it to file

I'm imagining writing to a temp file. Potentially also logging the location of said tempfile so that it can be easily found.

yunwwang · 2025-01-15T17:59:35Z

lib/inferno/dsl/must_support_metadata_extractor.rb

+      end
+
+      def must_support_slices
+        pattern_slices + type_slices + value_slices


pattern_slices is deperated in FHIR R5 and some IGs already adopt that change and merge patterns into the value_slices. See the latest changes in US Core here

Are those changes stable enough for me to copy them over here now, or do I need to hold off a little longer?

It has been used in US Core generation for v3.1.1 to v8.0.0. The generated metadata match those before though the order is changed a little bit.

We'll still need to support pattern slices for all of the existing IGs that use it.

The code does not throw away pattern slicing. It combines the value and pattern together. Before the change, the logic is:

If type = value, then extracts the fixed[x]

If type = pattern, then extracts the pattern[x]

The new logic is

If type is value or pattern, then extracts fixed[x]. If fixed[x] is empty, then extracts pattern[x]

This new logic handles both the FHIR R4 value and pattern type and the new FHIR R5 value type under method.

I'm struggling to integrate this change (meaning, struggling to refactor and reduce cyclomatic complexity to appease rubocop) because I don't understand what's supposed to be different in the output. If we just leave this as two separate methods pattern_slices and value_slices what's the difference?

If trying to reduce the cyclomatic complexity is making the code worse, you should just ignore the rule. Metrics-based rules for things like cyclomatic complex are useful as a guide, but I don't think trying to appease rubocop by making arbitrary methods just to meet the metrics is necessarily helpful.

Sure, but in this case the code has changed from 2 methods that each do 1 thing, to 1 method that conditionally does 2 things without any obvious overlap in those 2 things. Very possible I'm missing something but I'd rather stick with the old code than override a code quality rule if there's no difference in the intended result

Ok I should have gone to the spec before asking. It looks like in R4, there was an expectation that element.discriminator.type=pattern required a pattern[x] and element.discriminator.type=value required a fixed[x]. I assumed when Yunwei said pattern type was deprecated that pattern[x] was deprecated with it. But no the change is that now there's no difference in meaning between the two discriminator types so you can use either discriminator type with either fixed[x] or pattern[x]. So there is a difference in result. I'll take one more look at rubocop stuff but will ignore the rule if it comes to that

No. Actually that was what the intention at start. Then people found that there is no retriction put in there. That is no matter type=pattern or type=value, the slice can have either pattern[x] or fixed[x]. That caused the change in R5 to deprecate type=pattern (not pattern[x]). If you want to keep pattern_slices, that is fine. But you need to adjust the starting logic that when type=value, then find the slicing definition from either the value_slices or pattern_slices

yunwwang · 2025-01-15T20:24:08Z

lib/inferno/dsl/must_support_metadata_extractor.rb

+
+      def must_support_pattern_slice_elements
+        must_support_slice_elements.select do |element|
+          discriminators(sliced_element(element))&.first&.type == 'pattern'


US Core generator codes here are based on an assumption (not related to this pattern change) that all the discriminators of an element have the same type. It is ok so far. I have not seen any US Core profile having one element with discriminators of different types. But in theory this exists. This could be an additional feature to support if this is to used beyond US Core

Unfortunately I found an instance of this in the DaVinci CRD - Patient profile STU 2, slicing on Patient.identifier https://hl7.org/fhir/us/davinci-crd/STU2/StructureDefinition-profile-patient.html

It's been simplified in the STU 2.1 version of the profile https://hl7.org/fhir/us/davinci-crd/STU2.1/StructureDefinition-profile-patient.html so I'm wondering if the original complex multi-slicing was really intended but I'll still push to get STU 2 working here

I think this is OK since both value and pattern can be handled as value.

Do you think this is a blocker? I mean, the general idea of multiple discriminators on a sliced element, not the specific CRD example. The metadata "schema" currently assumes only a single discriminator, so I'm hesitant to significantly refactor the logic to support multiple discriminators when it doesn't seem to be common. That one instance I did find, I think it's actually incorrect. (The discriminator suggests a slice on a coding system but there's no element definition for that element to define what the slice is) If you don't think this is a blocker I'll create a follow-up ticket for it

yunwwang · 2025-01-15T20:26:13Z

lib/inferno/dsl/must_support_metadata_extractor.rb

+
+              {
+                path: discriminator_path,
+                value: fixed_element.fixedUri || fixed_element.fixedCode


The same here. fixedCode and fixUri are the two fixed[x] used by US Core. Other IGs could have additional fixed[x] value.

yunwwang · 2025-01-15T21:16:47Z

lib/inferno/dsl/fhir_evaluation/rules/all_must_supports_present.rb

+          end
+
+          def handle_must_support_element_choices
+            missing_elements.delete_if do |element|


I added a new choice type for US Core 8., which is called :elements. Each element choice has a path and a fixed_value. This is an expansion of :paths choice. You can see the code changes here

dehall force-pushed the fi-3556-must-support-rule branch from de0ff6e to 42148ca Compare January 13, 2025 16:47

dehall marked this pull request as ready for review January 13, 2025 18:38

dehall requested review from yunwwang and Jammjammjamm January 13, 2025 18:39

dehall commented Jan 14, 2025

View reviewed changes

Jammjammjamm reviewed Jan 15, 2025

View reviewed changes

dehall commented Jan 15, 2025

View reviewed changes

yunwwang reviewed Jan 15, 2025

View reviewed changes

dehall added 16 commits January 17, 2025 13:29

FI-3556: Migrate MustSupport logic into core

4214f48

rubocop fixes, part 1

bbcb82b

rubocop part 2, cyclomatic complexity

eef06bf

add validator attr to Evaluator

3cd7638

refactor profile conformance 'checker' to 'helper'

ef86cb0

found more spec tests in the us core repo

ddce00c

first pass of ValueExtractor tests

d9b42a5

additional unit tests

ce9b2df

a little more testing

92b7d03

fixes from testing additional IGs

6fb5e83

more tests

23d6378

just one more test

e114ed6

Add 'ig' as allowable method param name to rubocop

c3f07c2

Replace :uscdi_only with :by_requirement_extension_only

592252e

integrate us core 8 changes

c13d895

rubocop update after rebase

978b3cb

dehall force-pushed the fi-3556-must-support-rule branch from 3540f39 to 978b3cb Compare January 17, 2025 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: FI-3556: Migrate MustSupport logic into core #584

WIP: FI-3556: Migrate MustSupport logic into core #584

dehall commented Jan 3, 2025 •

edited

Loading

codecov bot commented Jan 3, 2025 •

edited

Loading

dehall Jan 14, 2025

Jammjammjamm Jan 15, 2025

Jammjammjamm Jan 15, 2025

Jammjammjamm Jan 15, 2025

dehall Jan 15, 2025

dehall Jan 15, 2025

Jammjammjamm Jan 15, 2025

yunwwang Jan 15, 2025

dehall Jan 15, 2025

yunwwang Jan 15, 2025 •

edited

Loading

Jammjammjamm Jan 16, 2025

yunwwang Jan 16, 2025 •

edited

Loading

dehall Jan 17, 2025

Jammjammjamm Jan 17, 2025

dehall Jan 17, 2025

dehall Jan 17, 2025

yunwwang Jan 21, 2025

yunwwang Jan 15, 2025 •

edited

Loading

dehall Jan 21, 2025

yunwwang Jan 21, 2025

dehall Jan 22, 2025

yunwwang Jan 15, 2025

yunwwang Jan 15, 2025

WIP: FI-3556: Migrate MustSupport logic into core #584

Are you sure you want to change the base?

WIP: FI-3556: Migrate MustSupport logic into core #584

Conversation

dehall commented Jan 3, 2025 • edited Loading

Summary

Notes on Changed Files

Testing Guidance

codecov bot commented Jan 3, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunwwang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunwwang Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunwwang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dehall commented Jan 3, 2025 •

edited

Loading

codecov bot commented Jan 3, 2025 •

edited

Loading

yunwwang Jan 15, 2025 •

edited

Loading

yunwwang Jan 16, 2025 •

edited

Loading

yunwwang Jan 15, 2025 •

edited

Loading