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.

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

entities with same name #1528

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

KaelynJefferson
Copy link
Collaborator

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

Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 276 to 284
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)
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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}.`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines 10314 to 10348
expect(loggerSpy.getAllLogs('error')).toHaveLength(0);
expect(loggerSpy.getAllLogs('error')).toHaveLength(1);
expect(loggerSpy.getMessageAtIndex(0, 'error')).toMatch(
/Multiple FSH entities created with name/
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should still be producing 0 errors.

Comment on lines 6879 to 6910
expect(loggerSpy.getMessageAtIndex(1, 'error')).toMatch(
expect(loggerSpy.getMessageAtIndex(7, 'error')).toMatch(
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's causing the six additional errors here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canonical() behavior when a CodeSystem and a ValueSet have exact same Ids
2 participants