[cascading] from release/11.4 to main (#2461)

## Cascading from release/11.4 to main The configuration requests the cascading to bypass reviewer in case of CI success. To not bypass the reviewing process, please check the following checkbox: - [ ]  🚫 stop reviewing process bypass for this Pull Request --- <small>This Pull Request has been generated with ❤️ by the [Otter](https://github.com/AmadeusITGroup/otter) cascading tool.</small>
AmadeusITGroup · Nov 14, 2024 · 16d19a7 · 16d19a7
2 parents b431c3d + ca337a9
commit 16d19a7
Show file tree

Hide file tree

Showing 12 changed files with 219 additions and 57 deletions.
diff --git a/...ages/@o3r/components/builders/metadata-check/helpers/config-metadata-comparison.helper.ts b/...ages/@o3r/components/builders/metadata-check/helpers/config-metadata-comparison.helper.ts
@@ -38,8 +38,9 @@ const getConfigurationArray = (content: ComponentConfigOutput[]): ComponentConfi
 
 const getConfigurationPropertyName = (config: ComponentConfigOutput) => `${config.library}#${config.name}` + (config.properties.length ? ` ${config.properties[0].name}` : '');
 
-const isMigrationConfigurationDataMatch = (config: ComponentConfigOutput, migrationData: MigrationConfigData, metadataType: string) =>
-  metadataType === 'CONFIG' &&
+const isRelevantContentType = (contentType: string) => contentType === 'CONFIG';
+
+const isMigrationConfigurationDataMatch = (config: ComponentConfigOutput, migrationData: MigrationConfigData) =>
   migrationData.libraryName === config.library
   && (!migrationData.configName || migrationData.configName === config.name)
   && (!migrationData.propertyName || config.properties[0]?.name === migrationData.propertyName);
@@ -50,5 +51,6 @@ const isMigrationConfigurationDataMatch = (config: ComponentConfigOutput, migrat
 export const configMetadataComparator: MetadataComparator<ComponentConfigOutput, MigrationConfigData, ComponentConfigOutput[]> = {
   getArray: getConfigurationArray,
   getIdentifier: getConfigurationPropertyName,
+  isRelevantContentType,
   isMigrationDataMatch: isMigrationConfigurationDataMatch
 };
diff --git a/packages/@o3r/components/builders/metadata-check/index.it.spec.ts b/packages/@o3r/components/builders/metadata-check/index.it.spec.ts
@@ -194,9 +194,17 @@ const initTest = async (
   newMetadata: ComponentConfigOutput[],
   migrationData: MigrationFile<MigrationConfigData>,
   packageNameSuffix: string,
-  options?: { allowBreakingChanges?: boolean; prerelease?: string }
+  options?: {
+    allowBreakingChanges?: boolean;
+    shouldCheckUnusedMigrationData?: boolean;
+    prerelease?: string;
+  }
 ) => {
-  const { allowBreakingChanges = false, prerelease } = options || {};
+  const {
+    allowBreakingChanges = false,
+    shouldCheckUnusedMigrationData = false,
+    prerelease
+  } = options || {};
   const { workspacePath, appName, applicationPath, o3rVersion, isYarnTest } = o3rEnvironment.testEnvironment;
   const execAppOptions = { ...getDefaultExecSyncOptions(), cwd: applicationPath };
   const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -227,6 +235,7 @@ const initTest = async (
     builder: '@o3r/components:check-config-migration-metadata',
     options: {
       allowBreakingChanges,
+      shouldCheckUnusedMigrationData,
       migrationDataPath: `apps/test-app/migration-scripts/MIGRATION-*.json`
     }
   };
@@ -275,7 +284,7 @@ describe('check metadata migration', () => {
       newConfigurationMetadata,
       defaultMigrationData,
       'allow-breaking-changes',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -288,7 +297,7 @@ describe('check metadata migration', () => {
       newConfigurationMetadata,
       defaultMigrationData,
       'allow-breaking-changes-prerelease',
-      { allowBreakingChanges: true, prerelease: 'rc' }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false, prerelease: 'rc' }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -304,7 +313,7 @@ describe('check metadata migration', () => {
         changes: []
       },
       'no-migration-data',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -337,7 +346,7 @@ describe('check metadata migration', () => {
         }))
       },
       'invalid-data',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -364,7 +373,7 @@ describe('check metadata migration', () => {
         changes: []
       },
       'breaking-changes',
-      { allowBreakingChanges: false }
+      { allowBreakingChanges: false, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -382,4 +391,35 @@ describe('check metadata migration', () => {
       });
     }
   });
+
+  test('should throw because of unused migration data', async () => {
+    const unusedMigrationItem = {
+      'contentType': 'CONFIG',
+      'before': {
+        'libraryName': 'fake-remove'
+      }
+    };
+    await initTest(
+      newConfigurationMetadata,
+      {
+        ...defaultMigrationData,
+        changes: [
+          ...defaultMigrationData.changes,
+          unusedMigrationItem
+        ]
+      },
+      'unused-migration-data',
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: true }
+    );
+    const { workspacePath, appName } = o3rEnvironment.testEnvironment;
+    const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
+
+    try {
+      packageManagerExec({ script: 'ng', args: ['run', `${appName}:check-metadata`] }, execAppOptionsWorkspace);
+      throw new Error('should have thrown before');
+    } catch (e: any) {
+      expect(e.message).not.toBe('should have thrown before');
+      expect(e.message).toContain(`The following migration data has been documented but no corresponding metadata change was found: ${JSON.stringify(unusedMigrationItem, null, 2)}`);
+    }
+  });
 });
diff --git a/packages/@o3r/components/builders/metadata-check/schema.json b/packages/@o3r/components/builders/metadata-check/schema.json
@@ -26,6 +26,11 @@
       "description": "Are breaking changes allowed.",
       "default": false
     },
