entities with same name #1528

KaelynJefferson · 2024-10-11T17:33:54Z

Description: This PR adds conditionals to check for entities that exist with the same name, and logs an error when entities are defined with a duplicate name.

Testing Instructions: jest unit tests

Related Issue: Fixes #1469

mint-thompson

Thank you for working on this! I have some feedback about some of your tests, and a suggestion for the message being logged.

mint-thompson · 2024-10-17T15:08:22Z

src/export/CodeSystemExporter.ts

+    if (
+      this.pkg.codeSystems.some(cs => cs.name === fshDefinition.name) ||
+      this.pkg.instances.some(instance => fshDefinition.name === instance._instanceMeta.name) ||
+      this.pkg.profiles.some(prof => fshDefinition.name === prof.name) ||
+      this.pkg.extensions.some(extn => fshDefinition.name === extn.name) ||
+      this.pkg.logicals.some(logical => fshDefinition.name === logical.name) ||
+      this.pkg.resources.some(resource => fshDefinition.name === resource.name) ||
+      this.pkg.valueSets.some(valueSet => fshDefinition.name === valueSet.name)
+    ) {


I think it would be possible to write this condition by checking this.pkg.fshMap rather than each of the package's lists of FSH entities individually.

mint-thompson · 2024-10-17T15:12:33Z

src/export/CodeSystemExporter.ts

+      this.pkg.resources.some(resource => fshDefinition.name === resource.name) ||
+      this.pkg.valueSets.some(valueSet => fshDefinition.name === valueSet.name)
+    ) {
+      logger.error(`Multiple FSH entities created with name ${fshDefinition.name}.`);


For the message, it might be useful to rephrase it so that it gives some more details about name overlap. Maybe something like "Cannot export [entity type] [name]: an [entity type] with this name already exists." Also, it would be useful to include the source info for the entity that can't be exported.

mint-thompson · 2024-10-17T15:15:32Z

test/export/InstanceExporter.test.ts

-        expect(loggerSpy.getAllLogs('error')).toHaveLength(0);
+        expect(loggerSpy.getAllLogs('error')).toHaveLength(1);
+        expect(loggerSpy.getMessageAtIndex(0, 'error')).toMatch(
+          /Multiple FSH entities created with name/
+        );


I'm a little confused by this. Why is this error appearing for these tests? There don't appear to be any entities with the same names defined.

This error is occurring for a different reason than below.

Stepping through this test, I see that when export is called in this group of tests, it adds the profile (CarProfile) and the logical (Car) at the same time, so when calling export for the Logical, its already in the this.pkg.FshMap when it checks for it, and throws the error.

mint-thompson · 2024-10-17T15:17:21Z

test/export/InstanceExporter.test.ts

@@ -2217,7 +2248,7 @@ describe('InstanceExporter', () => {
      thisIsName.isInstance = true;
      thisIsSeacow.rules.push(thisIsName);
      const exported = exportInstance(thisIsSeacow);
-      expect(loggerSpy.getAllMessages('error')).toHaveLength(0);
+      // expect(loggerSpy.getAllMessages('error')).toHaveLength(0);


I think this should still be producing 0 errors.

mint-thompson · 2024-10-17T15:17:41Z

test/export/InstanceExporter.test.ts

-      expect(loggerSpy.getMessageAtIndex(1, 'error')).toMatch(
+      expect(loggerSpy.getMessageAtIndex(7, 'error')).toMatch(


What's causing the six additional errors here?

I believe the extra error messages were due to how this test was calling exportInstance multiple times (exportInstance is defined as a function in the beforeEach that when called, calls export for cs, vs, and sd and the instance given) so on the second time, it logged the errors. I adjusted this test (and the one above) to just call export within the function instead.

mint-thompson

Thank you!

cmoesel

I think this is a good approach and the code/tests look good.

My one question is if duplicates really need to be errors or if they can be warnings instead. I've reviewed the FSH specification and it does not say that item names need to be unique. The FHIR spec explicitly states that item names do not need to be unique. For these reasons, I'm inclined to say this should be a warning. @KaelynJefferson, @mint-thompson, @jafeltra -- what do you think?

If the team decides that error is appropriate, then I would like to run a regression to see if it causes new errors for existing projects -- and if so, to what degree.

cmoesel · 2024-12-02T20:33:52Z

UPDATE: I ran the regression and this caused new errors in almost 60 projects of out the 510 projects updated in the last year (so... > 10% of projects are affected). That being the case, I don't think we want to introduce this as an error. And if we make it a warning, we might want to consider some additional guidance on how you can make the entity name unique while retaining the same name in the exported definition (as it seems that people must be doing this intentionally).

cmoesel · 2024-12-06T16:22:34Z

@KaelynJefferson: The rest of the team discussed this in a standup and agreed that we don't want this to be an error. We also thought that logging 50 warnings (as would be the case for a few IGs) might be too much. So we suggest that you collect all of the duplicate entity names and issue one warning message. Perhaps something like:

Detected FSH entity definitions with duplicate names. While FSH allows for duplicate names across entity types, they can lead to ambiguous results when referring to these entities by name elsewhere (e.g., in references). Consider using unique names in FSH declarations and assigning duplicated names using caret assignment rules instead. Detected duplicate names: Foo, Bar, Baz.

…names

cmoesel

I've made a small request for a change. In addition, I think this needs to be re-synched with the master branch.

src/import/FSHTank.ts

test/run/FshToFhir.test.ts

cmoesel

This looks great. Thank you, @KaelynJefferson!

KaelynJefferson added 2 commits September 23, 2024 10:06

adding conditional for checking same name with entities, start to tests

fbb11a7

added earlier conditional, tests, modified tests affected

78a71b3

mint-thompson requested changes Oct 17, 2024

View reviewed changes

KaelynJefferson added 7 commits October 20, 2024 17:46

edit to conditional, tests from feedback

14b7391

fix to tests, prettier

289ccc2

format, cleanup

1cac280

removing first approach within exporters

9b9cd65

adding conditional to fshtank, unit tests

8288b2e

reverting first approach unit test

c7e86c6

reverting changes to elementdefinition.constraintype.test.ts

761e93b

mint-thompson previously approved these changes Dec 2, 2024

View reviewed changes

Merge branch 'master' into entities-with-same-name

415d926

cmoesel reviewed Dec 2, 2024

View reviewed changes

convert error to warning, log singular warning with duplicate entity …

2894758

…names

KaelynJefferson dismissed mint-thompson’s stale review via 2894758 December 14, 2024 19:57

cmoesel requested changes Dec 16, 2024

View reviewed changes

src/import/FSHTank.ts Outdated Show resolved Hide resolved

test/run/FshToFhir.test.ts Outdated Show resolved Hide resolved

KaelynJefferson and others added 2 commits December 20, 2024 11:15

adding space between duplicate names in warning message

0ad375b

Merge branch 'master' into entities-with-same-name

51d137a

cmoesel approved these changes Dec 23, 2024

View reviewed changes

cmoesel added 2 commits December 23, 2024 12:33

Merge branch 'master' into entities-with-same-name

519dd58

Merge branch 'master' into entities-with-same-name

2d9c382

mint-thompson approved these changes Dec 26, 2024

View reviewed changes

KaelynJefferson merged commit dc4b717 into master Dec 30, 2024
14 checks passed

cmoesel deleted the entities-with-same-name branch December 30, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

entities with same name #1528

entities with same name #1528

KaelynJefferson commented Oct 11, 2024

mint-thompson left a comment

mint-thompson Oct 17, 2024

mint-thompson Oct 17, 2024

mint-thompson Oct 17, 2024

KaelynJefferson Oct 21, 2024

mint-thompson Oct 17, 2024

mint-thompson Oct 17, 2024

KaelynJefferson Oct 21, 2024

mint-thompson left a comment

cmoesel left a comment •

edited

Loading

cmoesel commented Dec 2, 2024

cmoesel commented Dec 6, 2024

cmoesel left a comment

cmoesel left a comment

		expect(loggerSpy.getMessageAtIndex(1, 'error')).toMatch(
		expect(loggerSpy.getMessageAtIndex(7, 'error')).toMatch(

entities with same name #1528

entities with same name #1528

Conversation

KaelynJefferson commented Oct 11, 2024

mint-thompson left a comment

Choose a reason for hiding this comment

mint-thompson Oct 17, 2024

Choose a reason for hiding this comment

mint-thompson Oct 17, 2024

Choose a reason for hiding this comment

mint-thompson Oct 17, 2024

Choose a reason for hiding this comment

KaelynJefferson Oct 21, 2024

Choose a reason for hiding this comment

mint-thompson Oct 17, 2024

Choose a reason for hiding this comment

mint-thompson Oct 17, 2024

Choose a reason for hiding this comment

KaelynJefferson Oct 21, 2024

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

cmoesel left a comment • edited Loading

Choose a reason for hiding this comment

cmoesel commented Dec 2, 2024

cmoesel commented Dec 6, 2024

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel left a comment •

edited

Loading