FF-1749: JS SDK Common UFC update #45

schmit · 2024-03-28T01:44:16Z

Fixes: #FF-1749

Motivation and Context

Migrating the JS SDK common lib to the UFC to enable more flexible feature flag evaluation and advanced use cases.

Description

Leverage UFC to evaluate flags
Create explicit integer and numeric variation types
Remove the JSONString assignment (can but it back, but is anyone using it?), and renamed getJSONParsedAssignment to getJSONAssignment

To do:

obfuscation
remove assignment hooks
enforce specifying default value

How has this been tested?

Updated unit tests
Updated cross-SDK UFC test cases

schmit · 2024-03-29T06:26:15Z

src/client/eppo-client.ts

    assignmentHooks?: IAssignmentHooks,
  ): number | null;

-  getJSONStringAssignment(


I figured no one would use this -- hence removing it. Let me know if you disagree.

We provided it to support the transition of customers who were using getAssignment() with JSON before we added typed values, so it would be an easier transition for them.

I agree its odd, and I'm fine ripping it out and only adding it back if somebody complains.

schmit · 2024-03-29T06:31:54Z

src/client/eppo-client.ts

+        allocationKey: result.allocationKey ?? '__eppo_no_allocation',
+        variationKey: result.variation?.key ?? '__eppo_no_variation',


using placeholder values here to populate the cache

schmit · 2024-03-29T06:32:16Z

src/eppo_value.spec.ts

@@ -7,7 +7,7 @@ describe('EppoValue toHashedString function', () => {
  });

  it('is JsonType', () => {
-    const myInstance = EppoValue.JSON('{"hello":"world"}', { hello: 'world' });
+    const myInstance = EppoValue.JSON({ hello: 'world' });


no more string version of JSON blob

aarsilv

🎉 This is great stuff! I especially love the tests! 👏 Very excited for how it all cleans things up as well and simplifies things. 📈

I'm requesting changes for three(ish) things:

The current implementation which uses falseyness checks in a terinary for passing through default values breaks things in unexpected ways
I think we need more strictness around the newly added Integer type within the internals of EppoValue, such as when we check if the type is valid.
(this one may be controversial) I'd argue that our tests should have the actual, hard-coded values to decouple our obfuscating/unobfuscating. That being said, it would be a huge pain in the ass and make tests less readable. So maybe we could just rely on tests of our hashing and encoding in isolation.

aarsilv · 2024-04-01T21:32:44Z

src/assignment-logger.ts

@@ -28,7 +29,7 @@ export interface IAssignmentEvent {
  /**
   * The assigned variation
   */
-  variation: string;
+  variation: string | null;


When would a (logged) assigned variation be null?

When no allocation matches (or when the flag is disabled) and we return noneResult. Is the question that you don't expect such occurrenced to be logged?

Yes this is to enable logging the lack of assignment, which seems valuable especially because you probably want to figure out why assignments are not working

This makes sense if its a change we want. Note that currently, when no assignment is given, we do not log the null assignment.

aarsilv · 2024-04-01T21:34:37Z

src/client/eppo-client.ts

@@ -74,6 +47,7 @@ export interface IEppoClient {
    flagKey: string,
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    subjectAttributes?: Record<string, any>,
+    defaultValue?: string | null,


Note that this pattern does not force the user to provide a default value.

True, I left this as optional for now pending on a decision on the FF UX project

Updated: this is no longer optional

aarsilv · 2024-04-01T21:36:27Z

src/client/eppo-client.ts

    assignmentHooks?: IAssignmentHooks,
  ): number | null;

-  getJSONStringAssignment(


We provided it to support the transition of customers who were using getAssignment() with JSON before we added typed values, so it would be an easier transition for them.

I agree its odd, and I'm fine ripping it out and only adding it back if somebody complains.

src/client/eppo-client.ts

aarsilv · 2024-04-01T21:39:52Z

src/client/eppo-client.ts


  constructor(
+    evaluator: Evaluator,


Should we use a more specific name? configurationEvaluator? Would help if in the future we have other evaluators, and parallels the other argument names.

This should be used internally only -- I think it's fine to change this in the future if we do add other evaluators

src/rules.spec.ts

src/rules.ts

src/index.ts

src/rules.spec.ts

src/eval.ts

giorgiomartini0 · 2024-04-02T23:28:26Z

src/assignment-logger.ts

@@ -28,7 +29,7 @@ export interface IAssignmentEvent {
  /**
   * The assigned variation
   */
-  variation: string;
+  variation: string | null;


When no allocation matches (or when the flag is disabled) and we return noneResult. Is the question that you don't expect such occurrenced to be logged?

src/assignment-logger.ts

src/eval.spec.ts

src/client/eppo-client.spec.ts

giorgiomartini0 · 2024-04-02T23:50:36Z

Ugh, I always get tricked when reviewing in editor mode that changing the review type also shares the review... What I wanted to add: this is a true tour the force! Left a few minor comments and requests. Aaron's review already hit the (few) larger issues, and I didn't duplicate his comments.

src/interfaces.ts

src/client/eppo-client.ts

…47)

vpai · 2024-04-15T20:21:27Z

src/client/eppo-client.spec.ts

+  }
+}
+
+function getTestAssignments(


@schmit Could we export these two functions somewhere? That way we don't need to duplicate them in both JS and Node SDKs.

Great callout: 9915183

src/index.ts

aarsilv

Thanks for iterating on all the suggested changes! 🎉

I threw in some more minor comments and cleanup suggestions that shouldn't be too big of a lift.

Requesting further changes so we don't accidentally merge before updating to handle our latest obfuscation plan. 📆 You can see the (currently draft) PR for updating the test data here, and the UFC-generation changes to obfuscate in this PR.

I know it's annoying to be chasing a moving target 😞

aarsilv · 2024-04-17T13:24:31Z

src/client/eppo-client.spec.ts

+        const typeAssignmentFunctions = {
+          [VariationType.BOOLEAN]: client.getBoolAssignment.bind(client),
+          [VariationType.NUMERIC]: client.getNumericAssignment.bind(client),
+          [VariationType.INTEGER]: client.getIntegerAssignment.bind(client),
+          [VariationType.STRING]: client.getStringAssignment.bind(client),
+          [VariationType.JSON]: client.getJSONAssignment.bind(client),
+        };
+
+        const assignmentFn = typeAssignmentFunctions[variationType];
+        if (!assignmentFn) {
+          throw new Error(`Unknown variation type: ${variationType}`);
+        }


aarsilv · 2024-04-17T13:27:31Z

src/client/eppo-client.spec.ts

+      async ({ flag, variationType, defaultValue, subjects }: IAssignmentTestCase) => {
+        const client = new EppoClient(storage, undefined, true);
+
+        const typeAssignmentFunctions = {


This through line 290 is all duplicated code and could be consolidated into a helper function

aarsilv · 2024-04-17T13:30:55Z

src/client/eppo-client.ts

  getStringAssignment(
    subjectKey: string,
    flagKey: string,
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    subjectAttributes?: Record<string, any>,
-    assignmentHooks?: IAssignmentHooks,
-  ): string | null;
+    defaultValue: string,
+    subjectAttributes?: Record<string, AttributeType>,
+  ): string;


Note that Optimizely and LaunchDarkly have flag keys first

aarsilv · 2024-04-17T13:31:38Z

src/client/eppo-client.ts

    this.configurationStore = configurationStore;
    this.configurationRequestParameters = configurationRequestParameters;
+    this.isObfuscated = obfuscated;


aarsilv · 2024-04-17T13:34:20Z

src/client/eppo-client.ts

-      obfuscated ? getMD5Hash(flagKey) : flagKey,
-    );
-    const allowListOverride = this.getSubjectVariationOverride(
+    const result = this.evaluator.evaluateFlag(


Like our pattern on the server side, I think it may be best to have an optional step before this one where we unobfuscated the flag if it's obfuscated and then pass that to the evaluator, who only needs to worry about unobfuscated flags.

Nevermind, we can't do this with hashes

aarsilv · 2024-04-17T13:49:50Z

src/version.ts

@@ -0,0 +1 @@
+export const LIB_VERSION = '3.0.0';


If we do want to log the version of this common module, we shouldn't hard code this and instead pull it from package.json

aarsilv · 2024-04-17T13:51:52Z

src/client/eppo-client.ts

+      metaData: {
+        obfuscated: this.isObfuscated,
+        sdkLanguage: 'javascript',
+        sdkLibVersion: LIB_VERSION,


This technically won't be the SDK version because this is just a dependency of that SDK, which will have its own version number. That version number is still pulled as it's used as a URL parameter. So if we want to include this we should make it a parameter in the constructor and have the upstream SDKs pass it.

Right but because this library contains all the logic, it seems more relevant to log this libary's function than the "proper" SDK one.

aarsilv · 2024-04-17T13:55:07Z

src/client/eppo-client.ts

-        return nullAssignment;
+    if (
+      result?.variation &&
+      !this.checkValueTypeMatch(expectedVariationType, result.variation.value)


Note that result.variation.value is now encoded and needs to be de-encoded

aarsilv · 2024-04-17T13:56:43Z

src/client/eppo-client.ts

+        return defaultValue;
+      }
+
+      return EppoValue.valueOf(result.variation.value, expectedVariationType);


Note that now result.variation.value will now be encoded and needs to be decoded

aarsilv · 2024-04-17T13:58:23Z

src/client/eppo-client.ts

+      allocation: result.allocationKey ?? null,
+      experiment: result.allocationKey ? `${result.flagKey}-${result.allocationKey}` : null,
+      featureFlag: result.flagKey,
+      variation: result.variation?.key ?? null,


Note that result.allocationKey, result.flagKey, and result.variation.key will all be obfuscated and should now be deobfuscated upstream.

aarsilv

Approving now as signatures match and the obfuscation moving target has been caught up with! 🏎️

schmit added 10 commits March 25, 2024 16:59

wip

f69cdc6

making progress

07b9cde

make more tests pass

fd04b4b

wip

ca7ea07

fix more tests

2a7b3ec

update event cache test

d676f4c

.bind(this)

12eb7d1

pass all the tests

e9bb4b3

Update script to create obfuscated UFC

684da8a

add obfuscation and make all tests pass

aa64366

schmit marked this pull request as ready for review March 29, 2024 06:25

schmit commented Mar 29, 2024

View reviewed changes

cleanup

4d6264e

schmit requested review from giorgiomartini0, leoromanovsky, aarsilv and sameerank March 29, 2024 06:37

aarsilv requested changes Apr 2, 2024

View reviewed changes

aarsilv assigned schmit Apr 2, 2024

giorgiomartini0 requested changes Apr 2, 2024

View reviewed changes

schmit commented Apr 3, 2024

View reviewed changes

src/interfaces.ts Outdated Show resolved Hide resolved

schmit added 6 commits April 3, 2024 10:38

Address PR comments

1e9e10e

Address PR comments

cab5b1f

Hardcoded obfuscation tests and other fixes

e5d4be1

test for overwriting id for matching rules

7400ed0

remove unused import

4d9a3a0

remove unneeded lint comment

67f7cbc

schmit added 4 commits April 4, 2024 14:51

update version

370d80a

bump version

d058c46

Merge branch 'main' into ufc-revamp

9ce18fb

dont be fancy

cf991bc

vpai reviewed Apr 9, 2024

View reviewed changes

src/client/eppo-client.ts Show resolved Hide resolved

vpai reviewed Apr 10, 2024

View reviewed changes

src/client/eppo-client.ts Outdated Show resolved Hide resolved

schmit and others added 3 commits April 10, 2024 13:41

remove evaluator as arg to eppo client constructor

6d063c4

Replace lru-cache package with simple local implementation (FF-1876) (#…

a5cfe95

…47)

Merge branch 'main' into ufc-revamp

bafd881

vpai mentioned this pull request Apr 12, 2024

update js client to support ufc update Eppo-exp/js-client-sdk#55

Merged

update endpoint to match backend

5958d19

vpai mentioned this pull request Apr 12, 2024

update node client to support ufc update Eppo-exp/node-server-sdk#52

Merged

[ufc] add null operator and more fixes (#50)

ee3d43a

vpai reviewed Apr 15, 2024

View reviewed changes

export test helpers

9915183

schmit requested a review from aarsilv April 15, 2024 20:28

vpai reviewed Apr 15, 2024

View reviewed changes

src/index.ts Show resolved Hide resolved

schmit added 3 commits April 15, 2024 15:17

export flag and variationtype

fd6e357

export attribute type

a28e473

export more types

08e36c8

schmit unassigned giorgiomartini0 Apr 16, 2024

aarsilv requested changes Apr 17, 2024

View reviewed changes

aarsilv assigned schmit and unassigned aarsilv Apr 17, 2024

schmit added 3 commits April 17, 2024 20:11

Update function signatures

7735cc6

[UFC] Update obfuscation decoding (#52)

7b25f18

Merge branch 'main' into ufc-revamp

555dece

aarsilv approved these changes Apr 19, 2024

View reviewed changes

schmit merged commit 7fd5a4a into main Apr 19, 2024
2 checks passed

leoromanovsky mentioned this pull request Apr 23, 2024

Initial implementation Eppo-exp/eppo-multiplatform#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FF-1749: JS SDK Common UFC update #45

FF-1749: JS SDK Common UFC update #45

schmit commented Mar 28, 2024 •

edited

Loading

schmit Mar 29, 2024

aarsilv Apr 1, 2024

schmit Mar 29, 2024

schmit Mar 29, 2024

aarsilv left a comment

aarsilv Apr 1, 2024

giorgiomartini0 Apr 2, 2024

schmit Apr 3, 2024

aarsilv Apr 3, 2024

aarsilv Apr 1, 2024

schmit Apr 2, 2024

schmit Apr 3, 2024

aarsilv Apr 1, 2024

aarsilv Apr 1, 2024

schmit Apr 3, 2024

giorgiomartini0 Apr 2, 2024

giorgiomartini0 commented Apr 2, 2024

vpai Apr 15, 2024

schmit Apr 15, 2024

aarsilv left a comment

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 19, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

schmit Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv Apr 17, 2024

aarsilv left a comment

		allocationKey: result.allocationKey ?? '__eppo_no_allocation',
		variationKey: result.variation?.key ?? '__eppo_no_variation',

FF-1749: JS SDK Common UFC update #45

FF-1749: JS SDK Common UFC update #45

Conversation

schmit commented Mar 28, 2024 • edited Loading

Motivation and Context

Description

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarsilv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiomartini0 commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarsilv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarsilv left a comment

Choose a reason for hiding this comment

schmit commented Mar 28, 2024 •

edited

Loading