+    "shouldCheckUnusedMigrationData": {
+      "type": "boolean",
+      "description": "Whether to throw an error in case of a migration item that is not used during metadata checks",
+      "default": false
+    },
     "packageManager": {
       "type": "string",
       "description": "Override of the package manager, otherwise it will be computed from the project setup."

diff --git a/packages/@o3r/extractors/src/core/comparator/metadata-comparator.interface.ts b/packages/@o3r/extractors/src/core/comparator/metadata-comparator.interface.ts
@@ -25,13 +25,18 @@ export interface MetadataComparator<MetadataItem, MigrationMetadataItem, Metadat
    */
   isSame?: (item1: MetadataItem, item2: MetadataItem) => boolean;
 
+  /**
+   * Returns true is the contentType is supported by the comparator
+   * @param contentType Content type of the migration item
+   */
+  isRelevantContentType: (contentType: string) => boolean;
+
   /**
    * Returns true if a migration item matches a metadata item.
    * @param metadataItem Metadata item
    * @param migrationItem Migration item
-   * @param metadataType Type of the metadata
    */
-  isMigrationDataMatch: (metadataItem: MetadataItem, migrationItem: MigrationMetadataItem, metadataType: string) => boolean;
+  isMigrationDataMatch: (metadataItem: MetadataItem, migrationItem: MigrationMetadataItem) => boolean;
 }
 
 /**
@@ -77,6 +82,9 @@ export interface MigrationMetadataCheckBuilderOptions extends JsonObject {
   /** Whether breaking changes are allowed.*/
   allowBreakingChanges: boolean;
 
+  /** Whether to throw an error in case of a migration item that is not used during metadata checks */
+  shouldCheckUnusedMigrationData: boolean;
+
   /** Override of the package manager, otherwise it will be determined from the project. */
   packageManager: SupportedPackageManagers;
 

