-
Notifications
You must be signed in to change notification settings - Fork 156
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
Errors splitting canonical in URL and version #1611
Comments
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. This fixes hapifhir#1611
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. This fixes hapifhir#1611
@jamesagnew would we consider a change of that magnitude? @Boereck we need a worked test case |
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 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. |
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 |
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
Alright. Do you mean a new test case in |
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
Ah, I think I get it: The tests are loaded from the |
Just R5 |
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
I created PR #1614, just with the changes to make the validation work for CodeSystem.supplement canonical with a version. |
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of hapifhir#1611 (not replacing all canonical splits in other places of the code).
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 |
It was moved to the test-cases repo, which is where those type of tests belong |
I see, it is then picked up by |
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 |
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
andorg.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 thehapi-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 thehapi-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
The text was updated successfully, but these errors were encountered: