Add built-in TargetingFilter impl #5
Conversation
Eskibear
requested review from
avanigupta,
mrm9084,
rossgrambo and
zhiyuanliang-ms
Eskibear
force-pushed
the
yanzh/targeting-filter
branch
from
fdb45ae to
ee83ae8
Compare
…ipt into yanzh/targeting-filter
…ipt into yanzh/targeting-filter
| // Cryptographic hashing algorithms ensure adequate entropy across hash values. | ||
| const contextMarker = stringToUint32(audienceContextId); | ||
| const contextPercentage = (contextMarker / 0xFFFFFFFF) * 100; | ||
| return contextPercentage < rolloutPercentage; |
There was a problem hiding this comment.
@Eskibear Did you validate that this produces the same number as dotnet/python?
| if (parameters.Audience.Groups !== undefined) { | ||
| for (const group of parameters.Audience.Groups) { | ||
| if (group.RolloutPercentage < 0 || group.RolloutPercentage > 100) { | ||
| throw new Error(`RolloutPercentage of group ${group.Name} must be a number between 0 and 100.`); |
There was a problem hiding this comment.
"between 0 and 100" makes it sound like it needs to be 1-99. Could do any of:
- "must be no less than 0 and no more than 100."
- "must be a number between 0 and 100, inclusive.
- "is out of the accepted range." (this is what dotnet says today- but it's less clear)
| // Licensed under the MIT license. | ||
|
|
||
| import { IFeatureFilter } from "./FeatureFilter"; | ||
| import { createHash } from "crypto"; |
There was a problem hiding this comment.
The portal team used CryptoJS. I imagine either works- but might make sense to align on one https://msazure.visualstudio.com/Azure%20AppConfig/_git/Portal?path=/AppConfigPortalExtension/AppConfigPortalExtension/Client/Resource/FeatureManager/ResourceFeatureFlagsBlade.ts&version=GBmaryanne/feature/splitExperimentation&_a=contents
We will need to use Base64 in the future if that helps decide.
|
|
||
| export interface IFeatureFilter { | ||
| name: string; // e.g. Microsoft.TimeWindow | ||
| evaluate(context: IFeatureFilterEvaluationContext, appContext?: unknown): Promise<boolean> | boolean; |
There was a problem hiding this comment.
This interface should just return Promise. The calling code is forced to use await anyway since this function might return a Promise. If await is used on a non-promise, it simply wraps it in an immediately resolved Promise anyway.
I believe the implementation can just stay "return true" and things will work fine- as long as the function is marked async.
| export class TargetingFilter implements IFeatureFilter { | ||
| name: string = "Microsoft.Targeting"; | ||
|
|
||
| evaluate(context: TargetingFilterEvaluationContext, appContext?: TargetingFilterAppContext): boolean | Promise<boolean> { |
There was a problem hiding this comment.
This feels like a lie- since we don't ever return Promise. I prefer marking this async and only doing Promise.
There was a problem hiding this comment.
@rossgrambo Good catch. That was a miss.
According to current impl, it returns a boolean instead of a promise, without any async operations. context is read from feature flags already loaded, and appContext is provided directly in parameter.
I'll create a PR changing it to
evaluate(context: TargetingFilterEvaluationContext, appContext?: TargetingFilterAppContext): booleanWDYT?
Same as other language, TargetingFilter will be one of the built-in filters. This PR implements that.