[App Configuration] Make feature flag conditions property optional #36761
+114
−14
Conversation
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
linglingye001
requested review from
RichardChen820,
jimmyca15,
minhanh-phan and
zhiyuanliang-ms
as code owners
github-actions
bot
added
the
App Configuration
Copilot finished reviewing on behalf of
linglingye001
November 28, 2025 03:10
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the enabled and conditions properties optional in the FeatureFlagValue interface to align with the Feature Flag v2.0.0 schema. The changes enable support for minimal feature flags that only specify an id field.
Key changes:
- Updated type definitions to make
enabledandconditionsoptional in bothFeatureFlagValueandJsonFeatureFlagValue - Modified
parseFeatureFlagto conditionally set theconditionsproperty only when present in the JSON - Added comprehensive test coverage for parsing feature flags with various combinations of optional fields
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/appconfiguration/app-configuration/src/internal/jsonModels.ts | Made enabled and conditions properties optional in the internal JSON model type |
| sdk/appconfiguration/app-configuration/src/featureFlag.ts | Updated FeatureFlagValue interface and modified serialization/deserialization logic to handle optional fields |
| sdk/appconfiguration/app-configuration/test/public/featureFlagOptionalFields.spec.ts | Added new test file with comprehensive coverage for parsing feature flags with optional fields |
| sdk/appconfiguration/app-configuration/review/app-configuration-node.api.md | Updated API review file to reflect the optional properties in the public interface |
Comment on lines
9
to
94
| describe("FeatureFlag - Optional Fields", () => { | ||
| it("should parse feature flag with only id field", () => { | ||
| const minimalFeatureFlag = { | ||
| key: ".appconfig.featureflag/minimal-flag", | ||
| value: JSON.stringify({ id: "minimal-flag" }), | ||
| contentType: featureFlagContentType, | ||
| } as ConfigurationSetting; | ||
|
|
||
| const parsed = parseFeatureFlag(minimalFeatureFlag); | ||
| assert.equal(parsed.value.id, "minimal-flag"); | ||
| assert.isUndefined(parsed.value.enabled); | ||
| assert.isUndefined(parsed.value.conditions); | ||
| assert.isUndefined(parsed.value.description); | ||
| assert.isUndefined(parsed.value.displayName); | ||
| }); | ||
|
|
||
| it("should parse feature flag without conditions", () => { | ||
| const featureFlagWithoutConditions = { | ||
| key: ".appconfig.featureflag/no-conditions", | ||
| value: JSON.stringify({ | ||
| id: "no-conditions", | ||
| enabled: true, | ||
| description: "A feature flag without conditions", | ||
| display_name: "No Conditions Flag", | ||
| }), | ||
| contentType: featureFlagContentType, | ||
| } as ConfigurationSetting; | ||
|
|
||
| const parsed = parseFeatureFlag(featureFlagWithoutConditions); | ||
| assert.equal(parsed.value.id, "no-conditions"); | ||
| assert.equal(parsed.value.enabled, true); | ||
| assert.equal(parsed.value.description, "A feature flag without conditions"); | ||
| assert.equal(parsed.value.displayName, "No Conditions Flag"); | ||
| assert.isUndefined(parsed.value.conditions); | ||
| }); | ||
|
|
||
| it("should parse feature flag with empty conditions", () => { | ||
| const featureFlagWithEmptyConditions = { | ||
| key: ".appconfig.featureflag/empty-conditions", | ||
| value: JSON.stringify({ | ||
| id: "empty-conditions", | ||
| enabled: false, | ||
| conditions: { | ||
| client_filters: [], | ||
| }, | ||
| }), | ||
| contentType: featureFlagContentType, | ||
| } as ConfigurationSetting; | ||
|
|
||
| const parsed = parseFeatureFlag(featureFlagWithEmptyConditions); | ||
| assert.equal(parsed.value.id, "empty-conditions"); | ||
| assert.equal(parsed.value.enabled, false); | ||
| assert.isDefined(parsed.value.conditions); | ||
| assert.deepEqual(parsed.value.conditions?.clientFilters, []); | ||
| assert.isUndefined(parsed.value.conditions?.requirementType); | ||
| }); | ||
|
|
||
| it("should parse feature flag with conditions and requirement_type", () => { | ||
| const fullFeatureFlag = { | ||
| key: ".appconfig.featureflag/full-flag", | ||
| value: JSON.stringify({ | ||
| id: "full-flag", | ||
| enabled: true, | ||
| description: "Full feature flag", | ||
| display_name: "Full Flag", | ||
| conditions: { | ||
| client_filters: [ | ||
| { name: "Filter1", parameters: { key: "value" } }, | ||
| { name: "Filter2" }, | ||
| ], | ||
| requirement_type: "All", | ||
| }, | ||
| }), | ||
| contentType: featureFlagContentType, | ||
| } as ConfigurationSetting; | ||
|
|
||
| const parsed = parseFeatureFlag(fullFeatureFlag); | ||
| assert.equal(parsed.value.id, "full-flag"); | ||
| assert.equal(parsed.value.enabled, true); | ||
| assert.equal(parsed.value.description, "Full feature flag"); | ||
| assert.equal(parsed.value.displayName, "Full Flag"); | ||
| assert.isDefined(parsed.value.conditions); | ||
| assert.equal(parsed.value.conditions?.clientFilters.length, 2); | ||
| assert.equal(parsed.value.conditions?.requirementType, "All"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests only cover deserialization (parsing) of feature flags with optional fields. Consider adding round-trip tests that serialize and then deserialize feature flags with undefined enabled and conditions fields. Since FeatureFlagHelper is internal, you can test the serialization path using addConfigurationSetting followed by getConfigurationSetting. For example:
it("should handle round-trip for feature flag with undefined enabled field", async () => {
const featureFlag = {
key: `${featureFlagPrefix}minimal-flag`,
value: { id: "minimal-flag" },
contentType: featureFlagContentType,
} as ConfigurationSetting<FeatureFlagValue>;
await client.addConfigurationSetting(featureFlag);
const retrieved = await client.getConfigurationSetting({ key: featureFlag.key });
const parsed = parseFeatureFlag(retrieved);
// Verify the enabled field behavior after round-trip
assert.isUndefined(parsed.value.enabled); // or assert.equal(parsed.value.enabled, false) depending on intended behavior
});This would help ensure consistency between serialization and deserialization for optional fields.
Member
|
@linglingye001 There were format issues in the PR. |
zhiyuanliang-ms
approved these changes
Dec 3, 2025
Member
|
Discussed offline. I suggest to skip making conditions and enabled optional as they have already been released as required. We should just ensure to set them to some reasonable default if a deserialized payload does not have them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Making
enabledandconditionsoptional according to the Feature Flag schema