org.hl7.fhir.core icon indicating copy to clipboard operation
org.hl7.fhir.core copied to clipboard

Review usage of Resource.getId()

Open dotasek opened this issue 2 years ago • 12 comments

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

dotasek avatar Apr 22 '22 15:04 dotasek

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.

dotasek avatar Apr 22 '22 17:04 dotasek

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~~

dotasek avatar Apr 22 '22 21:04 dotasek

~~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.

dotasek avatar Apr 26 '22 17:04 dotasek

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

dotasek avatar Apr 26 '22 21:04 dotasek

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.

dotasek avatar Apr 27 '22 15:04 dotasek

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"

dotasek avatar Apr 29 '22 21:04 dotasek

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

dotasek avatar Apr 29 '22 21:04 dotasek

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

dotasek avatar May 03 '22 19:05 dotasek

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

dotasek avatar May 03 '22 19:05 dotasek

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()

dotasek avatar May 06 '22 15:05 dotasek

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

dotasek avatar May 06 '22 15:05 dotasek

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()?

dotasek avatar May 06 '22 15:05 dotasek

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.

stale[bot] avatar Nov 18 '22 05:11 stale[bot]