Port existing Feature Flag implementation into Cellix - #317
Open
aaron-rabinowitz wants to merge 4 commits into
Open
Port existing Feature Flag implementation into Cellix#317aaron-rabinowitz wants to merge 4 commits into
aaron-rabinowitz wants to merge 4 commits into
Conversation
Contributor
Reviewer's GuidePorts EFDO-style feature flag support into the OCOM Cellix stack by adding backend blob-based feature flag retrieval with JSON-schema validation, a shared React feature flag provider with caching and fallbacks, wiring both UI apps to use it, and updating dependencies/config for security and JSON imports. Sequence diagram for backend blob feature flag retrievalsequenceDiagram
actor AppService
participant ServiceBlobStorage
participant BlobServiceClient
participant AjvValidator
AppService->>ServiceBlobStorage: getFeatureFlags()
ServiceBlobStorage->>ServiceBlobStorage: downloadBlobToString("public", FEATURE_FLAG_BLOB_NAME)
ServiceBlobStorage->>BlobServiceClient: getContainerClient("public")
BlobServiceClient-->>ServiceBlobStorage: containerClient
ServiceBlobStorage->>BlobServiceClient: getBlockBlobClient(FEATURE_FLAG_BLOB_NAME)
BlobServiceClient-->>ServiceBlobStorage: blockBlobClient
ServiceBlobStorage->>BlobServiceClient: downloadToBuffer()
alt blob exists
BlobServiceClient-->>ServiceBlobStorage: Buffer
ServiceBlobStorage->>ServiceBlobStorage: buffer.toString("utf-8")
ServiceBlobStorage-->>ServiceBlobStorage: featureFlagsRaw
else isBlobNotFoundError(error)
ServiceBlobStorage-->>ServiceBlobStorage: featureFlagsRaw = FeatureFlagsLocal
end
ServiceBlobStorage->>AjvValidator: compile(FeatureFlagsSchema)
AjvValidator-->>ServiceBlobStorage: validate
ServiceBlobStorage->>AjvValidator: validate(dataJson)
alt validation passes
ServiceBlobStorage-->>AppService: FeatureFlagsPayloadType
else validation fails
ServiceBlobStorage-->>AppService: Error("Feature flag payload validation failed ...")
end
Sequence diagram for React feature flag provider with caching and fallbackssequenceDiagram
actor ReactComponent
participant FeatureFlagProvider
participant LRUCache
participant BrowserFetch as fetch
ReactComponent->>FeatureFlagProvider: useFeatureFlags()
FeatureFlagProvider->>FeatureFlagProvider: useEffect(config)
alt isInStorybookEnv()
FeatureFlagProvider->>FeatureFlagProvider: setFeatureFlags(config.fallbackFlagValues)
FeatureFlagProvider-->>ReactComponent: GetFeatureFlagByName(name)
else [not Storybook]
alt [config.url is empty]
FeatureFlagProvider->>FeatureFlagProvider: setFeatureFlags(config.fallbackFlagValues)
FeatureFlagProvider-->>ReactComponent: GetFeatureFlagByName(name)
else [config.url provided]
FeatureFlagProvider->>LRUCache: get("featureFlags")
alt [cachedFeatureFlags]
LRUCache-->>FeatureFlagProvider: FeatureFlags
FeatureFlagProvider->>FeatureFlagProvider: setFeatureFlags(cachedFeatureFlags)
FeatureFlagProvider-->>ReactComponent: GetFeatureFlagByName(name)
else [no cache]
FeatureFlagProvider->>FeatureFlagProvider: fetchFeatureFlags(config.url)
FeatureFlagProvider->>BrowserFetch: fetch(urlWithTimestamp, { cache: "no-store" })
BrowserFetch-->>FeatureFlagProvider: response
alt [response.ok]
FeatureFlagProvider->>BrowserFetch: response.json()
BrowserFetch-->>FeatureFlagProvider: remote FeatureFlags
FeatureFlagProvider->>LRUCache: set("featureFlags", remote FeatureFlags)
FeatureFlagProvider->>FeatureFlagProvider: setFeatureFlags(remote FeatureFlags)
FeatureFlagProvider-->>ReactComponent: GetFeatureFlagByName(name)
else [fetch/json error or !ok]
FeatureFlagProvider->>FeatureFlagProvider: setFeatureFlags(config.fallbackFlagValues)
FeatureFlagProvider-->>ReactComponent: GetFeatureFlagByName(name)
end
end
end
end
Note over FeatureFlagProvider,LRUCache: setInterval refreshFeatureFlags() runs every cacheMilliseconds / 2
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
Fixed security issues:
-
js-yaml (link)
-
nanoid (link)
-
In
ServiceBlobStorage.getValidatedBlobDataObject, you compile the AJV schema on every call; consider pre-compilingFeatureFlagsSchemaonce (e.g.,const validateFeatureFlags = ajv.compile(FeatureFlagsSchema)) to avoid repeated compilation overhead on eachgetFeatureFlagsinvocation. -
The
FeatureFlagProvideruseEffect depends on the wholeconfigobject, which will retrigger the effect whenever a new object reference is passed; to avoid unnecessary refetches, either narrow the dependency array to specific primitive fields (e.g.,config.url,config.cache) or ensure callers memoize the config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ServiceBlobStorage.getValidatedBlobDataObject`, you compile the AJV schema on every call; consider pre-compiling `FeatureFlagsSchema` once (e.g., `const validateFeatureFlags = ajv.compile(FeatureFlagsSchema)`) to avoid repeated compilation overhead on each `getFeatureFlags` invocation.
- The `FeatureFlagProvider` useEffect depends on the whole `config` object, which will retrigger the effect whenever a new object reference is passed; to avoid unnecessary refetches, either narrow the dependency array to specific primitive fields (e.g., `config.url`, `config.cache`) or ensure callers memoize the config.
## Individual Comments
### Comment 1
<location path="packages/ocom/service-blob-storage/src/service-blob-storage.ts" line_range="9-11" />
<code_context>
+const ajv = new Ajv2020({ allErrors: true });
+
+export class ServiceBlobStorage extends CellixServiceBlobStorage {
+ private getValidatedBlobDataObject<T>(schema: object, dataRaw: string): T {
+ const dataJson: unknown = JSON.parse(dataRaw);
+ const validate = ajv.compile(schema);
+ if (!validate(dataJson)) {
+ throw new Error(`Feature flag payload validation failed: ${ajv.errorsText(validate.errors)}`);
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recompiling the AJV schema on every call to improve performance.
`getValidatedBlobDataObject` currently compiles the JSON schema on every call. Since `FeatureFlagsSchema` is static and this helper is only used for feature flags, compile the schema once (e.g., at module scope) and reuse the validator. If you plan to use this helper with multiple schemas, consider caching compiled validators in a map keyed by schema identity.
Suggested implementation:
```typescript
const ajv = new Ajv2020({ allErrors: true });
const validateFeatureFlags = ajv.compile(FeatureFlagsSchema);
```
```typescript
export class ServiceBlobStorage extends CellixServiceBlobStorage {
private getValidatedBlobDataObject<T>(dataRaw: string): T {
const dataJson: unknown = JSON.parse(dataRaw);
if (!validateFeatureFlags(dataJson)) {
throw new Error(`Feature flag payload validation failed: ${ajv.errorsText(validateFeatureFlags.errors)}`);
}
return dataJson as T;
}
```
```typescript
public async getFeatureFlags(): Promise<FeatureFlagsPayloadType> {
const featureFlagsRaw = (await this.downloadBlobToString('public', FEATURE_FLAG_BLOB_NAME)) ?? FeatureFlagsLocal;
return this.getValidatedBlobDataObject<FeatureFlagsPayloadType>(featureFlagsRaw);
}
```
</issue_to_address>
### Comment 2
<location path="packages/ocom/service-blob-storage/src/feature-flags.ts" line_range="15-24" />
<code_context>
+ readonly FeatureFlags: readonly FeatureFlag[];
+}
+
+export const FeatureFlagsSchema = {
+ $schema: 'https://json-schema.org/draft/2020-12/schema',
+ type: 'object',
+ properties: {
+ FeatureFlags: {
+ type: 'array',
+ items: {
+ type: 'object',
+ properties: {
+ Name: { type: 'string' },
+ Description: { type: 'string' },
+ Value: { type: 'string' },
+ AllowedValues: {
+ type: 'array',
+ items: { type: 'string' },
+ },
+ RetirementDate: { type: 'string' },
+ },
+ additionalProperties: false,
+ },
+ },
+ },
+ additionalProperties: false,
+} as const;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Strengthen the feature flag schema by requiring the FeatureFlags property.
Right now `{}` passes this schema because `FeatureFlags` isn’t required. If the backend always expects a `FeatureFlags` array in the payload, add `required: ['FeatureFlags']` and, if needed, a `minItems` constraint (e.g. `0` or `1`) so invalid payloads are rejected instead of silently accepted.
Suggested implementation:
```typescript
export const FeatureFlagsSchema = {
$schema: 'https://json-schema.org/draft/2020-12/schema',
type: 'object',
required: ['FeatureFlags'],
properties: {
FeatureFlags: {
type: 'array',
minItems: 0,
```
```typescript
RetirementDate: { type: 'string' },
},
additionalProperties: false,
},
},
},
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+24
| export const FeatureFlagsSchema = { | ||
| $schema: 'https://json-schema.org/draft/2020-12/schema', | ||
| type: 'object', | ||
| properties: { | ||
| FeatureFlags: { | ||
| type: 'array', | ||
| items: { | ||
| type: 'object', | ||
| properties: { | ||
| Name: { type: 'string' }, |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Strengthen the feature flag schema by requiring the FeatureFlags property.
Right now {} passes this schema because FeatureFlags isn’t required. If the backend always expects a FeatureFlags array in the payload, add required: ['FeatureFlags'] and, if needed, a minItems constraint (e.g. 0 or 1) so invalid payloads are rejected instead of silently accepted.
Suggested implementation:
export const FeatureFlagsSchema = {
$schema: 'https://json-schema.org/draft/2020-12/schema',
type: 'object',
required: ['FeatureFlags'],
properties: {
FeatureFlags: {
type: 'array',
minItems: 0, RetirementDate: { type: 'string' },
},
additionalProperties: false,
},
},
},…x 404 error from previous version download url
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce backend and frontend feature flag support wired to blob storage and shared UI provider, with validation, caching, and fallbacks for missing or invalid data.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: