Support Context keyword when defining Extensions #1282

mint-thompson · 2023-05-31T13:20:06Z

Fixes #1259 and completes task CIMPL-1090.

This adds the new Context keyword that can be used when defining an Extension. The value of the context can be specified in a few different ways:

A quoted string
A FSH path that starts with the name or id of some StructureDefinition
A URL for a StructureDefinition, optionally followed by # and a FSH path

Multiple contexts can be specified in a comma-separated list.

The resulting type of the context depends on what is specified:

For a quoted string, the fhirpath type is always used.
For an unquoted value that specifies an Extension definition or an element that is part of a complex extension, the extension type is used.
For any other element, the element type is used.

This allows for some flexibility for how authors specify the context, while consistently resolving contexts to the more specific extension type whenever possible. The included test cases demonstrate this flexibility.

For more information about extension contexts, the official documentation provides some useful details.

The Context keyword is used to specify the contexts in which the Extension should be used. Add the keyword to the grammar definition. Store contexts with source info when importing FSH.

Contexts specified with the Context keyword are applied to Extensions during export. If the value is quoted, it is a "fhirpath" context. If the value is not quoted and resolves to an Extension definition, it is an "extension" context. Otherwise, it is an "element" context.

The Context keyword is used at most once per extension. This is similar to other metadata keywords. To provide multiple contexts, a comma-separated list can be provided.

When using the Context keyword on an Extension, validate that the extension or element specified is something that actually exists. The element is specified by a FSH path, but is exported as a FHIR element id. Always use the resource id when the context is an element on a core FHIR resource. Otherwise, use the URL followed by an element id.

A complex extension may contain more than one level of extension elements. Traverse the extension by url to find deeply nested extensions. When an element path is given as context, it may represent a deeply nested extension. If so, set the context as extension context. The element counts as a deeply nested extension if every element along its path is also an extension.

Contained extensions must be specified the same as any other element by using a FSH path.

jafeltra

This looks really good! I had a few comments inline, as well as this:

One other thing related to this feature: while reviewing this, I realized that we need to add Context to the keywords that get highlighted in the VS Code extension. Ideally, I think we could release a new version of the extension around the same time, or even before, we release this feature in SUSHI.

src/fshtypes/Extension.ts

test/fshtypes/Extension.test.ts

jafeltra · 2023-06-02T16:11:12Z

test/export/StructureDefinition.ExtensionExporter.test.ts

+        'Could not find resource MysteryExtension to use as extension context.'
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(/File: Context\.fsh.*Line: 8\D*/s);
+    });


The last test in the custom resource block reminded me of possibly another test case that might be useful in this section. Is it worthwhile also testing that an invalid FSH path on a resource? I think the error will be triggered by the same logic, so it might be a duplicate test, so curious what you think.

Do you think adding that type of test would be redundant? I'm still just curious to hear what you think.

cmoesel

This is a really nice implementation of a tricky feature. Very well done. I think you did a great job leveraging some of the functions we already had, resulting in a very clean implementation.

I've left a few comments for your consideration. Thank you!

src/export/StructureDefinitionExporter.ts

test/export/StructureDefinition.ExtensionExporter.test.ts

Only set the default context if the Context keyword is unused and there is no existing context. Assume that the last # present in an unquoted context is what separates the URL from the path. This helps with URLs that contain a # character.

cmoesel

Everything is looking really good! I just found one small bit of dead code that can probably be deleted.

cmoesel · 2023-06-05T21:03:01Z

src/fshtypes/Extension.ts

+    const contextLines: string[] = [];
+    this.contexts.forEach(extContext => {
+      if (extContext.isQuoted) {
+        contextLines.push(`Context: "${fshifyString(extContext.value)}"`);
+      } else {
+        contextLines.push(`Context: ${extContext.value}`);
+      }
+    });


As far as I can tell, this is dead code now and can be deleted.

jafeltra

I only wanted to check in one outstanding question, other than what Chris mentioned.

jafeltra · 2023-06-05T21:06:00Z

test/export/StructureDefinition.ExtensionExporter.test.ts

+        'Could not find resource MysteryExtension to use as extension context.'
+      );
+      expect(loggerSpy.getLastMessage('error')).toMatch(/File: Context\.fsh.*Line: 8\D*/s);
+    });


