feat: adds RequireFlagsEnabled decorator #1159

kaushalkapasi · 2025-03-14T20:08:14Z

This PR

Feature: Adds a RequireFlagsEnabled decorator to allow a simple, reusable way to block access to a specific controller or endpoint based on the value of a list of one, or many, boolean flags

Notes

Discussions on the approach & implementation are welcome!

Follow-up Tasks

Update OpenFeature NestJS docs to include new RequireFlagsEnabled decorator & usage examples

How to test

npx jest --selectProject=nest

toddbaert · 2025-03-20T19:43:05Z

I think this is a good idea, and it "feels right" to me ergonomically.

Make sure you add something to the test-app to test and document this.

lukas-reining

I really like the addition!
Added some optional additions to it. Feel free to add them, otherwise we can add them in another PR :)
Thank you!

lukas-reining · 2025-03-25T21:04:22Z

packages/nest/src/require-flags-enabled.decorator.ts

+   * Defaults to a 404 Not Found exception.
+   * @see {@link HttpException}
+   */
+  exception?: HttpException;


I think it is okay to constrain the exceptions to HTTP exceptions, even though it might make sense to broaden this to general exceptions.
Sometimes custom filters are built for more complex cases so I think one could argue for being strict on that or allowing any exception to be thrown.

Since these decorators are meant to be used at a Controller or Route level in Nest, I opted to limit the exception type to be an HTTPException since it can then be handled by the caller in a predictable manner.

lukas-reining · 2025-03-25T21:05:05Z

packages/nest/src/require-flags-enabled.decorator.ts

