Skip to content

Fix "is-a" ValueSet expansion and add "descendent-of" support #5603

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

Closed
wants to merge 10 commits into from

Conversation

ohetrifork
Copy link
Contributor

Hi,

We're implementing validation for an custom implementation guide (IG) using HAPI FHIR. The IG contain ValueSets that are defined using "descendent-of" and "is-a" relations, using codes from a custom code system. To that end, we have identified a couple of issues with the HAPI FHIR ValueSet expansion:

  1. The existing implementation of "is-a" (see
    b.must(f.match().field("myParentPids").matching("" + code.getId()));
    ) is not correct. The validation rejects the actual code pointed to by the "is-a" definition although is should be allowed (along with its "child nodes" for that code). Actually, this is a correct implementation of the "descendent-of", which does not include the actual "root node" being pointed to. For definitions of "is-a" and "descendent-of" see https://build.fhir.org/valueset-filter-operator.html
  2. For ValueSet expansion "descendent-of" is not supported. When ValueSet expansion runs, the error "HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept" happens.

So far we have solved the problem locally using a classpath override of the TermReadSvcImpl class, but we would like to avoid that going forward.

@ohetrifork ohetrifork marked this pull request as draft January 19, 2024 13:01
ohetrifork and others added 7 commits January 22, 2024 08:31
This reverts commit 46c672b.
* fix default partition setting on resource

* changelog

* Handle DEFAULT partition in rule checker.

* Fix spotless

---------

Co-authored-by: Michael Buckley <michaelabuckley@gmail.com>
Co-authored-by: James Agnew <jamesagnew@gmail.com>
Co-authored-by: Long Ma <long@smilecdr.com>
* fix default partition setting on resource
* Handle DEFAULT partition in rule checker.

Co-authored-by: Ken Stevens <khstevens@gmail.com>
…5611)

* Add setting to make broker not use JacksonMessageConverter

* Add changelog

* Implement suggestions

---------

Co-authored-by: juan.marchionatto <juan.marchionatto@smilecdr.com>
@tadgh tadgh marked this pull request as ready for review February 2, 2024 17:37
@tadgh tadgh self-requested a review February 2, 2024 17:37
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.

Looks good to me. Going to have a few more people internally have a look, but its likely we can get this merged for 7.0.0. Can you please re-target this MR to rel_7_0 branch?

@tadgh
Copy link
Collaborator

tadgh commented Feb 3, 2024

Failures here:

12:45:15,091 [ERROR] Failures: 
12:45:15,091 [ERROR]   FhirResourceDaoDstu3TerminologyTest.testExpandWithIsAInExternalValueSet:507 
Expected: iterable with items ["childAA", "childAAA", "childAAB"] in any order
     but: no item matches: "childAA" in ["childAAA", "childAAB"]
12:45:15,091 [ERROR]   FhirResourceDaoDstu3TerminologyTest.testExpandWithIsAInExternalValueSetReindex:538 
Expected: iterable with items ["childAA", "childAAA", "childAAB"] in any order
     but: no item matches: "childAA" in ["childAAA", "childAAB"]
12:45:15,091 [ERROR] Errors: 
12:45:15,091 [ERROR]   FhirResourceDaoDstu3TerminologyTest.testExpandWithCodesAndDisplayFilterBlank:321 » InvalidRequest HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept
12:45:15,091 [ERROR]   FhirResourceDaoDstu3TerminologyTest.testIndexingIsDeferredForLargeCodeSystems:655 » InvalidRequest HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept
12:45:15,091 [ERROR]   ResourceProviderCustomSearchParamDstu3Test.testCreatingParamMarksCorrectResourcesForReindexing:204->BaseJpaTest.runInTransaction:591->lambda$testCreatingParamMarksCorrectResourcesForReindexing$0:205 » CannotCreateTransaction Could not open JPA EntityManager for transaction
12:45:15,091 [ERROR]   ResourceProviderDstu3ValueSetVersionedTest.testExpandInlineVsAgainstExternalCs:810 » InvalidRequest HTTP 400 Bad Request: HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept
12:45:15,091 [ERROR]   ResourceProviderDstu3ValueSetVersionedTest.testExpandLocalVsAgainstExternalCs:850 » InvalidRequest HTTP 400 Bad Request: HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept
12:45:15,091 [ERROR]   ResourceProviderDstu3ValueSetVersionedTest.testExpandLocalVsCanonicalAgainstExternalCs:889 » InvalidRequest HTTP 400 Bad Request: HAPI-0894: Don't know how to handle op=DESCENDENTOF on property concept
12:45:15,091 [INFO] 

@ohetrifork ohetrifork changed the base branch from master to rel_7_0 February 5, 2024 07:23
@tadgh tadgh mentioned this pull request Feb 5, 2024
@tadgh
Copy link
Collaborator

tadgh commented Feb 5, 2024

@ohetrifork I have cloned your work and made a repo-local copy(#5669) just for ease of merging as i deal with conflicts and spotless resolution. I have also attributed you in the changelog, and in our developers section. Please let me know if you do not want this, and I can remove your attribution. Otherwise I will get this merged ASAP

@tadgh
Copy link
Collaborator

tadgh commented Feb 6, 2024

Closing this as the other has merged.

@tadgh tadgh closed this Feb 6, 2024
@ohetrifork
Copy link
Contributor Author

Cool @tadgh, thanks for helping out

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.

6 participants