Errors splitting canonical in URL and version

[Boereck]

Hi,

We tried to validate a CodeSystem with a version in the supplements canonical and the validator complained about all codes in that supplement. The validator claimed that the codes are not in the supplemented CodeSystem, which is wrong.

The root cause is that the bodys of methods org.hl7.fhir.validation.BaseValidator#versionFromCanonical and org.hl7.fhir.validation.BaseValidator#systemFromCanonical are swapped.

We found a copy of those methods in org.hl7.fhir.r4b.renderers.DataRenderer as well (which are wrong respectively).

The logic is implemented correctly in org.hl7.fhir.utilities.CanonicalPair. By the way, there is also an implementation of the logic over in the hapi-fhir repository, which should be a tiny bit more efficient.

I, personally, would try to use org.hl7.fhir.utilities.CanonicalPair everywhere, where a canonical is needed to be split into URL and version. First here, and then in the hapi-fhir repository as well. This would produce one more object allocation, however it looks like an easy case for the JIT to optimize away.

If it is of help, I could open a PR with the proposed changes in this repository.

Best Regards,
Max

[grahamegrieve]

@jamesagnew would we consider a change of that magnitude?

@Boereck we need a worked test case

[Boereck]

I realized that there are way, way more places in the code where canonicals are split into URL and version. This would be a herculean task, if test cases shall be provided for each of those changes.

I could start with a PR replacing just the faulty methods with the use of CanonicalPair with test cases for these changes. I need some time, however, to find out how to test these changes in the best way. When looking into the other places where CanonicalPair could be used, I realized that a few more helper methods (around the availability of version) on that class would be nice. I could include them (with test cases) in that PR as well.

Further changes to replace splitting in other places could be done bit by bit (maybe module by module). I think this would make the PRs also easier to review.

[grahamegrieve]

I'm not sure how extensive the changes we'd consider. Just let's start with a worked test case for the supplement issue. In the test-cases repository, a validation test case

[Boereck]

Alright. Do you mean a new test case in org.hl7.fhir.validation.ResourceValidationTests with a new resource in org.hl7.fhir.r4/src/test/resources? Or is there another testing harness that I did not find? Sorry, I am new to this repository and all of it's bells an whistles.

[Boereck]

Ah, I think I get it: The tests are loaded from the fhir-test-cases dependency, and the according repository is https://github.com/FHIR/fhir-test-cases/. Should I add the example to the r5 folder only, or also for r4?

[grahamegrieve]

Just R5

[Boereck]

I created PR #1614, just with the changes to make the validation work for CodeSystem.supplement canonical with a version.

[Boereck]

Hello, I realized that the commit message of 5a9ef0b said that the supplement version test case was moved to a normal test case. However, I only see that the test case was removed from ResourceValidationTests. I don't see the test case where added to a different file. @grahamegrieve , did you forget to commit the file it was added to? Or was the test moved to a completely different place?

[grahamegrieve]

It was moved to the test-cases repo, which is where those type of tests belong

[Boereck]

I see, it is then picked up by org.hl7.fhir.validation.tests.ValidationTests in this repository. Thanks for this insight!

[Boereck]

All actual errors in the code we've found are taken care of in #1614 and #1663 . There is still a lot of duplication of code where the splitting logic of canonicals into url and version is implemented again and again. As stated in the comments of this issue, changing this would be a big change, and big changes are risky. Maybe this can be tackled piece by piece when code in a file has to be changed anyway, following the "boy scout rule": Leave the code cleaner than you found it.

Since the bugs related to url splitting are now all fixed, I close this issue.

If there is still interest in dedicated PRs replacing duplicate canonical splitting code by the usage of CanonicalPair, let me know. I could also split the canges into smaller PRs, if desired.