Validate measure library URL before CQL evaluation #768
Conversation
…e library if the URL from the measure is in conflict.
|
Formatting check succeeded! |
lukedegruchy
changed the title
…ing-enforcement-with-library
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #768 +/- ##
============================================
- Coverage 73.01% 72.93% -0.08%
Complexity 235 235
============================================
Files 538 539 +1
Lines 25170 25237 +67
Branches 3190 3207 +17
============================================
+ Hits 18377 18406 +29
- Misses 5270 5300 +30
- Partials 1523 1531 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tential error messages but otherwise leave the logic the same as in master.
There was a problem hiding this comment.
It seems to me that we ought to be using this class:
There are some tricky edge cases in dealing with canonical references, and this class wraps them all up nicely, we shouldn't be re-inventing that logic here, just re-use what's in the Canonicals class, yes?
…R4VersionedLibraryIdentifierUtils and add system to Canonicals.
lukedegruchy
changed the title
…ing-enforcement-with-library
| String url = getUrl(canonical); | ||
| String id = getIdPart(canonical); | ||
| String resourceType = getResourceType(canonical); | ||
| String system = getSystem(canonical, resourceType); |
There was a problem hiding this comment.
I suggest that we call this "canonicalBase", that's the term that is used to describe this portion of a FHIR canonical URL, whereas the term "system" typically means the canonical URL of a naming or code system.
For a VersionedIdentifier, we are using the "canonicalBase" of the FHIR URL as the "system" for our identifiers, so when we talk about it in VersionIdentifier, system is the right term, but here in the general utilities for dealing with FHIR canonical URLs, it's the canonicalBase.
| checkNotNull(canonical); | ||
|
|
||
| String url = getUrl(canonical); | ||
| String id = getIdPart(canonical); |
There was a problem hiding this comment.
While we're in here making changes, I strongly recommend that we change the name of this part to "tail", not "id" to help avoid the almost universal confusion between FHIR ids and FHIR canonical urls.
I suggest that we deprecate getIdPart() and that we introduce getTail()
| } | ||
|
|
||
| @Nonnull | ||
| private static Library validateBundle( |
There was a problem hiding this comment.
So, I'm not really sure what to do here because this validation will in general fail in ways that it shouldn't because it's not doing version resolution, and I really don't know that we should be doing version resolution here as part of validation.
If you have a version-less reference from a measure, it's not wrong for there to be multiple versions of that library on the server, the version-less reference should resolve to the latest.
If you have a versioned reference from a measure, then it should only resolve to that version of the library.
But.... putting that logic here means that we're making decisions about how to do version selection as part of this validation. Seems wrong to me.
I think all we ought to be saying here is:
- If the reference from the measure is version less, then any library that matches that url, regardless of version, is valid.
- If the reference from the measure is versioned, then only a library matching that url and version is valid.
There was a problem hiding this comment.
But then... in the case of a versionless reference do you validate each possible library and make sure that it's name matches it's URL?
|
Closes #766