URL Encode Cross-Version Extension URLs (#1549)

* Correctly encode brackets in cross-version extensions

For paths that include [x], the brackets should be URL-encoded in the url elements and, as a result, also encoded in the corresponding element ids and slice names. See: https://chat.fhir.org/#narrow/channel/215610-shorthand/topic/Issue.20with.20polymorphic.20inter-version.20extensions/near/488766521

* Log invalid path syntax due to unmatched brackets

* Fix incorrect regex in code comment

src/fhirdefs/impliedExtensions.ts

@@ -116,7 +116,7 @@ export function isImpliedExtension(url: string): boolean {
  * @returns {any} a JSON StructureDefinition representing the implied extension
  */
 export function materializeImpliedExtension(url: string, defs: FHIRDefinitions): any {
-  const match = url.match(IMPLIED_EXTENSION_REGEX);
+  const match = decodeURI(url).match(IMPLIED_EXTENSION_REGEX);
   if (match == null) {
     logger.error(
       `Unsupported extension URL: ${url}. Extension URLs for converting between versions of ` +
@@ -260,7 +260,7 @@ function applyMetadataToExtension(
   toExt: StructureDefinition
 ): void {
   const elementId = fromED.id ?? fromED.path;
-  toExt.id = `extension-${elementId}`;
+  toExt.id = encodeURIComponent(`extension-${elementId}`);
   toExt.url = `http://hl7.org/fhir/${fromVersion}/StructureDefinition/${toExt.id}`;
   toExt.version = fromSD.fhirVersion;
   toExt.name = `Extension_${elementId.replace(/[^A-Za-z0-9]/g, '_')}`;
@@ -457,7 +457,7 @@ function applyToExtensionElement(
     const id: string = e.id ?? e.path;
     const tail = id.slice(edId.length + 1);
     if (tail.indexOf('.') === -1 && !IGNORED_CHILDREN.includes(tail)) {
-      const slice = toED.addSlice(tail);
+      const slice = toED.addSlice(encodeURIComponent(tail));
       applyMetadataToElement(e, slice);
       slice.type = [new ElementDefinitionType('Extension')];
       slice.unfold(defs);

src/fhirtypes/StructureDefinition.ts

@@ -909,6 +909,15 @@ export class StructureDefinition {
         );
       }
     }
+    if (
+      !matchingSlice &&
+      pathPart.brackets?.every(p => p === decodeURIComponent(p)) &&
+      pathPart.brackets?.some(p => p !== encodeURIComponent(p))
+    ) {
+      const encodedPathPart = cloneDeep(pathPart);
+      encodedPathPart.brackets = encodedPathPart.brackets.map(p => encodeURIComponent(p));
+      return this.findMatchingSlice(fhirPathString, encodedPathPart, elements, fisher);
+    }
     // NOTE: This function will assume the 'brackets' field contains information about slices. Even
     // if you search for foo[sliceName][refName], this will try to find a re-slice
     // sliceName/refName. To find the matching element for foo[sliceName][refName], you must

src/utils/PathUtils.ts

@@ -2,48 +2,50 @@ import { flatten } from 'lodash';
 import { InstanceDefinition, PathPart } from '../fhirtypes';
 import { splitOnPathPeriods } from '../fhirtypes/common';
 import { CaretValueRule, Rule } from '../fshtypes/rules';
+import { SourceInfo } from '../fshtypes/FshEntity';
 import { logger } from './FSHLogger';
 
 /**
  * Parses a FSH Path into a more easily usable form
  * @param {string} fshPath - A syntactically valid path in FSH
  * @returns {PathPart[]} an array of PathParts that is easier to work with
  */
-export function parseFSHPath(fshPath: string): PathPart[] {
+export function parseFSHPath(fshPath: string, sourceInfo?: SourceInfo): PathPart[] {
   const pathParts: PathPart[] = [];
   const seenSlices: string[] = [];
   const indexRegex = /^[0-9]+$/;
   const splitPath = fshPath === '.' ? [fshPath] : splitOnPathPeriods(fshPath);
   for (const pathPart of splitPath) {
-    const splitPathPart = pathPart.split('[');
-    if (splitPathPart.length === 1 || pathPart.endsWith('[x]')) {
-      // There are no brackets, or the brackets are for a choice, so just push on the name
-      pathParts.push({ base: pathPart });
-    } else {
-      // We have brackets, let's  save the bracket info
-      let fhirPathBase = splitPathPart[0];
-      // Get the bracket elements and slice off the trailing ']'
-      let brackets = splitPathPart.slice(1).map(s => s.slice(0, -1));
-      // Get rid of any remaining [x] elements in the brackets
-      if (brackets[0] === 'x') {
-        fhirPathBase += '[x]';
-        brackets = brackets.slice(1);
+    const parsedPart: { base: string; brackets?: string[]; slices?: string[] } = {
+      base: pathPart.match(/^([^\[]+(\[x\])?)/)?.[0] ?? ''
+    };
+    if (pathPart.length > parsedPart.base.length) {
+      // Get the content from the outermost bracket pairs. (?:[^\[\]]*) ensures we don't
+      // match nested closing brackets (thank you, claude.ai)
+      parsedPart.brackets = Array.from(
+        pathPart.slice(parsedPart.base.length).matchAll(/\[([^\[\]]|\[(?:[^\[\]]*)\])*\]/g)
+      ).map(match => match[0].slice(1, -1));
+      seenSlices.push(
+        ...parsedPart.brackets.filter(b => !indexRegex.test(b) && !(b === '+' || b === '='))
+      );
+      if (seenSlices.length > 0) {
+        parsedPart.slices = [...seenSlices];
       }
-      brackets.forEach(bracket => {
-        if (!indexRegex.test(bracket) && !(bracket === '+' || bracket === '=')) {
-          seenSlices.push(bracket);
+      const parsedPartLength =
+        parsedPart.base.length +
+        parsedPart.brackets
+          .map(b => b.length + 2)
+          .reduce((total: number, current: number) => total + current, 0);
+      if (pathPart.length !== parsedPartLength) {
+        const message = `Error processing path due to unmatched brackets: ${fshPath}. `;
+        if (sourceInfo) {
+          logger.error(message, sourceInfo);
+        } else {
+          logger.error(message);
         }
-      });
-      if (seenSlices.length > 0) {
-        pathParts.push({
-          base: fhirPathBase,
-          brackets: brackets,
-          slices: [...seenSlices]
-        });
-      } else {
-        pathParts.push({ base: fhirPathBase, brackets: brackets });
       }
     }
+    pathParts.push(parsedPart);
   }
   return pathParts;
 }
@@ -208,11 +210,11 @@ export function resolveSoftIndexing(rules: Array<Rule | CaretValueRule>, strict
   // Parsing and separating rules by base name and bracket indexes
   const parsedRules = rules.map(rule => {
     const parsedPath: { path: PathPart[]; caretPath?: PathPart[] } = {
-      path: parseFSHPath(rule.path)
+      path: parseFSHPath(rule.path, rule.sourceInfo)
     };
     // If we have a CaretValueRule, we'll need a second round of parsing for the caret path
     if (rule instanceof CaretValueRule) {
-      parsedPath.caretPath = parseFSHPath(rule.caretPath);
+      parsedPath.caretPath = parseFSHPath(rule.caretPath, rule.sourceInfo);
     }
     return parsedPath;
   });

test/fhirdefs/impliedExtension.test.ts

@@ -814,6 +814,108 @@ describe('impliedExtensions', () => {
       expect(loggerSpy.getAllLogs('error')).toHaveLength(0);
     });
 
+    it('should materialize a simple R5 extension for a choice element and properly encode the brackets', () => {
+      const ext = materializeImpliedExtension(
+        'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm[x]',
+        defs
+      );
+      expect(ext).toBeDefined();
+      expect(ext).toMatchObject({
+        resourceType: 'StructureDefinition',
+        id: 'extension-Questionnaire.versionAlgorithm%5Bx%5D',
+        url: 'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm%5Bx%5D',
+        version: '5.0.0',
+        name: 'Extension_Questionnaire_versionAlgorithm_x_',
+        title: 'Implied extension for Questionnaire.versionAlgorithm[x]',
+        status: 'active',
+        description: 'Implied extension for Questionnaire.versionAlgorithm[x]',
+        fhirVersion: '4.0.1',
+        kind: 'complex-type',
+        abstract: false,
+        context: [{ type: 'element', expression: 'Element' }],
+        type: 'Extension',
+        baseDefinition: 'http://hl7.org/fhir/StructureDefinition/Extension',
+        derivation: 'constraint'
+      });
+
+      const diffUrl = ext.differential?.element?.find((e: any) => e.id === 'Extension.url');
+      expect(diffUrl).toEqual({
+        id: 'Extension.url',
+        path: 'Extension.url',
+        fixedUri:
+          'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm%5Bx%5D'
+      });
+      const snapUrl = ext.snapshot?.element?.find((e: any) => e.id === 'Extension.url');
+      expect(snapUrl).toMatchObject(diffUrl);
+
+      const diffValue = ext.differential?.element?.find((e: any) => e.id === 'Extension.value[x]');
+      expect(diffValue).toEqual({
+        id: 'Extension.value[x]',
+        path: 'Extension.value[x]',
+        binding: {
+          strength: 'extensible',
+          valueSet: 'http://hl7.org/fhir/ValueSet/version-algorithm'
+        },
+        type: [{ code: 'string' }, { code: 'Coding' }]
+      });
+      const snapValue = ext.snapshot?.element?.find((e: any) => e.id === 'Extension.value[x]');
+      expect(snapValue).toMatchObject(diffValue);
+
+      expect(loggerSpy.getAllLogs('warn')).toHaveLength(0);
+      expect(loggerSpy.getAllLogs('error')).toHaveLength(0);
+    });
+
+    it('should materialize a simple R5 extension for a choice element with brackets already encoded', () => {
+      const ext = materializeImpliedExtension(
+        'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm%5Bx%5D',
+        defs
+      );
+      expect(ext).toBeDefined();
+      expect(ext).toMatchObject({
+        resourceType: 'StructureDefinition',
+        id: 'extension-Questionnaire.versionAlgorithm%5Bx%5D',
+        url: 'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm%5Bx%5D',
+        version: '5.0.0',
+        name: 'Extension_Questionnaire_versionAlgorithm_x_',
+        title: 'Implied extension for Questionnaire.versionAlgorithm[x]',
+        status: 'active',
+        description: 'Implied extension for Questionnaire.versionAlgorithm[x]',
+        fhirVersion: '4.0.1',
+        kind: 'complex-type',
+        abstract: false,
+        context: [{ type: 'element', expression: 'Element' }],
+        type: 'Extension',
+        baseDefinition: 'http://hl7.org/fhir/StructureDefinition/Extension',
+        derivation: 'constraint'
+      });
+
+      const diffUrl = ext.differential?.element?.find((e: any) => e.id === 'Extension.url');
+      expect(diffUrl).toEqual({
+        id: 'Extension.url',
+        path: 'Extension.url',
+        fixedUri:
+          'http://hl7.org/fhir/5.0/StructureDefinition/extension-Questionnaire.versionAlgorithm%5Bx%5D'
+      });
+      const snapUrl = ext.snapshot?.element?.find((e: any) => e.id === 'Extension.url');
+      expect(snapUrl).toMatchObject(diffUrl);
+
+      const diffValue = ext.differential?.element?.find((e: any) => e.id === 'Extension.value[x]');
+      expect(diffValue).toEqual({
+        id: 'Extension.value[x]',
+        path: 'Extension.value[x]',
+        binding: {
+          strength: 'extensible',
+          valueSet: 'http://hl7.org/fhir/ValueSet/version-algorithm'
+        },
+        type: [{ code: 'string' }, { code: 'Coding' }]
+      });
+      const snapValue = ext.snapshot?.element?.find((e: any) => e.id === 'Extension.value[x]');
+      expect(snapValue).toMatchObject(diffValue);
+
+      expect(loggerSpy.getAllLogs('warn')).toHaveLength(0);
+      expect(loggerSpy.getAllLogs('error')).toHaveLength(0);
+    });
+
     it('should materialize a complex R5 extension', () => {
       const ext = materializeImpliedExtension(
         'http://hl7.org/fhir/5.0/StructureDefinition/extension-MedicationRequest.substitution',
@@ -893,12 +995,12 @@ describe('impliedExtensions', () => {
       });
 
       const diffAllowed = ext.differential?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x]'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D'
       );
       expect(diffAllowed).toEqual({
-        id: 'Extension.extension:allowed[x]',
+        id: 'Extension.extension:allowed%5Bx%5D',
         path: 'Extension.extension',
-        sliceName: 'allowed[x]',
+        sliceName: 'allowed%5Bx%5D',
         short: 'Whether substitution is allowed or not',
         definition:
           'True if the prescriber allows a different drug to be dispensed from what was prescribed.',
@@ -910,28 +1012,28 @@ describe('impliedExtensions', () => {
         type: [{ code: 'Extension' }]
       });
       const snapAllowed = ext.snapshot?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x]'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D'
       );
       expect(snapAllowed).toMatchObject(diffAllowed);
 
       const diffAllowedURL = ext.differential?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x].url'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D.url'
       );
       expect(diffAllowedURL).toEqual({
-        id: 'Extension.extension:allowed[x].url',
+        id: 'Extension.extension:allowed%5Bx%5D.url',
         path: 'Extension.extension.url',
-        fixedUri: 'allowed[x]'
+        fixedUri: 'allowed%5Bx%5D'
       });
       const snapAllowedURL = ext.snapshot?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x].url'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D.url'
       );
       expect(snapAllowedURL).toMatchObject(diffAllowedURL);
 
       const diffAllowedValue = ext.differential?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x].value[x]'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D.value[x]'
       );
       expect(diffAllowedValue).toEqual({
-        id: 'Extension.extension:allowed[x].value[x]',
+        id: 'Extension.extension:allowed%5Bx%5D.value[x]',
         path: 'Extension.extension.value[x]',
         type: [{ code: 'boolean' }, { code: 'CodeableConcept' }],
         binding: {
@@ -941,7 +1043,7 @@ describe('impliedExtensions', () => {
         }
       });
       const snapAllowedValue = ext.snapshot?.element?.find(
-        (e: any) => e.id === 'Extension.extension:allowed[x].value[x]'
+        (e: any) => e.id === 'Extension.extension:allowed%5Bx%5D.value[x]'
       );
       expect(snapAllowedValue).toMatchObject(diffAllowedValue);
 

test/fhirtypes/StructureDefinition.test.ts

@@ -1,9 +1,15 @@
 import fs from 'fs-extra';
 import path from 'path';
-import { FHIRDefinitions } from '../../src/fhirdefs/FHIRDefinitions';
+import { createFHIRDefinitions, FHIRDefinitions } from '../../src/fhirdefs/FHIRDefinitions';
 import { StructureDefinition } from '../../src/fhirtypes/StructureDefinition';
 import { ElementDefinition, ElementDefinitionType } from '../../src/fhirtypes/ElementDefinition';
-import { getTestFHIRDefinitions, loggerSpy, testDefsPath, TestFisher } from '../testhelpers';
+import {
+  getLocalVirtualPackage,
+  getTestFHIRDefinitions,
+  loggerSpy,
+  testDefsPath,
+  TestFisher
+} from '../testhelpers';
 import { FshCode, Invariant, Logical, Mapping, Profile, Resource } from '../../src/fshtypes';
 import { Type } from '../../src/utils/Fishable';
 import { AddElementRule, ObeysRule, OnlyRule } from '../../src/fshtypes/rules';
@@ -29,6 +35,9 @@ describe('StructureDefinition', () => {
 
   beforeAll(async () => {
     defs = await getTestFHIRDefinitions(true, testDefsPath('r4-definitions'));
+    const r5Defs = await createFHIRDefinitions(true);
+    r5Defs.loadVirtualPackage(getLocalVirtualPackage(testDefsPath('r5-definitions')));
+    defs.addSupplementalFHIRDefinitions('hl7.fhir.r5.core#5.0.0', r5Defs);
     fisher = new TestFisher().withFHIR(defs);
     // resolve observation once to ensure it is present in defs
     observation = fisher.fishForStructureDefinition('Observation');
@@ -1011,11 +1020,15 @@ describe('StructureDefinition', () => {
     let lipidProfile: StructureDefinition;
     let clinicalDocument: StructureDefinition;
     let valueSet: StructureDefinition;
+    let xVersionExtension: StructureDefinition;
     beforeEach(() => {
       respRate = fisher.fishForStructureDefinition('resprate');
       lipidProfile = fisher.fishForStructureDefinition('lipidprofile');
       clinicalDocument = fisher.fishForStructureDefinition('clinicaldocument');
       valueSet = fisher.fishForStructureDefinition('ValueSet');
+      xVersionExtension = fisher.fishForStructureDefinition(
+        'http://hl7.org/fhir/5.0/StructureDefinition/extension-MedicationRequest.substitution'
+      );
     });
 
     // Simple paths (no brackets)
@@ -1211,6 +1224,46 @@ describe('StructureDefinition', () => {
       expect(clinicalDocument.elements.length).toBe(originalLength + 4);
     });
 
+    // Complex cross-version extensions with child choice extensions (slice name has encoded [x])
+
+    it('should find element in cross-version extension representing a choice when brackets are not encoded', () => {
+      const allowed = xVersionExtension.findElementByPath('extension[allowed[x]]', fisher);
+      expect(allowed).toBeDefined();
+      expect(allowed.id).toBe('Extension.extension:allowed%5Bx%5D');
+    });
+
+    it('should find value[x] for element in cross-version extension representing a choice when brackets are not encoded', () => {
+      const allowedValue = xVersionExtension.findElementByPath(
+        'extension[allowed[x]].value[x]',
+        fisher
+      );
+      expect(allowedValue).toBeDefined();
+      expect(allowedValue.id).toBe('Extension.extension:allowed%5Bx%5D.value[x]');
+      expect(allowedValue.type).toEqual([
+        new ElementDefinitionType('boolean'),
+        new ElementDefinitionType('CodeableConcept')
+      ]);
+    });
+
+    it('should find element in cross-version extension representing a choice when brackets are encoded', () => {
+      const allowed = xVersionExtension.findElementByPath('extension[allowed%5Bx%5D]', fisher);
+      expect(allowed).toBeDefined();
+      expect(allowed.id).toBe('Extension.extension:allowed%5Bx%5D');
+    });
+
+    it('should find value[x] for element in cross-version extension representing a choice when brackets are encoded', () => {
+      const allowedValue = xVersionExtension.findElementByPath(
+        'extension[allowed%5Bx%5D].value[x]',
+        fisher
+      );
+      expect(allowedValue).toBeDefined();
+      expect(allowedValue.id).toBe('Extension.extension:allowed%5Bx%5D.value[x]');
+      expect(allowedValue.type).toEqual([
+        new ElementDefinitionType('boolean'),
+        new ElementDefinitionType('CodeableConcept')
+      ]);
+    });
+
     // Choices
     it('should make explicit a non-existent choice element by path', () => {
       const originalLength = observation.elements.length;