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

feat(import-alias):patch change for aliased imported types #894

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/concerto-analysis/src/compare-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export const defaultCompareConfig: CompareConfig = {
'enum-value-added': CompareResult.PATCH,
'enum-value-removed': CompareResult.MAJOR,
'property-type-changed': CompareResult.MAJOR,
'property-type-aliased':CompareResult.PATCH,
'property-validator-added': CompareResult.MAJOR,
'property-validator-removed': CompareResult.PATCH,
'property-validator-changed': CompareResult.MAJOR,
Expand Down
14 changes: 11 additions & 3 deletions packages/concerto-analysis/src/comparers/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,24 @@ const propertyTypeChanged: ComparerFactory = (context) => ({
// return;
// }
const versionDiff = semver.diff(aTypeVersion, bTypeVersion);
if (!versionDiff) {
return;
} else if (versionDiff === 'major' || versionDiff === 'premajor') {
if (versionDiff === 'major' || versionDiff === 'premajor') {
context.report({
key: 'property-type-changed',
message: `The ${aType} "${a.getName()}" in the ${classDeclarationType} "${a.getParent().getName()}" changed type from "${aFQTN}" to "${bFQTN}" (type version incompatible)`,
element: a
});
return;
}
// Up to this point, 'a' and 'b' have identical fully qualified type names with matching major and premajor versions.
// Next, we verify whether their types differ locally, which would indicate aliasing.
if (!versionDiff && (a.getType() !== b.getType())) {
context.report({
key: 'property-type-aliased',
message: `The local type name for "${a.getName()}" in the ${classDeclarationType} "${a.getParent().getName()}" is changed from ${a.getType()} to ${b.getType()}`,
element: a
});
return;
}
},
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace [email protected]

import [email protected].{ Bar }

concept Thing {
o Bar bar
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace [email protected]

import [email protected].{ Bar as b }

concept Thing {
o b bar
}
10 changes: 5 additions & 5 deletions packages/concerto-analysis/test/unit/compare-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe('CompareConfigBuilder', () => {
const actual = builder.default().build();

expect(actual.comparerFactories.length).toEqual(11);
expect(Object.keys(actual.rules).length).toEqual(20);
expect(Object.keys(actual.rules).length).toEqual(21);
expect(actual.rules['class-declaration-added']).toEqual(CompareResult.MINOR);
expect(actual.rules['optional-property-added']).toEqual(CompareResult.PATCH);
expect(actual.rules['map-value-type-changed']).toEqual(CompareResult.MAJOR);
Expand All @@ -37,7 +37,7 @@ describe('CompareConfigBuilder', () => {
const actual = builder.default().extend(toExtend).build();

expect(actual.comparerFactories.length).toEqual(12);
expect(Object.keys(actual.rules).length).toEqual(21);
expect(Object.keys(actual.rules).length).toEqual(22);
expect(actual.rules['a-new-rule']).toEqual(CompareResult.MAJOR);
});

Expand All @@ -47,7 +47,7 @@ describe('CompareConfigBuilder', () => {
const actual = builder.default().addComparerFactory(() => ({})).build();

expect(actual.comparerFactories.length).toEqual(12);
expect(Object.keys(actual.rules).length).toEqual(20);
expect(Object.keys(actual.rules).length).toEqual(21);
});

it('Should add a new rule', () => {
Expand All @@ -56,7 +56,7 @@ describe('CompareConfigBuilder', () => {
const actual = builder.default().addRule('a-new-rule', CompareResult.MAJOR).build();

expect(actual.comparerFactories.length).toEqual(11);
expect(Object.keys(actual.rules).length).toEqual(21);
expect(Object.keys(actual.rules).length).toEqual(22);
expect(actual.rules['a-new-rule']).toEqual(CompareResult.MAJOR);
});

Expand All @@ -66,7 +66,7 @@ describe('CompareConfigBuilder', () => {
const actual = builder.default().removeRule('optional-property-added').build();

expect(actual.comparerFactories.length).toEqual(11);
expect(Object.keys(actual.rules).length).toEqual(19);
expect(Object.keys(actual.rules).length).toEqual(20);
expect(actual.rules['optional-property-added']).toBeFalsy();
});

Expand Down
15 changes: 14 additions & 1 deletion packages/concerto-analysis/test/unit/compare.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ async function getModelFile(modelManager: ModelManager, fileName: string) {
async function getModelFiles(
aFileName: string,
bFileName: string,
importAliasing = false
): Promise<[a: ModelFile, b: ModelFile]> {
const modelManager = new ModelManager({ strict: true });
const modelManager = new ModelManager({ strict: true, importAliasing: importAliasing });
const a = await getModelFile(modelManager, aFileName);
const b = await getModelFile(modelManager, bFileName);
return [a, b];
Expand Down Expand Up @@ -255,6 +256,18 @@ test('should detect an array changing to a property', async () => {
expect(results.result).toBe(CompareResult.MAJOR);
});

test('should detect a field local type name change', async () => {
const [a, b] = await getModelFiles('field-local-type-change-a.cto', 'field-local-type-change-b.cto',true);
const results = new Compare().compare(a, b);
expect(results.findings).toEqual(expect.arrayContaining([
expect.objectContaining({
key: 'property-type-aliased',
message: 'The local type name for "bar" in the concept "Thing" is changed from Bar to b'
})
]));
expect(results.result).toBe(CompareResult.PATCH);
});

test('should detect a map key type changing from x to y', async () => {
process.env.ENABLE_MAP_TYPE = 'true'; // TODO Remove on release of MapType
const [a, b] = await getModelFiles('map-added.cto', 'map-changed-key.cto');
Expand Down
2 changes: 1 addition & 1 deletion packages/concerto-core/api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ class ModelLoader {
+ ModelManager[] loadModelManagerFromModelFiles(object[],string[],object,boolean?,boolean?,number?)
}
class ModelManager extends BaseModelManager {
+ void constructor(object?,boolean?,Object?,boolean?)
+ void constructor(object?,boolean?,Object?,boolean?,boolean?)
+ ModelFile addCTOModel(string,string?,boolean?) throws IllegalModelException
}
+ object getRootModel()
Expand Down
3 changes: 3 additions & 0 deletions packages/concerto-core/changelog.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
# Note that the latest public API is documented using JSDocs and is available in api.txt.
#

Version 3.17.5 {9bd69f9522c14a99a085f077e12ac4b2} 2024-08-29
- importAliasing added to ModelManager parameters

Version 3.17.4 {8cb49c7092c48b568da3b0a3eeab2777} 2024-08-01
- Added api getImportedType() in modeFile

Expand Down
2 changes: 1 addition & 1 deletion packages/concerto-core/lib/basemodelmanager.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
// TODO Remove on release of MapType
// Supports both env var and property based flag
this.enableMapType = !!options?.enableMapType;
this.importAliasing = !!options?.importAliasing;
this.importAliasing = process.env.IMPORT_ALIASING || !!options?.importAliasing;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.importAliasing = process.env.IMPORT_ALIASING || !!options?.importAliasing;
this.importAliasing = process?.env?.IMPORT_ALIASING === 'true' || !!options?.importAliasing;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// Cache a copy of the Metamodel ModelFile for use when validating the structure of ModelFiles later.
this.metamodelModelFile = new ModelFile(this, MetaModelUtil.metaModelAst, undefined, MetaModelNamespace);
Expand Down Expand Up @@ -269,7 +269,7 @@
if (this.isStrict()) {
throw new MetamodelException(err.message);
} else {
console.warn('Invalid metamodel found. This will throw an exception in a future release. ', err.message);

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (16.x, ubuntu-latest)

Unexpected console statement

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (16.x, windows-latest)

Unexpected console statement

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (16.x, macOS-latest)

Unexpected console statement

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (18.x, ubuntu-latest)

Unexpected console statement

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (18.x, windows-latest)

Unexpected console statement

Check warning on line 272 in packages/concerto-core/lib/basemodelmanager.js

View workflow job for this annotation

GitHub Actions / Unit Tests (18.x, macOS-latest)

Unexpected console statement
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/concerto-core/lib/modelmanager.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class ModelManager extends BaseModelManager {
* @param {boolean} [options.strict] - require versioned namespaces and imports
* @param {Object} [options.regExp] - An alternative regular expression engine.
* @param {boolean} [options.enableMapType] - When true, the Concerto Map Type feature is enabled
* @param {boolean} [options.importAliasing] - When true, the Concerto Map Type feature is enabled
*/
constructor(options) {
super(options, ctoProcessFile(options));
Expand Down
2 changes: 1 addition & 1 deletion packages/concerto-core/types/lib/basemodelmanager.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ declare class BaseModelManager {
importAliasing?: boolean;
};
enableMapType: boolean;
importAliasing: boolean;
importAliasing: string | boolean;
metamodelModelFile: any;
/**
* Returns true
Expand Down
2 changes: 2 additions & 0 deletions packages/concerto-core/types/lib/modelmanager.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ declare class ModelManager extends BaseModelManager {
* @param {boolean} [options.strict] - require versioned namespaces and imports
* @param {Object} [options.regExp] - An alternative regular expression engine.
* @param {boolean} [options.enableMapType] - When true, the Concerto Map Type feature is enabled
* @param {boolean} [options.importAliasing] - When true, the Concerto Map Type feature is enabled
*/
constructor(options?: {
strict?: boolean;
regExp?: any;
enableMapType?: boolean;
importAliasing?: boolean;
});
/**
* Adds a model in CTO format to the ModelManager.
Expand Down
Loading