Skip to content

Commit

Permalink
Handle non-string User Object attributes
Browse files Browse the repository at this point in the history
  • Loading branch information
@adams85
adams85 committed Nov 7, 2023
1 parent a43ef4c commit 54f1237
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 34 deletions.
10 changes: 10 additions & 0 deletions src/ConfigCatLogger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
60 changes: 44 additions & 16 deletions src/RolloutEvaluator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand All @@ -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;
Expand Down Expand Up @@ -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)}'`);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -190,7 +218,7 @@ export class RolloutEvaluator implements IRolloutEvaluator {
private evaluatePercentageOptions(percentageOptions: ReadonlyArray<PercentageOption>, 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) {
Expand All @@ -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.`);

Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -816,7 +844,7 @@ export function evaluate<T extends SettingValue>(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<T>(key, evaluateResult, getTimestampAsDate(remoteConfig), user);
}
Expand All @@ -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) {
Expand Down
22 changes: 14 additions & 8 deletions src/User.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<unknown>, 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);
}
}
}
Expand Down
112 changes: 102 additions & 10 deletions test/UserTests.ts
View file
Original file line number Diff line number Diff line change
@@ -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][]>[
["Identifier", void 0, ""],
["Identifier", null, ""],
["Identifier", "", ""],
["Identifier", "id", "id"],
["Identifier", "\t", "\t"],
["Identifier", "\u1F600", "\u1F600"],
["Email", void 0, void 0],
["Email", null, void 0],
["Email", "", ""],
["Email", "a@example.com", "a@example.com"],
["Country", void 0, void 0],
["Country", null, void 0],
["Country", "", ""],
["Country", "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);
});
}

Expand Down Expand Up @@ -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);
});
}

});

0 comments on commit 54f1237

Please sign in to comment.