From 9e8b16bca4e09cc3cedaa6ebbd8fa5a794ed7a0a Mon Sep 17 00:00:00 2001 From: y-lakhdar Date: Fri, 8 Apr 2022 17:28:07 -0400 Subject: [PATCH 1/2] fix: fix numeric type evolution --- src/fieldAnalyser/fieldAnalyser.spec.ts | 24 ++++++++++ src/fieldAnalyser/fieldAnalyser.ts | 23 ++++++---- src/fieldAnalyser/typeUtils.spec.ts | 60 +++++++++++++++++++------ src/fieldAnalyser/typeUtils.ts | 33 ++++++++------ 4 files changed, 106 insertions(+), 34 deletions(-) diff --git a/src/fieldAnalyser/fieldAnalyser.spec.ts b/src/fieldAnalyser/fieldAnalyser.spec.ts index 2e7b1a07..2607653b 100644 --- a/src/fieldAnalyser/fieldAnalyser.spec.ts +++ b/src/fieldAnalyser/fieldAnalyser.spec.ts @@ -157,6 +157,30 @@ describe('FieldAnalyser', () => { }, ]); }); + + it('should always take the most englobing numeric type regardless of the transition direction', async () => { + const docBuilders = [ + buildDocument({ + from_long_to_double: 10, + from_double_to_long: 20.1, + }), + buildDocument({ + from_long_to_double: 10.1, + from_double_to_long: 20, + }), + ]; + const {fields} = (await analyser.add(docBuilders)).report(); + expect(fields).toStrictEqual([ + { + name: 'from_long_to_double', + type: 'DOUBLE', + }, + { + name: 'from_double_to_long', + type: 'DOUBLE', + }, + ]); + }); }); describe('when the batch contains inconsistent metadata', () => { diff --git a/src/fieldAnalyser/fieldAnalyser.ts b/src/fieldAnalyser/fieldAnalyser.ts index 5a5b16f8..61084773 100644 --- a/src/fieldAnalyser/fieldAnalyser.ts +++ b/src/fieldAnalyser/fieldAnalyser.ts @@ -4,7 +4,7 @@ import {listAllFieldsFromOrg} from './fieldUtils'; import {FieldStore} from './fieldStore'; import {Inconsistencies} from './inconsistencies'; import {InvalidPermanentId} from '../errors/fieldErrors'; -import {getGuessedTypeFromValue, isValidTypeTransition} from './typeUtils'; +import {getGuessedTypeFromValue, getMostEnglobingType} from './typeUtils'; import {ensureNecessaryCoveoPrivileges} from '../validation/preconditions/apiKeyPrivilege'; import {writeFieldsPrivilege} from '../validation/preconditions/platformPrivilege'; @@ -77,17 +77,24 @@ export class FieldAnalyser { private storeMetadata(metadataKey: string, metadataValue: MetadataValue) { const alreadyGuessedType = this.missingFields.get(metadataKey); - const newGuessedType = getGuessedTypeFromValue(metadataValue); + const firstTypeGuess = getGuessedTypeFromValue(metadataValue); - if ( - !alreadyGuessedType || - isValidTypeTransition(alreadyGuessedType, newGuessedType) - ) { - this.missingFields.set(metadataKey, newGuessedType); + if (!alreadyGuessedType) { + this.missingFields.set(metadataKey, firstTypeGuess); + return; + } + + const secondTypeGuess = getMostEnglobingType( + alreadyGuessedType, + firstTypeGuess + ); + + if (secondTypeGuess) { + this.missingFields.set(metadataKey, secondTypeGuess); } else { this.inconsistencies.add(metadataKey, [ alreadyGuessedType, - newGuessedType, + firstTypeGuess, ]); } } diff --git a/src/fieldAnalyser/typeUtils.spec.ts b/src/fieldAnalyser/typeUtils.spec.ts index 9ab528ae..6ade1cf3 100644 --- a/src/fieldAnalyser/typeUtils.spec.ts +++ b/src/fieldAnalyser/typeUtils.spec.ts @@ -1,5 +1,5 @@ import {FieldTypes} from '@coveord/platform-client'; -import {getGuessedTypeFromValue, isValidTypeTransition} from './typeUtils'; +import {getGuessedTypeFromValue, getMostEnglobingType} from './typeUtils'; describe('typeUtils', () => { const signedIntegerLimit = 0b1111111111111111111111111111111; @@ -26,24 +26,58 @@ describe('typeUtils', () => { describe('when the transition is authorized', () => { it.each([ - {from: FieldTypes.LONG, to: FieldTypes.LONG}, - {from: FieldTypes.LONG, to: FieldTypes.LONG_64}, - {from: FieldTypes.LONG, to: FieldTypes.DOUBLE}, - {from: FieldTypes.LONG_64, to: FieldTypes.DOUBLE}, - {from: FieldTypes.DOUBLE, to: FieldTypes.DOUBLE}, - ])('should return true', ({from, to}) => { - expect(isValidTypeTransition(from, to)).toBe(true); + { + from: FieldTypes.LONG, + to: FieldTypes.LONG, + expectation: FieldTypes.LONG, + }, + { + from: FieldTypes.LONG, + to: FieldTypes.LONG_64, + expectation: FieldTypes.LONG_64, + }, + { + from: FieldTypes.LONG, + to: FieldTypes.DOUBLE, + expectation: FieldTypes.DOUBLE, + }, + { + from: FieldTypes.LONG_64, + to: FieldTypes.DOUBLE, + expectation: FieldTypes.DOUBLE, + }, + { + from: FieldTypes.DOUBLE, + to: FieldTypes.DOUBLE, + expectation: FieldTypes.DOUBLE, + }, + { + from: FieldTypes.LONG_64, + to: FieldTypes.LONG, + expectation: FieldTypes.LONG_64, + }, + { + from: FieldTypes.DOUBLE, + to: FieldTypes.LONG_64, + expectation: FieldTypes.DOUBLE, + }, + ])('should return true', ({from, to, expectation}) => { + expect(getMostEnglobingType(from, to)).toBe(expectation); }); }); describe('when the transition is not authorized', () => { it.each([ - {from: FieldTypes.LONG_64, to: FieldTypes.LONG}, - {from: FieldTypes.DOUBLE, to: FieldTypes.LONG_64}, - {from: FieldTypes.LONG_64, to: FieldTypes.STRING}, - {from: FieldTypes.STRING, to: FieldTypes.LONG}, + { + from: FieldTypes.LONG_64, + to: FieldTypes.STRING, + }, + { + from: FieldTypes.STRING, + to: FieldTypes.LONG, + }, ])('should return true', ({from, to}) => { - expect(isValidTypeTransition(from, to)).toBe(false); + expect(getMostEnglobingType(from, to)).toBe(null); }); }); }); diff --git a/src/fieldAnalyser/typeUtils.ts b/src/fieldAnalyser/typeUtils.ts index 14f3b586..86e97d12 100644 --- a/src/fieldAnalyser/typeUtils.ts +++ b/src/fieldAnalyser/typeUtils.ts @@ -1,21 +1,28 @@ import {FieldTypes} from '@coveord/platform-client'; -const acceptedTransitions: Array<{from: FieldTypes; to: Array}> = [ - {from: FieldTypes.LONG, to: [FieldTypes.LONG_64, FieldTypes.DOUBLE]}, - {from: FieldTypes.LONG_64, to: [FieldTypes.DOUBLE]}, +const acceptedTypeEvolutions = [ + [FieldTypes.LONG, FieldTypes.LONG_64, FieldTypes.DOUBLE], ]; -export function isValidTypeTransition( - currentState: FieldTypes, - nextState: FieldTypes -): boolean { - if (currentState === nextState) { - return true; +export function getMostEnglobingType( + possibleType1: FieldTypes, + possibleType2: FieldTypes +): FieldTypes | null { + if (possibleType1 === possibleType2) { + return possibleType1; } - const differentStateTransition = acceptedTransitions - .find((a) => a.from === currentState) - ?.to.includes(nextState); - return Boolean(differentStateTransition); + + for (const acceptedTypeEvolution of acceptedTypeEvolutions) { + if ( + acceptedTypeEvolution.includes(possibleType1) && + acceptedTypeEvolution.includes(possibleType2) + ) { + const idx1 = acceptedTypeEvolution.indexOf(possibleType1); + const idx2 = acceptedTypeEvolution.indexOf(possibleType2); + return acceptedTypeEvolution[Math.max(idx1, idx2)]; + } + } + return null; } export function getGuessedTypeFromValue(obj: unknown): FieldTypes { From 3644ce3482474a527da8affd1c6e82f219464f18 Mon Sep 17 00:00:00 2001 From: y-lakhdar Date: Wed, 4 May 2022 13:11:59 -0400 Subject: [PATCH 2/2] adjust spec --- src/fieldAnalyser/typeUtils.spec.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/fieldAnalyser/typeUtils.spec.ts b/src/fieldAnalyser/typeUtils.spec.ts index 6ade1cf3..6047f976 100644 --- a/src/fieldAnalyser/typeUtils.spec.ts +++ b/src/fieldAnalyser/typeUtils.spec.ts @@ -61,9 +61,12 @@ describe('typeUtils', () => { to: FieldTypes.LONG_64, expectation: FieldTypes.DOUBLE, }, - ])('should return true', ({from, to, expectation}) => { - expect(getMostEnglobingType(from, to)).toBe(expectation); - }); + ])( + 'From $from to $to should return $expectation', + ({from, to, expectation}) => { + expect(getMostEnglobingType(from, to)).toBe(expectation); + } + ); }); describe('when the transition is not authorized', () => { @@ -76,7 +79,7 @@ describe('typeUtils', () => { from: FieldTypes.STRING, to: FieldTypes.LONG, }, - ])('should return true', ({from, to}) => { + ])('From $from to $to should return $expectation', ({from, to}) => { expect(getMostEnglobingType(from, to)).toBe(null); }); });