Skip to content
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.

Sign up for GitHub

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

Fish With Version #1275

Merged
merged 15 commits into from
May 19, 2023
Merged

Fish With Version #1275

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
900e6fc
Support fishing with a version
jafeltra May 8, 2023
19ab78e
Add tests and minor tweeks around fishing with versions in FSH Tank a…
jafeltra May 10, 2023
4e871d3
Only append version if not already provided in URL and add test to ch…
jafeltra May 10, 2023
ff23abe
Fish with version in Package
jafeltra May 10, 2023
4780f60
Add back version if fished up a value set
jafeltra May 11, 2023
3e8be48
Use FPL v0.3.0
jafeltra May 15, 2023
2966e53
Merge branch 'master' into fpl-fishes-with-version
jafeltra May 17, 2023
11d0cfa
Merge branch 'master' into fpl-fishes-with-version
jafeltra May 17, 2023
9668cdc
Simplify check for matching version on instances
jafeltra May 17, 2023
70e31fc
Simplify places where version needs to be appended to a fished up url
jafeltra May 17, 2023
0d0852d
Invariants, Mappings, RuleSets won't have a version so don't need to …
jafeltra May 17, 2023
18b8ba8
Simplify warning message when incorrect version is fished up and simp…
jafeltra May 17, 2023
3a9f3e9
Assigning a code with an invalid code system should only warn when as…
cmoesel May 18, 2023
858c75d
Log source info for mismatched version when fishing for best version
cmoesel May 19, 2023
0b7ae4d
Remove unnecessary import
cmoesel May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions npm-shrinkwrap.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"chalk": "^3.0.0",
"commander": "^8.2.0",
"fhir": "^4.9.0",
"fhir-package-loader": "^0.2.0",
"fhir-package-loader": "^0.3.0",
"fs-extra": "^8.1.0",
"html-minifier-terser": "5.1.1",
"https-proxy-agent": "^5.0.0",
Expand Down
64 changes: 51 additions & 13 deletions src/export/Package.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,85 +30,123 @@ export class Package implements Fishable {
];
}

// version needs to be checked separately from the base
const [base, ...versionParts] = item?.split('|') ?? ['', ''];
const version = versionParts.join('|') || null;