diff --git a/packages/@o3r/extractors/src/core/comparator/metadata-comparison.helper.ts b/packages/@o3r/extractors/src/core/comparator/metadata-comparison.helper.ts
@@ -15,40 +15,51 @@ function checkMetadataFile<MetadataItem, MigrationMetadataItem, MetadataFile>(
   lastMetadataFile: MetadataFile,
   newMetadataFile: MetadataFile,
   migrationData: MigrationData<MigrationMetadataItem>[],
-  isBreakingChangeAllowed: boolean,
-  comparator: MetadataComparator<MetadataItem, MigrationMetadataItem, MetadataFile>
+  comparator: MetadataComparator<MetadataItem, MigrationMetadataItem, MetadataFile>,
+  options?: {allowBreakingChanges?: boolean; shouldCheckUnusedMigrationData?: boolean}
 ): Error[] {
+  const { allowBreakingChanges = false, shouldCheckUnusedMigrationData = false } = options || {};
   const errors: Error[] = [];
   const newMetadataArray = comparator.getArray(newMetadataFile);
   const lastMetadataArray = comparator.getArray(lastMetadataFile);
-  for (const lastValue of lastMetadataArray) {
-    const isInNewMetadata = newMetadataArray.some((newValue) =>
-      comparator.isSame
-        ? comparator.isSame(newValue, lastValue)
-        : comparator.getIdentifier(newValue) === comparator.getIdentifier(lastValue)
-    );
-    if (!isInNewMetadata) {
-      if (!isBreakingChangeAllowed) {
-        errors.push(new Error(`Property ${comparator.getIdentifier(lastValue)} is not present in the new metadata and breaking changes are not allowed`));
-        continue;
-      }
-
-      const migrationMetadataValue = migrationData.find((metadata) => metadata.before && comparator.isMigrationDataMatch(lastValue, metadata.before, metadata.contentType));
-
-      if (!migrationMetadataValue) {
-        errors.push(new Error(`Property ${comparator.getIdentifier(lastValue)} has been modified but is not documented in the migration document`));
-        continue;
-      }
-
-      if (migrationMetadataValue.after) {
-        const isNewValueInNewMetadata = newMetadataArray.some((newValue) => comparator.isMigrationDataMatch(newValue, migrationMetadataValue.after!, migrationMetadataValue.contentType));
-        if (!isNewValueInNewMetadata) {
-          errors.push(new Error(`Property ${comparator.getIdentifier(lastValue)} has been modified but the new property is not present in the new metadata`));
-          continue;
+  const relevantMigrationData = migrationData.filter((migrationItem) =>
+    comparator.isRelevantContentType(migrationItem.contentType));
+  const unusedMigrationData = new Set(relevantMigrationData);
+  const isSameMetadata = (a: MetadataItem, b: MetadataItem) => comparator.isSame
+    ? comparator.isSame(a, b)
+    : comparator.getIdentifier(a) === comparator.getIdentifier(b);
+  const removedMetadata = lastMetadataArray.filter((lastMetadata) =>
+    !newMetadataArray.some((newMetadata) => isSameMetadata(lastMetadata, newMetadata)));
+
+  // Check that removed metadata are properly documented
+  if (!allowBreakingChanges) {
+    errors.push(...removedMetadata.map((removedItem) =>
+      new Error(`Property ${comparator.getIdentifier(removedItem)} is not present in the new metadata and breaking changes are not allowed`)));
+  } else {
+    for (const removedItem of removedMetadata) {
+      const migrationMetadataValue = relevantMigrationData.find((metadata) =>
+        metadata.before && comparator.isMigrationDataMatch(removedItem, metadata.before));
+
+      if (migrationMetadataValue) {
+        unusedMigrationData.delete(migrationMetadataValue);
+        if (migrationMetadataValue.after) {
+          const isNewValueInNewMetadata = newMetadataArray.some((newValue) => comparator.isMigrationDataMatch(newValue, migrationMetadataValue.after!));
+          if (!isNewValueInNewMetadata) {
+            errors.push(new Error(`Property ${comparator.getIdentifier(removedItem)} has been modified but the new property is not present in the new metadata`));
+          }
         }
+      } else {
+        errors.push(new Error(`Property ${comparator.getIdentifier(removedItem)} has been modified but is not documented in the migration document`));
       }
     }
   }
+  // Check that migration data match metadata changes
+  if (shouldCheckUnusedMigrationData) {
+    for (const unusedMigrationItem of unusedMigrationData) {
+      errors.push(new Error(`The following migration data has been documented but no corresponding metadata change was found: ${JSON.stringify(unusedMigrationItem, null, 2)}`));
+    }
+  }
+
   return errors;
 }
 
@@ -161,7 +172,7 @@ Detection of package manager runner will fallback on the one used to execute the
   const newFile = getLocalMetadataFile<MetadataFile>(metadataPathInWorkspace);
 
 
-  const errors = checkMetadataFile<MetadataItem, MigrationMetadataItem, MetadataFile>(metadata, newFile, migrationData.changes, options.allowBreakingChanges, comparator);
+  const errors = checkMetadataFile<MetadataItem, MigrationMetadataItem, MetadataFile>(metadata, newFile, migrationData.changes, comparator, options);
 
   if (errors.length) {
     return {

diff --git a/...r/localization/builders/metadata-check/helpers/localization-metadata-comparison.helper.ts b/...r/localization/builders/metadata-check/helpers/localization-metadata-comparison.helper.ts
@@ -17,8 +17,10 @@ const getLocalizationArray = (content: LocalizationMetadata) => content;
 
 const getLocalizationName = (localization: JSONLocalization) => localization.key;
 
-const isMigrationLocalizationDataMatch = (localization: JSONLocalization, migrationData: MigrationLocalizationMetadata, metadataType: string) =>
-  metadataType === 'LOCALIZATION' && getLocalizationName(localization) === migrationData.key;
+const isRelevantContentType = (contentType: string) => contentType === 'LOCALIZATION';
+
+const isMigrationLocalizationDataMatch = (localization: JSONLocalization, migrationData: MigrationLocalizationMetadata) =>
+  getLocalizationName(localization) === migrationData.key;
 
 
 /**
@@ -27,5 +29,6 @@ const isMigrationLocalizationDataMatch = (localization: JSONLocalization, migrat
 export const localizationMetadataComparator: MetadataComparator<JSONLocalization, MigrationLocalizationMetadata, LocalizationMetadata> = {
   getArray: getLocalizationArray,
   getIdentifier: getLocalizationName,
+  isRelevantContentType,
   isMigrationDataMatch: isMigrationLocalizationDataMatch
 };
diff --git a/packages/@o3r/localization/builders/metadata-check/index.it.spec.ts b/packages/@o3r/localization/builders/metadata-check/index.it.spec.ts
@@ -70,9 +70,17 @@ const initTest = async (
   newMetadata: LocalizationMetadata,
   migrationData: MigrationFile<MigrationLocalizationMetadata>,
   packageNameSuffix: string,
-  options?: { allowBreakingChanges?: boolean; prerelease?: string }
+  options?: {
+    allowBreakingChanges?: boolean;
+    shouldCheckUnusedMigrationData?: boolean;
+    prerelease?: string;
+  }
 ) => {
-  const { allowBreakingChanges = false, prerelease } = options || {};
+  const {
+    allowBreakingChanges = false,
+    shouldCheckUnusedMigrationData = false,
+    prerelease
+  } = options || {};
   const { workspacePath, appName, applicationPath, o3rVersion, isYarnTest } = o3rEnvironment.testEnvironment;
   const execAppOptions = { ...getDefaultExecSyncOptions(), cwd: applicationPath };
   const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -103,6 +111,7 @@ const initTest = async (
     builder: '@o3r/localization:check-localization-migration-metadata',
     options: {
       allowBreakingChanges,
+      shouldCheckUnusedMigrationData,
       migrationDataPath: `apps/test-app/migration-scripts/MIGRATION-*.json`
     }
   };
@@ -151,7 +160,7 @@ describe('check metadata migration', () => {
       newLocalizationMetadata,
       defaultMigrationData,
       'allow-breaking-changes',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -164,7 +173,7 @@ describe('check metadata migration', () => {
       newLocalizationMetadata,
       defaultMigrationData,
       'allow-breaking-changes-prerelease',
-      { allowBreakingChanges: true, prerelease: 'rc' }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false, prerelease: 'rc' }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -180,7 +189,7 @@ describe('check metadata migration', () => {
         changes: []
       },
       'no-migration-data',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -212,7 +221,7 @@ describe('check metadata migration', () => {
         }))
       },
       'invalid-data',
-      { allowBreakingChanges: true }
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -238,7 +247,7 @@ describe('check metadata migration', () => {
         changes: []
       },
       'breaking-changes',
-      { allowBreakingChanges: false }
+      { allowBreakingChanges: false, shouldCheckUnusedMigrationData: false }
     );
     const { workspacePath, appName } = o3rEnvironment.testEnvironment;
     const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
@@ -255,4 +264,35 @@ describe('check metadata migration', () => {
       });
     }
   });
+
+  test('should throw because of unused migration data', async () => {
+    const unusedMigrationItem = {
+      'contentType': 'LOCALIZATION',
+      'before': {
+        'key': 'fake-remove'
+      }
+    };
+    await initTest(
+      newLocalizationMetadata,
+      {
+        ...defaultMigrationData,
+        changes: [
+          ...defaultMigrationData.changes,
+          unusedMigrationItem
+        ]
+      },
+      'unused-migration-data',
+      { allowBreakingChanges: true, shouldCheckUnusedMigrationData: true }
+    );
+    const { workspacePath, appName } = o3rEnvironment.testEnvironment;
+    const execAppOptionsWorkspace = { ...getDefaultExecSyncOptions(), cwd: workspacePath };
+
+    try {
+      packageManagerExec({ script: 'ng', args: ['run', `${appName}:check-metadata`] }, execAppOptionsWorkspace);
+      throw new Error('should have thrown before');
+    } catch (e: any) {
+      expect(e.message).not.toBe('should have thrown before');
+      expect(e.message).toContain(`The following migration data has been documented but no corresponding metadata change was found: ${JSON.stringify(unusedMigrationItem, null, 2)}`);
+    }
+  });
 });
diff --git a/packages/@o3r/localization/builders/metadata-check/schema.json b/packages/@o3r/localization/builders/metadata-check/schema.json
@@ -26,6 +26,11 @@
       "description": "Are breaking changes allowed.",
       "default": false
     },
+    "shouldCheckUnusedMigrationData": {
+      "type": "boolean",
+      "description": "Whether to throw an error in case of a migration item that is not used during metadata checks",
+      "default": false
+    },
     "packageManager": {
       "type": "string",
       "description": "Override of the package manager, otherwise it will be determined from the project."