From b88db79fe8130973c7586ced06b8d62e46c2af2a Mon Sep 17 00:00:00 2001 From: Adam Simon Date: Mon, 6 Nov 2023 23:09:42 +0100 Subject: [PATCH] Handle non-string User Object attributes --- src/ConfigCatLogger.ts | 10 ++++ src/RolloutEvaluator.ts | 60 +++++++++++++++------ src/User.ts | 22 +++++--- test/UserTests.ts | 112 ++++++++++++++++++++++++++++++++++++---- 4 files changed, 170 insertions(+), 34 deletions(-) diff --git a/src/ConfigCatLogger.ts b/src/ConfigCatLogger.ts index ae0fe85..bfa2cd2 100644 --- a/src/ConfigCatLogger.ts +++ b/src/ConfigCatLogger.ts @@ -338,6 +338,16 @@ export class LoggerWrapper implements IConfigCatLogger { /* SDK-specific warning messages (4000-4999) */ + userObjectAttributeTypeIsInvalid(key: string, attributeName: string, attributeValue: string): LogMessage { + return this.log( + LogLevel.Warn, + 4004, + FormattableLogMessage.from( + "KEY", "ATTRIBUTE_NAME", "ATTRIBUTE_VALUE" + )`Evaluation of setting '${key}' may not produce the expected result (the User.${attributeName} attribute is not a string value, thus it was converted to '${attributeValue}' using the runtime's default conversion). User Object attribute values should be passed as strings. You can use the static helper methods of the \`User\` class to produce attribute values with the correct type and format.` + ); + } + /* Common info messages (5000-5999) */ settingEvaluated(evaluateLog: string): LogMessage { diff --git a/src/RolloutEvaluator.ts b/src/RolloutEvaluator.ts index 3568c25..725b3ee 100644 --- a/src/RolloutEvaluator.ts +++ b/src/RolloutEvaluator.ts @@ -11,10 +11,31 @@ import { getUserAttributes } from "./User"; import { errorToString, formatStringList, isArray, parseFloatStrict, utf8Encode } from "./Utils"; export class EvaluateContext { - private $userAttributes?: { [key: string]: string } | null; - get userAttributes(): { [key: string]: string } | null { + private $userAttributes?: { + readonly user: User; + map?: Readonly<{ [key: string]: string }>; + typeMismatches?: string[]; + }; + + get userAttributes(): Readonly<{ [key: string]: string }> | undefined { const attributes = this.$userAttributes; - return attributes !== void 0 ? attributes : (this.$userAttributes = this.user ? getUserAttributes(this.user) : null); + return attributes + ? attributes.map ??= getUserAttributes(attributes.user, attributes.typeMismatches = []) + : void 0; + } + + getUserAttribute(attributeName: string, key: string, logger: LoggerWrapper): string | undefined { + const attributes = this.userAttributes; + if (attributes) { + const attributeValue = attributes[attributeName]; + const typeMismatches = this.$userAttributes!.typeMismatches!; + let index: number; + if (attributeValue != null && (index = typeMismatches.indexOf(attributeName)) >= 0) { + logger.userObjectAttributeTypeIsInvalid(key, attributeName, attributeValue); + typeMismatches.splice(index, 1); + } + return attributeValue; + } } private $visitedFlags?: string[]; @@ -28,13 +49,20 @@ export class EvaluateContext { constructor( readonly key: string, readonly setting: Setting, - readonly user: User | undefined, - readonly settings: Readonly<{ [name: string]: Setting }> + readonly settings: Readonly<{ [name: string]: Setting }>, ) { } + static forMainFlag(key: string, setting: Setting, user: User | undefined, settings: Readonly<{ [name: string]: Setting }>): EvaluateContext { + const context = new EvaluateContext(key, setting, settings); + if (user) { + context.$userAttributes = { user }; + } + return context; + } + static forPrerequisiteFlag(key: string, setting: Setting, dependentFlagContext: EvaluateContext): EvaluateContext { - const context = new EvaluateContext(key, setting, dependentFlagContext.user, dependentFlagContext.settings); + const context = new EvaluateContext(key, setting, dependentFlagContext.settings); context.$userAttributes = dependentFlagContext.$userAttributes; context.$visitedFlags = dependentFlagContext.visitedFlags; // crucial to use the computed property here to make sure the list is created! context.logBuilder = dependentFlagContext.logBuilder; @@ -74,7 +102,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { logBuilder.append(`Evaluating '${context.key}'`); - if (context.user) { + if (context.userAttributes) { logBuilder.append(` for User '${JSON.stringify(context.userAttributes)}'`); } @@ -111,8 +139,8 @@ export class RolloutEvaluator implements IRolloutEvaluator { if (!isAllowedReturnValue) { throw new TypeError( returnValue === null ? "Setting value is null." : - returnValue === void 0 ? "Setting value is undefined." : - `Setting value '${returnValue}' is of an unsupported type (${typeof returnValue}).`); + returnValue === void 0 ? "Setting value is undefined." : + `Setting value '${returnValue}' is of an unsupported type (${typeof returnValue}).`); } return result; @@ -190,7 +218,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { private evaluatePercentageOptions(percentageOptions: ReadonlyArray, targetingRule: TargetingRule | undefined, context: EvaluateContext): IEvaluateResult | undefined { const logBuilder = context.logBuilder; - if (!context.user) { + if (!context.userAttributes) { logBuilder?.newLine("Skipping % options because the User Object is missing."); if (!context.isMissingUserObjectLogged) { @@ -202,7 +230,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { } const percentageOptionsAttributeName = context.setting.percentageOptionsAttribute; - const percentageOptionsAttributeValue = context.userAttributes![percentageOptionsAttributeName]; + const percentageOptionsAttributeValue = context.getUserAttribute(percentageOptionsAttributeName, context.key, this.logger); if (percentageOptionsAttributeValue == null) { logBuilder?.newLine(`Skipping % options because the User.${percentageOptionsAttributeName} attribute is missing.`); @@ -308,7 +336,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { const logBuilder = context.logBuilder; logBuilder?.appendUserCondition(condition); - if (!context.user) { + if (!context.userAttributes) { if (!context.isMissingUserObjectLogged) { this.logger.userObjectIsMissing(context.key); context.isMissingUserObjectLogged = true; @@ -318,7 +346,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { } const userAttributeName = condition.comparisonAttribute; - const userAttributeValue = context.userAttributes![userAttributeName]; + const userAttributeValue = context.getUserAttribute(userAttributeName, context.key, this.logger); if (!userAttributeValue) { // besides null and undefined, empty string is considered missing value as well this.logger.userObjectAttributeIsMissingCondition(formatUserCondition(condition), context.key, userAttributeName); return missingUserAttributeError(userAttributeName); @@ -647,7 +675,7 @@ export class RolloutEvaluator implements IRolloutEvaluator { const logBuilder = context.logBuilder; logBuilder?.appendSegmentCondition(condition); - if (!context.user) { + if (!context.userAttributes) { if (!context.isMissingUserObjectLogged) { this.logger.userObjectIsMissing(context.key); context.isMissingUserObjectLogged = true; @@ -816,7 +844,7 @@ export function evaluate(evaluator: IRolloutEvaluator, s return evaluationDetailsFromDefaultValue(key, defaultValue, getTimestampAsDate(remoteConfig), user, errorMessage); } - const evaluateResult = evaluator.evaluate(defaultValue, new EvaluateContext(key, setting, user, settings)); + const evaluateResult = evaluator.evaluate(defaultValue, EvaluateContext.forMainFlag(key, setting, user, settings)); return evaluationDetailsFromEvaluateResult(key, evaluateResult, getTimestampAsDate(remoteConfig), user); } @@ -835,7 +863,7 @@ export function evaluateAll(evaluator: IRolloutEvaluator, settings: Readonly<{ [ for (const [key, setting] of Object.entries(settings)) { let evaluationDetails: IEvaluationDetails; try { - const evaluateResult = evaluator.evaluate(null, new EvaluateContext(key, setting, user, settings)); + const evaluateResult = evaluator.evaluate(null, EvaluateContext.forMainFlag(key, setting, user, settings)); evaluationDetails = evaluationDetailsFromEvaluateResult(key, evaluateResult, getTimestampAsDate(remoteConfig), user); } catch (err) { diff --git a/src/User.ts b/src/User.ts index 1d2a403..5de8852 100644 --- a/src/User.ts +++ b/src/User.ts @@ -59,29 +59,35 @@ export class User { // NOTE: This could be an instance method of the User class, however formerly we suggested `const user = { ... }`-style initialization in the SDK docs, // which would lead to "...is not a function" errors if we called functions on instances created that way as those don't have the correct prototype. -export function getUserAttributes(user: User): { [key: string]: string } { - // TODO: https://trello.com/c/A3DDpqYU +export function getUserAttributes(user: User, typeMismatches?: string[]): { [key: string]: string } { + + function setAttribute(result: { [key: string]: string }, name: string, value: NonNullable, typeMismatches?: string[]) { + result[name] = typeof value === "string" + ? value + : (typeMismatches?.push(name), value + ""); + } + const result: { [key: string]: string } = {}; const identifierAttribute: WellKnownUserObjectAttribute = "Identifier"; const emailAttribute: WellKnownUserObjectAttribute = "Email"; const countryAttribute: WellKnownUserObjectAttribute = "Country"; - result[identifierAttribute] = user.identifier ?? ""; + setAttribute(result, identifierAttribute, user.identifier ?? "", typeMismatches); if (user.email != null) { - result[emailAttribute] = user.email; + setAttribute(result, emailAttribute, user.email, typeMismatches); } if (user.country != null) { - result[countryAttribute] = user.country; + setAttribute(result, countryAttribute, user.country, typeMismatches); } if (user.custom != null) { const wellKnownAttributes: string[] = [identifierAttribute, emailAttribute, countryAttribute]; - for (const attributeName of Object.keys(user.custom)) { - if (wellKnownAttributes.indexOf(attributeName) < 0) { - result[attributeName] = user.custom[attributeName]; + for (const [attributeName, attributeValue] of Object.entries(user.custom)) { + if (attributeValue != null && wellKnownAttributes.indexOf(attributeName) < 0) { + setAttribute(result, attributeName, attributeValue, typeMismatches); } } } diff --git a/test/UserTests.ts b/test/UserTests.ts index 0d63a27..623fdea 100644 --- a/test/UserTests.ts +++ b/test/UserTests.ts @@ -1,27 +1,61 @@ import { assert } from "chai"; import "mocha"; +import { LogLevel, LoggerWrapper } from "../src/ConfigCatLogger"; import { User } from "../src/index"; +import { Config } from "../src/ProjectConfig"; +import { RolloutEvaluator, evaluate } from "../src/RolloutEvaluator"; import { WellKnownUserObjectAttribute, getUserAttributes } from "../src/User"; import { parseFloatStrict } from "../src/Utils"; +import { FakeLogger } from "./helpers/fakes"; const identifierAttribute: WellKnownUserObjectAttribute = "Identifier"; const emailAttribute: WellKnownUserObjectAttribute = "Email"; const countryAttribute: WellKnownUserObjectAttribute = "Country"; +function createUser(attributeName: string, attributeValue: string) { + const user = new User(""); + switch (attributeName) { + case identifierAttribute: + user.identifier = attributeValue; + break; + case emailAttribute: + user.email = attributeValue; + break; + case countryAttribute: + user.country = attributeValue; + break; + default: + user.custom[attributeName] = attributeValue; + } + return user; +} + describe("User Object", () => { - for (const [identifier, expectedValue] of <[string, string][]>[ - [void 0, ""], - [null, ""], - ["", ""], - ["id", "id"], - ["\t", "\t"], - ["\u1F600", "\u1F600"], + for (const [attributeName, attributeValue, expectedValue] of <[string, string, string][]>[ + [identifierAttribute, void 0, ""], + [identifierAttribute, null, ""], + [identifierAttribute, "", ""], + [identifierAttribute, "id", "id"], + [identifierAttribute, "\t", "\t"], + [identifierAttribute, "\u1F600", "\u1F600"], + [emailAttribute, void 0, void 0], + [emailAttribute, null, void 0], + [emailAttribute, "", ""], + [emailAttribute, "a@example.com", "a@example.com"], + [countryAttribute, void 0, void 0], + [countryAttribute, null, void 0], + [countryAttribute, "", ""], + [countryAttribute, "US", "US"], + ["Custom1", void 0, void 0], + ["Custom1", null, void 0], + ["Custom1", "", ""], + ["Custom1", "3.14", "3.14"], ]) { - it(`Create user - should set id - identifier: ${identifier}`, () => { - const user = new User(identifier); + it(`Create user - should set attribute value - ${attributeName}: ${attributeValue}`, () => { + const user = createUser(attributeName, attributeValue); - assert.strictEqual(getUserAttributes(user)[identifierAttribute], expectedValue); + assert.strictEqual(getUserAttributes(user)[attributeName], expectedValue); }); } @@ -156,4 +190,62 @@ describe("User Object", () => { }); } + for (const [attributeName, attributeValue, comparisonValue] of <[string, unknown, string][]>[ + [identifierAttribute, false, "false"], + [emailAttribute, false, "false"], + [countryAttribute, false, "false"], + ["Custom1", false, "false"], + [identifierAttribute, 1.23, "1.23"], + [emailAttribute, 1.23, "1.23"], + [countryAttribute, 1.23, "1.23"], + ["Custom1", 1.23, "1.23"], + ]) { + it(`Non-string attribute values should be handled - attributeName: ${attributeName} | attributeValue: ${attributeValue}`, () => { + const configJson = { + "f": { + "test": { + "t": 0, + "r": [ + { + "c": [ + { + "u": { "a": attributeName, "c": 1, "l": [comparisonValue] } + } + ], + "s": { "v": { "b": false } } + }, + { + "c": [ + { + "u": { "a": attributeName, "c": 0, "l": [comparisonValue] } + } + ], + "s": { "v": { "b": true } } + }, + ], + "v": { "b": false } + } + } + }; + + const config = new Config(configJson); + const fakeLogger = new FakeLogger(); + const logger = new LoggerWrapper(fakeLogger); + const evaluator = new RolloutEvaluator(logger); + + const user = createUser(attributeName, attributeValue as any); + const evaluationDetails = evaluate(evaluator, config.settings, "test", null, user, null, logger); + const actualReturnValue = evaluationDetails.value; + + assert.isTrue(actualReturnValue); + + const warnings = fakeLogger.events.filter(([level]) => level === LogLevel.Warn); + + assert.strictEqual(1, warnings.length); + + const [, eventId] = warnings[0]; + assert.strictEqual(4004, eventId); + }); + } + });