org.hl7.fhir.core
org.hl7.fhir.core copied to clipboard
Review usage of Resource.getId()
This issue (#766) revealed that Resource.getId() returns version history as well as the logical ID.
For example: 1234/_history/1
There may be cases where this is not the expected response, and instead, the logical part of the ID is actually needed (1234
in the above example).
Existing uses of getId() should be evaluated to verify that they are in fact meant to use the whole ID, including history.
This review should cover all modules:
- [ ] org.hl7.fhir.convertors
- [ ] org.hl7.fhir.dstu2
- [ ] org.hl7.fhir.dstu3
- [ ] org.hl7.fhir.dstu2016may
- [ ] org.hl7.fhir.r4
- [ ] org.hl7.fhir.r4b
- [ ] org.hl7.fhir.r5
- [ ] org.hl7.fhir.utilities
- [ ] org.hl7.fhir.validation
org.hl7.fhir.convertors
~~ExtensionDefinitionGenerator~~
~~In ExtensionDefinitionGenerator we're using the ID of a StructureDefinition. I don't expect these will ever have history in the same manner as a FHIR resource:~~
https://github.com/hapifhir/org.hl7.fhir.core/blob/23efd7ba335f935f37015bd928ddcdd9fca9de5b/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/ExtensionDefinitionGenerator.java#L178
~~This line in particular would create an invalid file if history was included:~~
https://github.com/hapifhir/org.hl7.fhir.core/blob/23efd7ba335f935f37015bd928ddcdd9fca9de5b/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/ExtensionDefinitionGenerator.java#L373
~~ExtensionDefinitionGenerator has no unit tests, and should probably get some coverage if we're changing to getIdPart()~~
~~R2R3ConversionManager~~
~~This file could be a source of trouble here:~~ https://github.com/hapifhir/org.hl7.fhir.core/blob/23efd7ba335f935f37015bd928ddcdd9fca9de5b/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/R2R3ConversionManager.java#L345
~~However, this method is never called inside R2R3ConversionManager and I don't believe R2R3ConversionManager is actually used anywhere else in the codebase. Is it being referenced as a command-line run in another project perhaps?~~
~~SpecDifferenceEvaluator~~
~~This is another instance only apparently referenced by run in command-line.~~
~~It appears to only load published builds, which I imagine wouldn't contain any history. In addition, it uses getId() to generate paths, which would cause problems when /
characters are used.~~
After discussion with @grahamegrieve, the above were not in use, and not a priority for change.
Contents of org.hl7.fhir.convertors.advisors + org.hl7.fhir.convertors.convXX_YY
There are essentially two types of call to getId() in these packages. They are either creating a new id using the content of an existing id, as it is here:
https://github.com/hapifhir/org.hl7.fhir.core/blob/58c0e216e8a2d58619c3fcdd458091b4313ea04f/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/conv10_30/datatypes10_30/Element10_30.java#L61
Or, getId() is being used for evaluating type, as it is here:
https://github.com/hapifhir/org.hl7.fhir.core/blob/58c0e216e8a2d58619c3fcdd458091b4313ea04f/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/conv10_30/resources10_30/StructureDefinition10_30.java#L197
Should history be conserved in the above cases? I imagine in the first, it may be, but I don't believe it would apply in the second.
org.hl7.fhir.convertors (cont.)
org.hl7.fhir.convertors.misc
~~DicomPackageBuilder~~
- ~~self-contained~~
- ~~saves files based on getId()... probably not intended for history~~
~~GenValueSetExpansionConvertor~~
- ~~self-contained~~
- ~~saves files based on getId()... probably not intended for history~~
~~IEEE11073Convertor~~
- ~~self-contained~~
- ~~saves files based on getId()... probably not intended for history~~
After discussion with @grahamegrieve, the above were not in use, and not a priority for change.
IGR2ConvertorAdvisor
- setting CodeSystem id to ValueSet id. This is used only in ExtensionDefinitionGenerator, and in tests, for some reason
IGR2ConvertorAdvisor5
- setting CodeSystem id to ValueSet id. This is used in IGLoader and ValidationEngine
After discussion with @grahamegrieve, the below were not in use, and not a priority for change.
~~ObservationStatsBuilder~~
- ~~self-contained~~
- ~~builds Observation urls from ids; seems like a valid usage for history~~
~~PackagesPreparer~~
- ~~self-contained~~
- ~~saves files based on getId()... probably not intended for history~~
~~PhinVadsImporter~~
- ~~self-contained~~
- ~~saves files based on getId()... probably not intended for history~~
- ~~references resources based in getId(), apparently canonical ones~~
~~org.hl7.fhir.convertors.misc.argonaut~~
~~ArgonautConvertor~~
- ~~Self-contained~~
- ~~Uses getId() to generate file names.~~
- ~~Uses getId() to generate references (this should be OK. References can use history)~~
~~org.hl7.fhir.convertors.misc.iso21090~~
~~ISO21090Importer~~
- ~~Self-contained~~
- ~~Generates ValueSet and StructureDefinition using getId() to generate URL. Likely not something we want _history for.~~
~~org.hl7.fhir.convertors.misc.searchparam~~
~~SearchParameterProcessor~~
- ~~Self-contained~~
- ~~Generates ConceptMap using getId() to generate URL. Likely not something we want _history for.~~
After discussion with @grahamegrieve, the above are not in use, and not a priority for change.
org.hl7.fhir.dstu2.model
ExpressionNode
- Used in evaluating Type... likely not _history
org.hl7.fhir.dstu2.test
FluentPathTests
- Disabled. There's a TODO there.
- Seems to only apply to a StructureDefinition. Likely not _history
org.hl7.fhir.dstu2.utils
BatchLoader
- Uses getId() to compose target URL for PUT. This id should NOT contain history, as it is being used to update a resource. In HAPI, this would result in: "HAPI-0550: HAPI-0989: Trying to update Patient/john/_history/1 but this is not the current version" or something similar, or would allow the alteration of history in a lesser implementation, which would be bad.
FhirPathEngine
- Uses getId() from StructureDefinition, and uses it to match against ElementDefinition. Since the path in ElementDefinition is meant to begin with the name of the resource or extension, this should not use _history by definition.
NarrativeGenerator
- Wraps getId() in ResourceWrapper, which is used for URL construction of references at line 3024. Since references CAN contain history, this should continue to use getId()
- 3024 contains a search of the Bundle feed for a particular resource by ID. If the getId() contains the full history, identical to the reference, this will work.
ProfileUtilities
- Uses getId() from StructureDefinition. Likely not _history
- Majority of methods aren't used. JavaDoc no longer seems to accurately desribe contents.
QuestionnaireBuilder
- Uses getId() from Questionnaire to compose Reference URL. References CAN contain _history.
- Uses getId() from ValueSet. These should be without _history, but we should ask James about this.
ResourceUtilities
- getById() Uses getId() to find resources matching a reference in a bundle. This
- Single use of getId() to generate a URL for a DataElement resource. This would be broken for a full url in the getId()
SimpleWorkerContext
- Caches StructureDefinitions and ValueSets in memory using both URL and getId().
Unbundler
- Composes a ValueSet id from the portion of the URL after the last /. This means, not using _history. Also, this generates files based on getId() utils.client
FHIRToolingClient
- Generates PUT url based on getId() and should probably use getIdPart(). See comments on BatchLoader
Contents of org.hl7.fhir.convertors.advisors + org.hl7.fhir.convertors.convXX_YY
There are essentially two types of call to getId() in these packages. They are either creating a new id using the content of an existing id, as it is here:
https://github.com/hapifhir/org.hl7.fhir.core/blob/58c0e216e8a2d58619c3fcdd458091b4313ea04f/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/conv10_30/datatypes10_30/Element10_30.java#L61
Or, getId() is being used for evaluating type, as it is here:
https://github.com/hapifhir/org.hl7.fhir.core/blob/58c0e216e8a2d58619c3fcdd458091b4313ea04f/org.hl7.fhir.convertors/src/main/java/org/hl7/fhir/convertors/conv10_30/resources10_30/StructureDefinition10_30.java#L197
Should history be conserved in the above cases? I imagine in the first, it may be, but I don't believe it would apply in the second.
After discussion with @grahamegrieve, ValueSets should be using getIdPart(). The initial commits of PR #799 contain code changes to address this in 10_40; if approved, they should be applied to all conversions.
While reviewing other parts of the code and testing some things out in HAPI-FHIR, I noticed that our GraphQL output still appears to be wrong. We are finding the right resource, but the GraphQL output is including the history string in the ID, which is contrary to what a resource's output would look like:
{
"id":"example/_history/1",
"ConditionList":[{
"id":"example"
}]
}
The "id" should conform to the expectations and be "id": "example"
Similar to the above, if I add a FHIRPath test, I also get the full name returned
If I add the below added to tests-fhir-r5.xml and manually set the id to contain history with res.setId(res.getIdPart() + "/_history/1");
, the full history gets returned in the result.
<test name="testId" inputfile="patient-example.xml">
<expression>id</expression>
<output type="id">example</output>
</test>
org.opentest4j.AssertionFailedError: Outcome 0: Value should be example but was example/_history/1 for expression id ==>
Expected :example
Actual :example/_history/1
org.hl7.fhir.dstu3
====================
org.hl7.fhir.dstu3.conformance
ProfileUtilities
- see dstu2 org.hl7.fhir.dstu2.utils.ProfileUtilities
- has fewer unused methods
ShExGenerator
- see dstu2
- more methods, but all use StructureDefinition
org.hl7.fhir.dstu3.context
BaseWorkerContext
- see dstu2 SimpleWorkerContext
- This expands cacheing to include:
- ExtensionDefinition
- Questionnaire
- Operation
- ValueSet
- Profile (StructureDefinition)
- CodeSystem
SimpleWorkerContext
- see BaseWorkerContext. More of the same.
org.hl7.fhir.dstu3.elementmodel
Property
- see dstu2 org.hl7.fhir.dstu2016may.metamodel.Property
org.hl7.fhir.dstu3.test
SnapShotGenerationTestsContext
- test is disabled.
- Checks for resources contained in the TestScript via id. Could use _history, so should be OK to keep as getId()
org.hl7.fhir.dstu3.utils
BatchLoader
- see dstu2
FHIRPathEngine
- see dstu2
GraphQLEngine
- already done in previous PR
NarrativeGenerator
- Uses primarily CodeSystem getId in the generation of narratives. It seems unlikely that _history would be involved here.
NarrativeGenerator.ResourceWrapperDirect
- see dstu2
- generate uses a ResourceDefinition getId().
QuestionnaireBuilder
- see dstu2
R3TEchnicalCorrectionProcessor
- Saves processed resources to files based on getId(). Should not include _history elements.
ResourceUtilities
- see dstu2
StructureMapUtilities
- produceStructureMap is building FHIR mapping langauge. This should likely be using getIdPart()
- getActualType is selecting type from StructureDefinition, and should likely use getIdPart()
- runTransform - see dstu2
- translate - see dstu2
- updateProfile uses StructureDefinition, and should likely use getIdPart()
- createProfile uses StructureDefinition, and should likely use getIdPart()
- resolveType uses StructureDefinition, and should likely use getIdPart()
Unbundler
- see dstu2
org.hl7.fhir.dstu3.utils.client
FHIRToolingClient
- see dstu2
org.hl7.fhir.r4
=================
org.hl7.fhir.r4.conformance
ProfileUtilities
- see dstu3
ShExGenerator
- see dstu3
org.hl7.fhir.r4.context
BaseWorkerContext
- see dstu3 SimpleWorkerContext
CanonicalResourceManager
- Caches canonical resources. Should likely use getIdPart()
org.hl7.fhir.r4.elementmodel
Property
- see dstu3
org.fl7.fhir.r4.test
GraphQLEngineTests
- already done in previous PR
SnapShotGenerationTests.TestPKP
- test is disabled.
- generates path on local drive using getId() should use getIdPart()
org.fl7.fhir.r4.utils
BatchLoader
- see dstu2
FHIRPathEngine
- see dstu2
GraphQLEngine
- already done in previous PR
NarrativeGenerator
- addRefToCode uses a reference to a CodeSystem. This will likely not use the full getId()
- generate uses a ResourceDefinition getId().
QuestionnaireBuilder
- see dstu2
ResourceUtilities
- see dstu2
StructureMapUtilities
- see dstu2
- the code in translate could be refactored to be more efficient.
Unbundler
- see dstu2
org.hl7.fhir.r4.utils.client
FHIRToolingClient
- see dstu2
org.hl7.fhir.r4b
==================
org.hl7.fhir.r4b.comparison
CanonicalResourceComparison
- compares CanonicalResources via getId(). Could likely use getIdPart()
ComparisonRenderer
- uses getId() from CodeSystem and ValueSet. Could likely use getIdPart()
ComparisonSession
- Generates what appears to be a canonical comparison url via getId(). Could likely use getIdPart() ResourceComparer.ResourceComparison
- generates table cells via CanonicalResource getId(). Could likely use getIdPart()
org.hl7.fhir.r4b.conformance
ProfileUtilities
- see dstu3
ShExGenerator
- see dstu3
org.hl7.fhir.r4b.context
BaseWorkerContext
- see dstu3 SimpleWorkerContext
CanonicalResourceManager.CachedCanonicalResource
- Wrapper for Cacehed canonical resources. Should likely use getIdPart()
org.hl7.fhir.elementmodel
Property
- see dstu3
org.hl7.fhir.r4b.model
DomainResource
- finds contained resource via getId(). Should continue to do so.
org.hl7.fhir.r4b.patterns
PatternBaseImpl
- wrapper for resource getId().
- code is only referenced in another unused class, ActivityDefinitionDefinitionImpl
org.hl7.fhir.r4b.renderers
BinaryRenderer
- uses getId to write out to a file. Cannot use getId() with history
- never used?
BundleRenderer
- Uses getId in place of type. Likely should be getIdPart()
CodeSystemRenderer
- Uses getId for a CodeSystem-value combo. Likely should be getIdPart()
LibraryRenderer
- uses getId to write out to a file. For attachments. Cannot use getId() with history
ParametersRenderer
- delegates a table cell to ResourceRenderer.
ProfileDrivenRenderer
- Uses getId() in rendering a URI. Should likely continue to do so.
QuestionnaireRenderer
- Uses getId() for internal xhtml element ids. OK.
- renderLinks generates a link item that points to http://todo.nlm.gov, which is unavailable. Since this is an internal link to package contents, is history applicable?
QuestionnareResponseRenderer
- Uses getId() for internal xhtml element ids. OK.
- renderLinks generates a link item that points to http://todo.nlm.gov, which is unavailable. Since this is an internal link to package contents, is history applicable?
ResourceRenderer
- Generates bundleUrl with getId(), which is used as a key for ResourceWithReference which is internally used to resolve # references in a bundle. dstu2 NarrativeGenerator comments may apply too. Seems legal to use the full Id here, or else how would we resolve multiple elements in the bundle?
StructureDefinitionRenderer
- Uses getId() for internal xhtml element ids. OK.
ValueSetRenderer
- Uses getId for a ValueSet-value combo. Likely should be getIdPart()
org.hl7.fhir.r4b.renderers.utils
DirectWrappers.ResourceWrapperDirect
- Wraps getId() with a direct call.
- Could benefit from a getIdPart() wrapping as well?
Resolver.ResourceContext
- Uses getId() to find a resource in a context. Should likely use getId()
org.hl7.fhir.r4b.test
CanonicalResourceManagerTests.DeferredLoadTestResource
- Appears to only refer to Canonical resources. Should likely use getIdPart()
GraphQLEngineTests
- Covered in previous PR
SnapShotGenerationTests.TestPKP
- getLinkForProfile uses StructureDefinition, and should likely use getIdPart()
org.hl7.fhir.r4b.utils
BatchLoader
- see dstu2
FHIRPathEngine
- see dstu2
GraphDefinitionEngine
- uses getId() for referencing bundle resources. Should continue to use getId().
GraphQLEngine
- already done in previous PR
QuestionnaireBuilder
- see dstu2
ResourceFixer
- visitResource is checking for duplicate resources. If history is included in this check, it should remain getId()
- visitCodeSystem only provides System.out referring to getId(). This is for codeSystem, so should likely be getIdPart()
ResourceSorters
- compare method is comparing two canonical resources, which should use getIdPart()
ResourceUtilities
- see dstu2
Unbundler
- see dstu2
org.hl7.fhir.r4b.utils.client
FHIRToolingClient
- see dstu2
org.hl7.fhir.r4b.utils.structuremap
StructureMapUtilities
- see org.hl7.fhir.r4.utils.StructureMapUtilities
ValidationContextCarrier
- loads a contained resource. Should continue to use getId()
org.hl7.fhir.r5
=================
org.hl7.fhir.r5.comparison
CanonicalResourceComparer
- see r4b
ComparisonRenderer
- see r4b
ComparisonSession
- see r4b
ResourceComparer
- see r4b
org.hl7.fhir.r5.conformance
ProfileUtilities
- see dstu3
ShExGenerator
- see dstu3
org.hl7.fhir.r5.context
BaseWorkerContext
- see dstu3
CanonicalResourceManager
- see r4b
org.hl7.fhir.r5.elementmodel
Property
- see dstu3
org.hl7.fhir.r5.model
DomainResource
- see r4b
org.hl7.fhir.r5.patterns
PatternBaseImpl
- see r4b
org.hl7.fhir.r5.renderers
BinaryRenderer
- see r4b
BundleRenderer
- see r4b
CodeSystemRenderer
- see r4b
LibraryRenderer
- see r4b
ParametersRenderer
- see r4b
ProfileDrivenRenderer
- see r4b
QuestionnaireRenderer
- see r4b
QuestionnaireResponseRenderer
- see r4b
ResourceRenderer
- see r4b
StructureDefinitionRenderer
- see r4b
ValueSetRenderer
- see r4b
org.hl7.fhir.r5.renderers.utils
DirectWrappers
- see r4b
Resolver
- see r4b
org.hl7.fhir.r5.test
CanonicalResourceManagerTests
- see r4b
GraphQLEngineTests
- Covered in previous PR
SnapshotGenerationTests
- see r4b
org.hl7.fhir.r5.utils
BatchLoader
- see dstu2
FHIRPathEngine
- see dstu2
GraphDefinitionEngine
- see r4b
GraphQLEngine
- already done in previous PR
QuestionnaireBuilder
- see dstu2
ResourceFixer
- see r4b
ResourceSorters
- see r4b
ResourceUtilities
- see dstu2
Unbundler
- see dstu2
org.hl7.fhir.utils.client
FHIRToolingClient
- see dstu2
org.hl7.fhir.utils.structuremap
StructureMapUtilities
- see org.hl7.fhir.r4.utils.StructureMapUtilities
org.hl7.fhir.utils.validation
ValidationContextCarrier
- see r4b
org.hl7.fhir.validation
=========================
org.hl7.fhir.conversion.tests
R3R4ConversionTests
- test(String, byte[]) writes to filenames based on getId(). Not failing yet, but could cause trouble with _history.
- otherwise getId() is used to resolve contained references and should remain getId()
- this may be a good place to check if getId(), history, and filenames break anything
org.hl7.fhir.validation
SnapShotGenerationXTests.TestPKP
- see org.hl7.fhir.r4b.SnapShotGenerationTests
BaseValidator
- resolveBindingReference uses getId() to pull a ValueSet from contained resources. Should use getIdPart()
- getXVerExt uses getId() for StructureDefinitions from http://hl7.org/fhir/. This should not use history, so should use getIdPart()
org.hl7.fhir.validation.codesystem
CodeSystemValidator
- Checks codes. Should use getIdPart()
org.hl7.fhir.validation.instance
InstanceValidator
- resolveProfiles gets a StructureDefinition from contained resources. Should use getIdPart()?
- other getId() calls are referring to StructureDefinitions as well. Should use getIdPart()?
InstanceVaslidator.ValidatorHostServices
- resolves contained ValueSet. Should use getIdPart()
org.hl7.fhir.validation.profile
ProfileValidator
- Uses StructureDefinition getId(). Should use getIdPart()?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.