for (const type of types) {
let def;
switch (type) {
case Type.Profile:
def = this.profiles.find(p => p.id === item || p.name === item || p.url === item);
def = this.profiles.find(
p =>
(p.id === base || p.name === base || p.url === base) &&
(version == null || p.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'StructureDefinition' &&
i.derivation === 'constraint' &&
i.type !== 'Extension' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
cmoesel marked this conversation as resolved.
Show resolved Hide resolved
);
}
break;
case Type.Extension:
def = this.extensions.find(e => e.id === item || e.name === item || e.url === item);
def = this.extensions.find(
e =>
(e.id === base || e.name === base || e.url === base) &&
(version == null || e.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'StructureDefinition' &&
i.derivation === 'constraint' &&
i.type === 'Extension' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
);
}
break;
case Type.Logical:
def = this.logicals.find(e => e.id === item || e.name === item || e.url === item);
def = this.logicals.find(
l =>
(l.id === base || l.name === base || l.url === base) &&
(version == null || l.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'StructureDefinition' &&
i.derivation === 'specialization' &&
i.kind === 'logical' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
);
}
break;
case Type.Resource:
def = this.resources.find(e => e.id === item || e.name === item || e.url === item);
def = this.resources.find(
r =>
(r.id === base || r.name === base || r.url === base) &&
(version == null || r.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'StructureDefinition' &&
i.derivation === 'specialization' &&
i.kind === 'resource' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
);
}
break;
case Type.ValueSet:
def = this.valueSets.find(vs => vs.id === item || vs.name === item || vs.url === item);
def = this.valueSets.find(
vs =>
(vs.id === base || vs.name === base || vs.url === base) &&
(version == null || vs.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'ValueSet' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
);
}
break;
case Type.CodeSystem:
def = this.codeSystems.find(cs => cs.id === item || cs.name === item || cs.url === item);
def = this.codeSystems.find(
cs =>
(cs.id === base || cs.name === base || cs.url === base) &&
(version == null || cs.version === version)
);
if (!def) {
def = this.instances.find(
i =>
i._instanceMeta.usage === 'Definition' &&
i.resourceType === 'CodeSystem' &&
(i.id === item || i._instanceMeta.name === item || i.url === item)
(i.id === base || i._instanceMeta.name === base || i.url === base) &&
(version == null || (i.version && i.version === version))
);
}
break;
case Type.Instance:
def = this.instances.find(i => i.id === item || i._instanceMeta.name === item);
def = this.instances.find(
i =>
(i.id === base || i._instanceMeta.name === base) &&
(version == null || (i.version && i.version === version))
);
break;
case Type.Type: // Package doesn't currently support types
default:
Expand Down
34 changes: 23 additions & 11 deletions src/export/StructureDefinitionExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,15 @@ import {
ObeysRule,
OnlyRule
} from '../fshtypes/rules';
import { logger, Type, Fishable, Metadata, MasterFisher, resolveSoftIndexing } from '../utils';
import {
logger,
Type,
Fishable,
fishForFHIRBestVersion,
Metadata,
MasterFisher,
resolveSoftIndexing
} from '../utils';
import {
applyInsertRules,
cleanResource,
Expand Down Expand Up @@ -596,7 +604,15 @@ export class StructureDefinitionExporter implements Fishable {
const target = structDef.getReferenceOrCanonicalName(rule.path, element);
element.constrainType(rule, this, target);
} else if (rule instanceof BindingRule) {
const vsURI = this.fishForMetadata(rule.valueSet, Type.ValueSet)?.url ?? rule.valueSet;
const vsMetadata = this.fishForMetadata(rule.valueSet, Type.ValueSet);
const [, ...versionParts] = rule.valueSet.split('|');
const version = versionParts.join('|') || null;
const vsURIWithVersion = vsMetadata?.url
? `${vsMetadata?.url}${
version && !vsMetadata.url.endsWith(version) ? `|${version}` : ''
}`
: null;
const vsURI = vsURIWithVersion ?? rule.valueSet;
cmoesel marked this conversation as resolved.
Show resolved Hide resolved
const csURI = this.fishForMetadata(rule.valueSet, Type.CodeSystem)?.url;
if (csURI && !isUri(vsURI)) {
throw new MismatchedBindingTypeError(rule.valueSet, rule.path, 'ValueSet');
Expand Down Expand Up @@ -750,24 +766,20 @@ export class StructureDefinitionExporter implements Fishable {
}
rule.items.forEach(item => {
if (item.type) {
// there might be a |version appended to the type, so don't include it while fishing
const [typeWithoutVersion, version] = item.type.split('|', 2);
const extension = this.fishForFHIR(typeWithoutVersion, Type.Extension);
// there might be a |version appended to the type, so try to use the version but fall back
// to any version if necessary
const extension = fishForFHIRBestVersion(this, item.type, Type.Extension);
if (extension == null) {
logger.error(
`Cannot create ${item.name} extension; unable to locate extension definition for: ${item.type}.`,
rule.sourceInfo
);
return;
}
if (version != null && extension.version != null && version != extension.version) {
logger.warn(
`The ${typeWithoutVersion} extension was specified with version ${version}, but SUSHI found version ${extension.version}`,
rule.sourceInfo
);
}
try {
let profileUrl = extension.url;
const [, ...versionPart] = item.type.split('|');
const version = versionPart.join('|') || null;
if (version) {
profileUrl += `|${version}`;
}
cmoesel marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion src/export/ValueSetExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class ValueSetExporter {
if (component.from.system) {
const systemParts = component.from.system.split('|');
const foundSystem = (
this.fisher.fishForMetadata(systemParts[0], Type.CodeSystem)?.url ??
this.fisher.fishForMetadata(component.from.system, Type.CodeSystem)?.url ??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this change makes it more consistent w/ the approach we take for value sets below -- so that's good.

I also see that if there is a version, we drop the version when there's a match but keep it when there isn't (value sets below does the same thing). I think this is one of those places where we want to keep the version when there is a match. I think we had decided, however, that these kinds of changes aren't necessarily in scope for this PR and can be deferred to another PR if we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I have your nifty regex replace in my back pocket, I don't mind doing it here. But I can also hold off for a separate PR if that's preferred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, wait, now I'm not sure if I agree we should keep the version. The version the author specified gets added to the composeElement.version regardless of if we find a match on line 63. And because the foundSystem variable does a final split('|') and then only foundSystem[0] is used later, the version is always spliced off and never added to composeElement.system. So do we want to include the |version on the system property, as well as on the version property?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, dang. You're right! For whatever reason, ValueSet.compose.include splits the version off into its own element. And the system element is uri not canonical. So, yep, we definitely do not want to attach the |version. Good call!

component.from.system
).split('|');
composeElement.system = foundSystem[0];
Expand Down
46 changes: 27 additions & 19 deletions src/fhirtypes/ElementDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,14 @@ import {
isReferenceType,
isModifierExtension
} from './common';
import { Fishable, Type, Metadata, logger } from '../utils';
import {
Fishable,
Type,
Metadata,
logger,
fishForMetadataBestVersion,
fishForFHIRBestVersion
} from '../utils';
import { InstanceDefinition } from './InstanceDefinition';
import { idRegex } from './primitiveTypes';
import sax = require('sax');
Expand Down Expand Up @@ -1239,14 +1246,19 @@ export class ElementDefinition {
// Stop when we can't find a definition or the base definition is blank.
let currentType = type;
while (currentType != null) {
const [name, version] = currentType.split('|', 2);
const result = fisher.fishForMetadata(name);
if (result) {
if (version != null && result.version != null && result.version != version) {
logger.error(
// fishForMetadataBestVersion is not used here in order to provide additional details in the warning
let result = fisher.fishForMetadata(currentType);
if (result == null) {
const [name, ...versionParts] = currentType.split('|');
const version = versionParts.join('|') || null;
result = fisher.fishForMetadata(name);
if (result && version != null && result.version != null && result.version != version) {
logger.warn(
`${type} is based on ${name} version ${version}, but SUSHI found version ${result.version}`
);
}
}
if (result) {
results.push(result);
}
currentType = result?.parent;
Expand Down Expand Up @@ -1855,9 +1867,9 @@ export class ElementDefinition {
if (sdType && this.type[0].targetProfile) {
const validTypes: string[] = [];
this.type[0].targetProfile.forEach(tp => {
// target profile may have a version after a | character. don't include it when fishing.
const unversionedProfile = tp.split('|', 2)[0];
const tpType = fisher.fishForMetadata(unversionedProfile)?.sdType;
// target profile may have a version after a | character.
// fish for matching version, but fall back to any version if necessary.
const tpType = fishForMetadataBestVersion(fisher, tp)?.sdType;
if (tpType) {
validTypes.push(tpType);
}
Expand Down Expand Up @@ -2090,7 +2102,7 @@ export class ElementDefinition {
private assignFshCode(code: FshCode, exactly = false, fisher?: Fishable): void {
if (code.system) {
const csURI = code.system.split('|')[0];
const vsURI = fisher?.fishForMetadata(code.system, Type.ValueSet)?.url ?? '';
const vsURI = fishForMetadataBestVersion(fisher, code.system, Type.ValueSet)?.url ?? '';
if (vsURI) {
throw new MismatchedBindingTypeError(code.system, this.path, 'CodeSystem');
} else if (!isUri(csURI)) {
Expand Down Expand Up @@ -2313,20 +2325,16 @@ export class ElementDefinition {
if (newElements.length === 0) {
// If we have exactly one profile to use, use that, otherwise use the code
const type = profileToUse ?? this.type[0].code;
// There could possibly be a |version appended to the type, so don't include it while fishing
const [typeWithoutVersion, version] = type.split('|', 2);
let json = fisher.fishForFHIR(
typeWithoutVersion,
// There could possibly be a |version appended to the type, so try to fish
// for that version but fall back to any version if necessary
let json = fishForFHIRBestVersion(
fisher,
type,
Type.Resource,
Type.Type,
Type.Profile,
Type.Extension
);
if (json && version != null && json.version != null && json.version !== version) {
logger.warn(
`${type} is based on ${typeWithoutVersion} version ${version}, but SUSHI found version ${json.version}`
);
}
if (!json && profileToUse) {
logger.warn(
`SUSHI tried to find profile ${type} but could not find it and instead will try to use ${this.type[0].code}`
Expand Down
Loading