+/**
+ * Options for injecting a feature flag into a route handler.
+ */
+interface RequireFlagsEnabledProps {


I think we could have some optional static evaluation context given here too.

What would this look like? Is there an example you could share (maybe in another SDK) since I'm not fully familiar with that concept.

lukas-reining · 2025-03-25T21:10:13Z

packages/nest/src/require-flags-enabled.decorator.ts

+/**
+ * Options for injecting a feature flag into a route handler.
+ */
+interface RequireFlagsEnabledProps {


Also we could have a context factory here too. Basically what we globally have for the OpenFeatureModule:

js-sdk/packages/nest/src/open-feature.module.ts

Line 138 in cf89e7d

contextFactory?: ContextFactory;

But this would be optional to me. We do not have it for the other decorators too:

js-sdk/packages/nest/src/feature.decorator.ts

Line 36 in cf89e7d

interface FeatureProps<T extends FlagValue> {

So to me this is optional for this PR but it would be a great addition.

Sounds good, will do.

lukas-reining · 2025-03-25T21:13:00Z

packages/nest/src/require-flags-enabled.decorator.ts

+/**
+ * Returns a domain scoped or the default OpenFeature client with the given context.
+ * @param {string} domain The domain of the OpenFeature client.
+ * @returns {Client} The OpenFeature client.
+ */
+function getClientForEvaluation(domain?: string) {
+  return domain ? OpenFeature.getClient(domain) : OpenFeature.getClient();
+}


We already have this function:

js-sdk/packages/nest/src/feature.decorator.ts

Line 65 in cf89e7d

function getClientForEvaluation(domain?: string, context?: EvaluationContext) {

Should I change that to be an exported function in a utils.ts file that can be imported across decorators? Additionally, should the decorators live in their own folder or folders, along the lines of: nest/src/decorators/require-flags-enabled/require-flags-enabled.ts?

lukas-reining · 2025-03-25T21:14:39Z

packages/nest/src/require-flags-enabled.decorator.ts

+export const RequireFlagsEnabled = (props: RequireFlagsEnabledProps): ClassDecorator & MethodDecorator =>
+  applyDecorators(UseInterceptors(FlagsEnabledInterceptor(props)));


I think there could be value in similar function like: RequireFlagEquals.
But this could also be added in another PR.

Yeah! I opted to implement this first as the "easiest" option, a follow up would be to take a flag key and expected value combination to support String, Number and Object type flags.

Yep that makes sense to me. I think most people would use this current decorator, which could basically just be a shorthand for a RequireFlagEquals implementation. I think instead of a key/value combination though, a lambda predicate might be easier (a lambda taking the EvalutionDetails which will run to decide to run the request or not). For RequireFlagsEnabled, the lambda would just be evalutationDetails.value === true

lukas-reining · 2025-03-25T21:16:45Z

packages/nest/src/require-flags-enabled.decorator.ts

+      const client = getClientForEvaluation(props.domain);
+
+      for (const flagKey of props.flagKeys) {
+        const endpointAccessible = await client.getBooleanValue(flagKey, false);


Sometimes there are cases where it is better to fall back to true as default value if the flag evaluation fails.
I think it would be worth it adding the default value as a parameter, defaulting to false.

Sounds good, will do.

lukas-reining · 2025-03-25T21:23:42Z

packages/nest/src/require-flags-enabled.decorator.ts

+  flagKeys: string[];
+  /**
+   * The exception to throw if any of the required feature flags are not enabled.
+   * Defaults to a 404 Not Found exception.


I am not entirely sure why you chose 404 here.
Is it to prevent unwanted information disclosure in the case the feature is meant to be hidden?

That's right. Defaulting to 404 as the preferred behaviour is to "hide" the existence of the Controller or Route from the caller entirely. This could also be integrated with any API documentation that the end user may generate.

Ya I actually think for security reasons this is the most reasonable default.

lukas-reining · 2025-03-25T21:32:51Z

I will have a look at what is wrong with the e2e test tomorrow @kaushalkapasi :)

toddbaert · 2025-03-27T18:43:33Z

I will have a look at what is wrong with the e2e test tomorrow @kaushalkapasi :)

@lukas-reining @kaushalkapasi

Seems to me like the test-harness git submodule was simply deleted. I've added it back in the latest commit. The e2e tests are working now but you have some small lint errors causing CI failures.

toddbaert · 2025-03-27T19:09:51Z

packages/nest/src/require-flags-enabled.decorator.ts

+   * Global {@link EvaluationContext} for OpenFeature.
+   * @see {@link OpenFeature#setContext}
+   */
+  context?: EvaluationContext;


I'm a bit confused as to why we'd want to set the global context here?

It seems like a bit of a separation-of-concerns violation. Can somebody help me understand why it's useful here?

That's a mistake, it shouldn't be the global context. Will be updated to: * The {@link EvaluationContext} for evaluating the feature flag.

toddbaert · 2025-03-27T19:14:05Z

packages/nest/test/fixtures.ts

+  testBooleanFlag2: {
+    defaultVariant: 'default',
+    variants: { default: false, enabled: true },
+    disabled: false,
+  },


Could we add another integration test where we use a flag with a simple targeting rule, which requires an evaluation context value (which is injected in an interceptor) to be present? That would ensure that we are using a per-request context as expected in our decorator(s).

Sounds good, will add the test.

toddbaert

I have a couple questions, and one request for an additional test (please let me know if you think this is not necessary) but this looks really cool. Great work!

Happy to approve from my perspective after these are resolved.

…er & endpoint access based on boolean flag values Signed-off-by: Kaushal Kapasi <[email protected]>

…s enabled decorator Signed-off-by: Kaushal Kapasi <[email protected]>

Signed-off-by: Kaushal Kapasi <[email protected]>

…text and allow for flags to change the default value for flag evaluation. move getClientForEvaluation method to utils file Signed-off-by: Kaushal Kapasi <[email protected]>

Signed-off-by: Todd Baert <[email protected]>

Signed-off-by: Kaushal Kapasi <[email protected]>

…add tests with targeting rules defined for the InMemoryProvider Signed-off-by: Kaushal Kapasi <[email protected]>

beeme1mr requested a review from lukas-reining March 18, 2025 21:15

toddbaert requested review from toddbaert and luizgribeiro March 19, 2025 19:15

kaushalkapasi force-pushed the feat/require-flags-decorator branch 2 times, most recently from ca912f0 to a67cc35 Compare March 25, 2025 17:59

kaushalkapasi marked this pull request as ready for review March 25, 2025 18:00

kaushalkapasi requested a review from a team as a code owner March 25, 2025 18:00

lukas-reining approved these changes Mar 25, 2025

View reviewed changes

kaushalkapasi requested a review from lukas-reining March 26, 2025 15:54

toddbaert reviewed Mar 27, 2025

View reviewed changes

toddbaert self-requested a review March 27, 2025 19:16

toddbaert requested changes Mar 27, 2025

View reviewed changes

kaushalkapasi and others added 7 commits March 31, 2025 11:51

feat: adds RequireFlagsEnabled decorator to allow reusable controll…

cdc9f8b

…er & endpoint access based on boolean flag values Signed-off-by: Kaushal Kapasi <[email protected]>

fix: update imports for types and cleanup js docs on the require flag…

3c029b8

…s enabled decorator Signed-off-by: Kaushal Kapasi <[email protected]>

feat: add tests for RequireFlagsEnabled decorator

985b8cf

Signed-off-by: Kaushal Kapasi <[email protected]>

feat: update options for RequireFlagsEnabled decorator to include con…

b3e8174

…text and allow for flags to change the default value for flag evaluation. move getClientForEvaluation method to utils file Signed-off-by: Kaushal Kapasi <[email protected]>

fixup: restore sm

c141b50

Signed-off-by: Todd Baert <[email protected]>

chore: remove unused import to fix lint errors

d683796

Signed-off-by: Kaushal Kapasi <[email protected]>

fix: update docs for context definition on RequireFlagsEnabledProps. …

ba2c4d6

…add tests with targeting rules defined for the InMemoryProvider Signed-off-by: Kaushal Kapasi <[email protected]>

kaushalkapasi force-pushed the feat/require-flags-decorator branch from aa03a3a to ba2c4d6 Compare March 31, 2025 15:53

kaushalkapasi requested a review from toddbaert March 31, 2025 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds RequireFlagsEnabled decorator #1159

feat: adds RequireFlagsEnabled decorator #1159

kaushalkapasi commented Mar 14, 2025 •

edited

Loading

toddbaert commented Mar 20, 2025

lukas-reining left a comment

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

toddbaert Mar 27, 2025 •

edited

Loading

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

lukas-reining Mar 25, 2025

kaushalkapasi Mar 26, 2025

toddbaert Mar 27, 2025

lukas-reining commented Mar 25, 2025

toddbaert commented Mar 27, 2025 •

edited

Loading

toddbaert Mar 27, 2025

kaushalkapasi Mar 31, 2025

toddbaert Mar 27, 2025

kaushalkapasi Mar 28, 2025

toddbaert left a comment •

edited

Loading

		export const RequireFlagsEnabled = (props: RequireFlagsEnabledProps): ClassDecorator & MethodDecorator =>
		applyDecorators(UseInterceptors(FlagsEnabledInterceptor(props)));

feat: adds RequireFlagsEnabled decorator #1159

Are you sure you want to change the base?

feat: adds RequireFlagsEnabled decorator #1159

Conversation

kaushalkapasi commented Mar 14, 2025 • edited Loading

This PR

Notes

Follow-up Tasks

How to test

toddbaert commented Mar 20, 2025

lukas-reining 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

toddbaert Mar 27, 2025 • edited Loading

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

lukas-reining commented Mar 25, 2025

toddbaert commented Mar 27, 2025 • edited Loading

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

toddbaert left a comment • edited Loading

Choose a reason for hiding this comment

kaushalkapasi commented Mar 14, 2025 •

edited

Loading

toddbaert Mar 27, 2025 •

edited

Loading

toddbaert commented Mar 27, 2025 •

edited

Loading

toddbaert left a comment •

edited

Loading