From d9998aef164676b3be55042a63008ffff7bced1f Mon Sep 17 00:00:00 2001 From: Zeping Bai Date: Wed, 24 Apr 2024 20:30:41 +0800 Subject: [PATCH] fix: differ output behavior (#48) * fix: differ output behavior * simplify --- apps/cli/src/differ/differv3.ts | 86 ++++++++++++++----------- apps/cli/src/differ/specs/basic.spec.ts | 56 +++++++++++++++- 2 files changed, 102 insertions(+), 40 deletions(-) diff --git a/apps/cli/src/differ/differv3.ts b/apps/cli/src/differ/differv3.ts index 5293d29..82c4a70 100644 --- a/apps/cli/src/differ/differv3.ts +++ b/apps/cli/src/differ/differv3.ts @@ -360,41 +360,8 @@ export class DifferV3 { localItem, ); - // If the resource may contain plugin configurations, perform a - // diff check on each plugin - let pluginChanged = false; - if ( - resourceType === ADCSDK.ResourceType.SERVICE || - resourceType === ADCSDK.ResourceType.CONSUMER || - resourceType === ADCSDK.ResourceType.GLOBAL_RULE || - resourceType === ADCSDK.ResourceType.ROUTE || - resourceType === ADCSDK.ResourceType.STREAM_ROUTE || - resourceType === ADCSDK.ResourceType.CONSUMER_GROUP || - resourceType === ADCSDK.ResourceType.PLUGIN_CONFIG - ) { - let mergedLocalPlugins: ADCSDK.Plugins = {}; - [pluginChanged, mergedLocalPlugins] = this.diffPlugins( - 'plugins' in mergedLocalItem - ? (cloneDeep(mergedLocalItem.plugins) as ADCSDK.Plugins) - : {}, - 'plugins' in remoteItem ? (remoteItem.plugins as ADCSDK.Plugins) : {}, - ); - - // If no changes are detected on all plugins, the plugins field - // is removed from the current resource. - // Since checking for resource changes at the plugin level considers - // those default values added by the backend, they need to be removed. - // When checking for changes at the main resource level, the default - // value will be treated as a modification on the plugin's configuration, - // which should not be the case. - if (!pluginChanged) { - unset(mergedLocalItem, 'plugins'); - unset(remoteItem, 'plugins'); - } else { - //@ts-expect-error it has been asserted that the resource type can contain the plugins field - mergedLocalItem.plugins = mergedLocalPlugins; - } - } + let outputLocalItem: ADCSDK.Resource; + let outputRemoteItem: ADCSDK.Resource; // Special handling of resources containing nested resources: routes, consumer_groups const subEvents: Array = []; @@ -445,13 +412,53 @@ export class DifferV3 { }); } + // If the resource may contain plugin configurations, perform a + // diff check on each plugin + let pluginChanged = false; + if ( + resourceType === ADCSDK.ResourceType.SERVICE || + resourceType === ADCSDK.ResourceType.CONSUMER || + resourceType === ADCSDK.ResourceType.GLOBAL_RULE || + resourceType === ADCSDK.ResourceType.ROUTE || + resourceType === ADCSDK.ResourceType.STREAM_ROUTE || + resourceType === ADCSDK.ResourceType.CONSUMER_GROUP || + resourceType === ADCSDK.ResourceType.PLUGIN_CONFIG + ) { + let mergedLocalPlugins: ADCSDK.Plugins = {}; + [pluginChanged, mergedLocalPlugins] = this.diffPlugins( + 'plugins' in mergedLocalItem + ? (cloneDeep(mergedLocalItem.plugins) as ADCSDK.Plugins) + : {}, + 'plugins' in remoteItem ? (remoteItem.plugins as ADCSDK.Plugins) : {}, + ); + + //@ts-expect-error it has been asserted that the resource type can contain the plugins field + mergedLocalItem.plugins = mergedLocalPlugins; + + // The cache is used to output the old and new values of the diff report. + // They are characterized by the fact that they do not contain subresources + // such as route; however, they should be the current resource's containing + // the plugins field. + outputLocalItem = cloneDeep(mergedLocalItem); + outputRemoteItem = cloneDeep(remoteItem); + + // Since plugins will be checked separately, differences in the plugins + // should not be considered when checking for current resource changes. + if (!pluginChanged) { + unset(mergedLocalItem, 'plugins'); + unset(remoteItem, 'plugins'); + } + } + // Checking other resource properties, lhs is old(remote), rhs is new(local) + // The resources checked are exclusively the current resource itself, without + // plugins and sub-resources. const diff = objectDiff(cloneDeep(remoteItem), mergedLocalItem); this.logger.debug({ message: 'main resource diff', diff, - local: mergedLocalItem, - remote: remoteItem, + local: outputLocalItem, + remote: outputRemoteItem, transactionId: this.transactionId, }); @@ -477,10 +484,10 @@ export class DifferV3 { : ADCSDK.EventType.UPDATE, resourceId: remoteId, resourceName: remoteName, - oldValue: remoteItem, + oldValue: outputRemoteItem, // Merged defaults should not be included to prevent bothering the user - newValue: mergedLocalItem, + newValue: outputLocalItem, // Even if only the plugin part has changed, the difference is recorded // on the whole resource. @@ -533,6 +540,7 @@ export class DifferV3 { local, remote, emptyLocal: isEmpty(local), + emptyRemote: isEmpty(remote), transactionId: this.transactionId, }); if (isEmpty(local) && isEmpty(remote)) return [false, local]; diff --git a/apps/cli/src/differ/specs/basic.spec.ts b/apps/cli/src/differ/specs/basic.spec.ts index febcb6f..b0fe1f2 100644 --- a/apps/cli/src/differ/specs/basic.spec.ts +++ b/apps/cli/src/differ/specs/basic.spec.ts @@ -1,4 +1,5 @@ import * as ADCSDK from '@api7/adc-sdk'; +import { unset } from 'lodash'; import { DifferV3 } from '../differv3'; @@ -204,7 +205,7 @@ describe('Differ V3 - basic', () => { ], }, { - plugin: { + plugins: { 'key-auth': { added: 'added' }, }, }, @@ -476,4 +477,57 @@ describe('Differ V3 - basic', () => { }, ]); }); + + it('should keep plugins when plugins not be changed', () => { + const serviceName = 'Test Service'; + const oldService: ADCSDK.Service = { + name: serviceName, + plugins: { + test: { + testKey: 'testValue', + added: 'added', + }, + }, + }; + + const newService = structuredClone(oldService); + newService.path_prefix = '/test'; + + // ensure local resource does not include default added field + unset(newService, 'plugins.test.added'); + + expect( + DifferV3.diff( + { + services: [structuredClone(newService)], + }, + { + services: [structuredClone(oldService)], + }, + { + plugins: { + test: { + added: 'added', + }, + }, + }, + ), + ).toEqual([ + { + diff: [ + { + kind: 'N', + path: ['path_prefix'], + rhs: '/test', + }, + ], + newValue: newService, + oldValue: oldService, + resourceId: ADCSDK.generateId(serviceName), + resourceName: serviceName, + resourceType: ADCSDK.ResourceType.SERVICE, + type: ADCSDK.EventType.UPDATE, + }, + ]); + }); });