Do you think adding that type of test would be redundant? I'm still just curious to hear what you think.

cmoesel

I just did some manual testing on this, and unfortunately, it looks like it's not quite ready yet.

Given an extension with multiple contexts like this:

Extension: Foo
Context: Patient, Practitioner, "some.fhirpath"
* value[x] only boolean

We get the following parse errors:

error token recognition error at: ' '
  File: /Users/cmoesel/data/fsh/ScratchPad/input/fsh/scratch.fsh
  Line: 12
error token recognition error at: ' '
  File: /Users/cmoesel/data/fsh/ScratchPad/input/fsh/scratch.fsh
  Line: 12

It also emits errors if we try to separate them on separate lines (one error for every space character).

So... we'll hold this feature back for SUSHI 3.1.0.

cmoesel · 2023-06-05T21:33:53Z

test/import/FSHImporter.Extension.test.ts

+        Id: some-extension
+        Context: "some.fhirpath()", Observation.component, http://example.org/MyPatient#identifier,
+                 "another.fhirpath(var, 0)"
+        `;


We need to add an assertion that confirms that no errors are emitted. As of now, this test does produce errors because the parser doesn't know what to do w/ the spaces between the contexts.

Remove unused code from Extension toFSH method.

cmoesel

YES!

mint-thompson added 6 commits May 31, 2023 08:48

Import Extensions using Context keyword

00ac74b

The Context keyword is used to specify the contexts in which the Extension should be used. Add the keyword to the grammar definition. Store contexts with source info when importing FSH.

Use Context keyword only once

ec9bf66

The Context keyword is used at most once per extension. This is similar to other metadata keywords. To provide multiple contexts, a comma-separated list can be provided.

comment cleanup

e7d683a

mint-thompson requested review from jafeltra and cmoesel as code owners May 31, 2023 13:20

mint-thompson mentioned this pull request May 31, 2023

Add Context keyword for Extension HL7/fhir-shorthand#156

Merged

Remove special syntax for contained extensions

644dfdc

Contained extensions must be specified the same as any other element by using a FSH path.

jafeltra reviewed Jun 2, 2023

View reviewed changes

cmoesel requested changes Jun 2, 2023

View reviewed changes

mint-thompson added 3 commits June 5, 2023 10:50

Merge branch 'master' into cimpl-1090-extension-context

9b57acd

Extension toFSH only writes Context keyword once

ae56275

cmoesel requested changes Jun 5, 2023

View reviewed changes

jafeltra reviewed Jun 5, 2023

View reviewed changes

cmoesel requested changes Jun 5, 2023

View reviewed changes

mint-thompson added 2 commits June 6, 2023 09:22

Merge branch 'master' into cimpl-1090-extension-context

41d5e19

Discard extra whitespace tokens in list of contexts

4efb548

Remove unused code from Extension toFSH method.

cmoesel approved these changes Jun 8, 2023

View reviewed changes

jafeltra approved these changes Jun 9, 2023

View reviewed changes

cmoesel merged commit efcb2fa into master Jun 9, 2023

cmoesel deleted the cimpl-1090-extension-context branch June 9, 2023 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Context keyword when defining Extensions #1282

Support Context keyword when defining Extensions #1282

mint-thompson commented May 31, 2023 •

edited by cmoesel

Loading

jafeltra left a comment

jafeltra Jun 2, 2023

jafeltra Jun 5, 2023

cmoesel left a comment

cmoesel left a comment

cmoesel Jun 5, 2023

jafeltra left a comment

jafeltra Jun 5, 2023

cmoesel left a comment

cmoesel Jun 5, 2023

cmoesel left a comment

Support Context keyword when defining Extensions #1282

Support Context keyword when defining Extensions #1282

Conversation

mint-thompson commented May 31, 2023 • edited by cmoesel Loading

jafeltra left a comment

Choose a reason for hiding this comment

jafeltra Jun 2, 2023

Choose a reason for hiding this comment

jafeltra Jun 5, 2023

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel Jun 5, 2023

Choose a reason for hiding this comment

jafeltra left a comment

Choose a reason for hiding this comment

jafeltra Jun 5, 2023

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel Jun 5, 2023

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

mint-thompson commented May 31, 2023 •

edited by cmoesel

Loading