Repository: FHIR/sushi
Pull request #866 (2021-07-02 14:32:35)

Fix bug with applying path to insert rules with empty paths
When a rule in a `RuleSet` has an empty path (such as a `^` rule), and that `RuleSet` is applied from an `insert` rule that has a path context, we were incorrectly joining the empty path with the context, resulting in paths such as `name.`, which are incorrect. This fixes that and adds a test.

See this [thread](https://chat.fhir.org/#narrow/stream/215610-shorthand/topic/Caret.20paths.20in.20.22deeply.20inserted.22.20rulesets) for context.

And here is some example FSH that does not work on `master`, but should work on this branch:
```
RuleSet: rs
* ^short = "test"

Profile: TestPatient
Parent: Patient
* name insert rs
```

ngfreiter: > I think the right behavior is ambiguous because:
> 
> * On one hand, people might think of `.` path as "this element", which would imply the short should be applied to `category` in the example above.
> * On the other hand, we say the `.` represents the "root" element, and so in a way, the previous context doesn't matter if `.` is always "root".

I had thought of it as `.` representing the path to the "root" element, so in that way I still think it does matter if you put previous context in front of it. Just like with any other path to a specific element, if I then prepend some other path, I'm going to end up with a broken path. So I'm on team "don't allow it but make a friendlier error message".
cmoesel: > https://github.com/FHIR/sushi/pull/866#issuecomment-875597833

Cool.  Is it fair to request that as part of this PR?
ngfreiter: > Cool. Is it fair to request that as part of this PR?

@cmoesel, this should be done in https://github.com/FHIR/sushi/pull/866/commits/c530718c560120d7e695428f287312fe732da364. I also added some processing to indentation so that something like this:
```
Profile: MyP
Parent: Patient
* name
  * . ^short = "foo"
```
would be covered, since that seems like a similar situation.

Patch:
diff --git a/src/fhirtypes/common.ts b/src/fhirtypes/common.ts
index 6f09421..b7efda3 100644
--- a/src/fhirtypes/common.ts
+++ b/src/fhirtypes/common.ts
@@ -474,7 +474,17 @@ export function applyInsertRules(
         if (isAllowedRule(fshDefinition, ruleSetRule)) {
           const ruleSetRuleClone = cloneDeep(ruleSetRule);
           if (context) {
-            ruleSetRuleClone.path = `${context}.${ruleSetRuleClone.path}`;
+            let newPath = context;
+            if (ruleSetRuleClone?.path === '.') {
+              logger.error(
+                "The special '.' path is only allowed in top-level rules. The rule will be processed as if it is not indented.",
+                ruleSetRule.sourceInfo
+              );
+              newPath = ruleSetRuleClone.path;
+            } else if (ruleSetRuleClone.path) {
+              newPath += `.${ruleSetRuleClone.path}`;
+            }
+            ruleSetRuleClone.path = newPath;
           }
           expandedRules.push(ruleSetRuleClone);
           if (firstRule) {
diff --git a/src/import/FSHImporter.ts b/src/import/FSHImporter.ts
index 71fca59..8879d2f 100644
--- a/src/import/FSHImporter.ts
+++ b/src/import/FSHImporter.ts
@@ -2048,6 +2048,17 @@ export class FSHImporter extends FSHVisitor {
     const currentIndent = location.startColumn - this.baseIndent;
     const contextIndex = currentIndent / INDENT_WIDTH;
 
+    if (this.pathContext.length && path.length === 1 && path[0] === '.') {
+      logger.error(
+        "The special '.' path is only allowed in top-level rules. The rule will be processed as if it is not indented.",
+        {
+          location,
+          file: this.currentFile
+        }
+      );
+      return path;
+    }
+
     if (!this.isValidContext(location, currentIndent, this.pathContext)) {
       return path;
     }


Test patch:
diff --git a/test/fhirtypes/applyInsertRules.test.ts b/test/fhirtypes/applyInsertRules.test.ts
index a5562b5..afcbc98 100644
--- a/test/fhirtypes/applyInsertRules.test.ts
+++ b/test/fhirtypes/applyInsertRules.test.ts
@@ -5,14 +5,16 @@ import {
   CardRule,
   ConceptRule,
   AssignmentRule,
-  AddElementRule
+  AddElementRule,
+  CaretValueRule
 } from '../../src/fshtypes/rules';
 import {
   loggerSpy,
   assertCardRule,
   assertValueSetConceptComponent,
   assertAssignmentRule,
-  assertAddElementRule
+  assertAddElementRule,
+  assertCaretValueRule
 } from '../testhelpers';
 import { minimalConfig } from '../utils/minimalConfig';
 import { applyInsertRules } from '../../src/fhirtypes/common';
@@ -89,6 +91,28 @@ describe('applyInsertRules', () => {
     assertCardRule(profile.rules[0], 'category.coding', 1, '*');
   });
 
+  it('should apply a rule without a path from a single level insert rule with a path', () => {
+    // RuleSet: Bar
+    // * ^short = "foo"
+    //
+    // Profile: Foo
+    // Parent: Observation
+    // * category insert Bar
+    const caretRule = new CaretValueRule('');
+    caretRule.caretPath = 'short';
+    caretRule.value = 'foo';
+    ruleSet1.rules.push(caretRule);
+
+    const insertRule = new InsertRule('category');
+    insertRule.ruleSet = 'Bar';
+    profile.rules.push(insertRule);
+
+    applyInsertRules(profile, tank);
+
+    expect(profile.rules).toHaveLength(1);
+    assertCaretValueRule(profile.rules[0], 'category', 'short', 'foo', undefined, []);
+  });
+
   it('should only apply soft indexing once when applying rules from a rule with a path', () => {
     // RuleSet: Bar
     // * family = "foo"
@@ -472,6 +496,32 @@ describe('applyInsertRules', () => {
     );
   });
 
+  it('should log an error when applying context to a . path', () => {
+    // RuleSet: Bar
+    // * . ^short = "foo"
+
+    // Profile: Foo
+    // Parent: Observation
+    // * category insert Bar
+    const caretValueRule = new CaretValueRule('.');
+    caretValueRule.caretPath = 'short';
+    caretValueRule.path = '.';
+    caretValueRule.value = 'foo';
+    ruleSet1.rules.push(caretValueRule);
+
+    const insertRule = new InsertRule('category').withFile('Bar.fsh').withLocation([1, 2, 3, 4]);
+    insertRule.ruleSet = 'Bar';
+    profile.rules.push(insertRule);
+
+    applyInsertRules(profile, tank);
+
+    expect(profile.rules).toHaveLength(1);
+    assertCaretValueRule(profile.rules[0], '.', 'short', 'foo', undefined, []);
+    expect(loggerSpy.getLastMessage('error')).toMatch(
+      /The special '\.' path.*File: Bar\.fsh.*Line: 1 - 3\D*/s
+    );
+  });
+
   it('should apply add element rules from a single level insert rule', () => {
     // RuleSet: AddCoreElements
     // primaryId 1..1 SU uri "Primary ID"
diff --git a/test/import/FSHImporter.context.test.ts b/test/import/FSHImporter.context.test.ts
index 1fcd32b..7e93db6 100644
--- a/test/import/FSHImporter.context.test.ts
+++ b/test/import/FSHImporter.context.test.ts
@@ -340,6 +340,27 @@ describe('FSHImporter', () => {
       });
     });
 
+    it('should log an error when a . rule is indented', () => {
+      const input = `
+      Profile: Foo
+      Parent: Patient
+      * name 1..1
+        * . ^short = "foo"
+    `;
+
+      const result = importSingleText(input, 'Context.fsh');
+      expect(loggerSpy.getAllMessages('error')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('error')).toMatch(/The special '\.' path/);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(0);
+      expect(result.profiles.size).toBe(1);
+      const profile = result.profiles.get('Foo');
+      expect(profile.name).toBe('Foo');
+      expect(profile.parent).toBe('Patient');
+      expect(profile.rules.length).toBe(2);
+      assertCardRule(profile.rules[0], 'name', 1, 1);
+      assertCaretValueRule(profile.rules[1], '.', 'short', 'foo', false, ['.']);
+    });
+
     it('should log an error when a rule is indented below a rule without a path', () => {
       const input = `
       Profile: Foo