Skip to content

Commit

Permalink
fix(#2553): remove config with no properties + support metadata check…
Browse files Browse the repository at this point in the history
… for config without properties (#2641)

## Proposed change
- Remove config with no properties
- Support metadata check for config without properties

## Related issues

<!--
Please make sure to follow the [contribution
guidelines](https://github.com/amadeus-digital/Otter/blob/main/CONTRIBUTING.md)
-->
<!-- * 🐛 Fix #issue -->
* 🐛 Fix resolves #2553
<!-- * 🚀 Feature #issue -->
<!-- * 🚀 Feature resolves #issue -->
<!-- * :octocat: Pull Request #issue -->
  • Loading branch information
matthieu-crouzet authored Jan 7, 2025
2 parents 0df8705 + 74f57c7 commit a2ea47b
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,9 @@ export class ComponentExtractor {
} else {
this.logger.warn(message);
}
if (propertiesWithDefaultValue.length === 0) {
return acc;
}
const configWithoutIncompatibleProperties = {
...config,
properties: propertiesWithDefaultValue
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import {
configMetadataComparator,
} from './config-metadata-comparison.helper';
import {
ComponentConfigOutput,
} from '@o3r/components';

describe('configMetadataComparator', () => {
describe('getArray', () => {
it('should return one element per configuration property', () => {
const result = configMetadataComparator.getArray([
{ properties: [
{ description: 'description', label: 'prop1', name: 'prop1', type: 'string' },
{ description: 'description', label: 'prop2', name: 'prop2', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT' }
]);
expect(result.length).toBe(2);
result.forEach((item) => {
expect(item.properties.length).toBe(1);
});
});

it('should return only configurations with properties', () => {
const result = configMetadataComparator.getArray([
{ properties: [], library: 'lib', name: 'ConfigWithoutProp', path: '', type: 'COMPONENT' },
{ properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT' }
]);
expect(result.length).toBe(1);
expect(result[0].name).toBe('ConfigWithProp');
});
});

describe('getIdentifier', () => {
it('should return an identifier with the property name', () => {
const result = configMetadataComparator.getIdentifier({
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
});
expect(result).toBe('lib#ConfigWithProp prop');
});

it('should return an identifier without property name if none available', () => {
const result = configMetadataComparator.getIdentifier({
properties: [], library: 'lib', name: 'ConfigWithoutProp', path: '', type: 'COMPONENT'
});
expect(result).toBe('lib#ConfigWithoutProp');
});
});

describe('isMigrationDataMatch', () => {
it('should return true', () => {
const metadata = {
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
} satisfies ComponentConfigOutput;
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp', propertyName: 'prop' }
)).toBeTruthy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp' }
)).toBeTruthy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib' }
)).toBeTruthy();
});

it('should return false', () => {
const metadata = {
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
} satisfies ComponentConfigOutput;
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'anotherLib', configName: 'ConfigWithProp', propertyName: 'prop' }
)).toBeFalsy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'AnotherConfig', propertyName: 'prop' }
)).toBeFalsy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp', propertyName: 'anotherProp' }
)).toBeFalsy();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export interface MigrationConfigData {
* ```
*/
const getConfigurationArray = (content: ComponentConfigOutput[]): ComponentConfigOutput[] => content.flatMap((config) =>
config.properties.length > 1
? config.properties.map((prop) => ({ ...config, properties: [prop] }))
: [config]
config.properties.map((prop) => ({ ...config, properties: [prop] }))
);

const getConfigurationPropertyName = (config: ComponentConfigOutput) => `${config.library}#${config.name}` + (config.properties.length > 0 ? ` ${config.properties[0].name}` : '');
Expand Down
73 changes: 54 additions & 19 deletions packages/@o3r/components/builders/metadata-check/index.it.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ import {
import {
inc,
} from 'semver';
import type {
MigrationConfigData,
import {
configMetadataComparator,
type MigrationConfigData,
} from './helpers/config-metadata-comparison.helper';
import type {
ComponentConfigOutput,
Expand Down Expand Up @@ -185,7 +186,10 @@ const previousConfigurationMetadata: ComponentConfigOutput[] = [
createConfig('@o3r/lib6', 'MyConfig6', ['prop6']),
createConfig('@o3r/lib7', 'MyConfig7', ['prop7']),
createConfig('@o3r/lib8', 'MyConfig8', ['prop8']),
createConfig('@o3r/lib9', 'MyConfig9', ['prop9'])
createConfig('@o3r/lib9', 'MyConfig9', ['prop9']),
// This case should not happen anymore as we filter config without properties
// Adding this case to ensure the support of older metadata
createConfig('@o3r/lib10', 'MyConfig10', [])
];

const newConfigurationMetadata: ComponentConfigOutput[] = [
Expand All @@ -198,7 +202,8 @@ const newConfigurationMetadata: ComponentConfigOutput[] = [
createConfig('@new/lib6', 'NewConfig6', ['newProp6Name']),
createConfig('@o3r/lib7', 'NewConfig7', ['prop7']),
createConfig('@new/lib8', 'MyConfig8', ['prop8']),
createConfig('@new/lib9', 'MyConfig9', ['prop9'])
createConfig('@new/lib9', 'MyConfig9', ['prop9']),
createConfig('@o3r/lib10', 'MyConfig10', ['prop10'])
];

async function writeFileAsJSON(path: string, content: object) {
Expand Down Expand Up @@ -298,6 +303,12 @@ const initTest = async (
await writeFileAsJSON(migrationDataPath, migrationData);
};

const getMessagesForId = (id: string) => ({
notDocumented: `Property ${id} has been modified but is not documented in the migration document`,
documentedButNotPresent: `Property ${id} has been modified but the new property is not present in the new metadata`,
breakingChangesNotAllowed: `Property ${id} is not present in the new metadata and breaking changes are not allowed`
});

describe('check metadata migration', () => {
test('should not throw', async () => {
await initTest(
Expand Down Expand Up @@ -344,11 +355,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).not.toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).not.toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand Down Expand Up @@ -379,11 +398,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).not.toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).not.toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand All @@ -408,11 +435,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).not.toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).not.toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@
}
}
]
}
},
"minItems": 1
}
}
}
Expand Down

0 comments on commit a2ea47b

Please sign in to comment.