Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AppConfig-BugFix] Preserve unknown properties in FeatureFlag and SecretReference ConfigurationSettings #36725

Merged
merged 12 commits into from
Sep 18, 2023

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Sep 12, 2023

FeatureFlagConfigurationSetting and SecretReferenceConfigurationSetting will now retain custom attributes in the setting value. Previously, only attributes that were defined in the associated JSON schema were allowed and unknown attributes were discarded.

fixes: #34904

  • FeatureFlagConfigurationSetting can take unknown properties other than the swagger-defined property definition. Such as
featureFlag.setValue("{\"conditions\":{\"requirement_type\":\"All\"}}"); // "requirement_type" is user-defined property

Users can edit the ConfigurationSetting's value directly or through Azure Portal UI.

  • Also, added similar consideration to SecretReferenceConfigurationSetting.

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. App Configuration Azure.ApplicationModel.Configuration labels Sep 12, 2023
@mssfang mssfang added this to the 2023-09 milestone Sep 12, 2023
@mssfang mssfang self-assigned this Sep 12, 2023
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-data-appconfiguration

@mssfang mssfang marked this pull request as ready for review September 13, 2023 06:43
@alzimmermsft
Copy link
Member

alzimmermsft commented Sep 13, 2023

Just trying to understand this change a bit more before giving a detailed review. What we're trying to support is FeatureFlagFilters that don't conform to the standard {"name":"name","parameters":{...}}? Or is this additional data additional JSON properties in the Feature Flag Configuration setting, {<regular Feature Flag JSON>,"additionalProperty":"additionalValue"}?

@mrm9084
Copy link
Member

mrm9084 commented Sep 13, 2023

Just trying to understand this change a bit more before giving a detailed review. What we're trying to support is FeatureFlagFilters that don't conform to the standard {"name":"name","parameters":{...}}?

The service supports any custom values that the customer wants, or any additional fields the service team wants.

For example we are trying to add a requirement_type field that allows for switching between or/and logic for feature flags. Currently if any modifications are made to the feature flag object any unknown fields are erased which shouldn't happen.

This issue is present in most of the SDKs.

@mssfang mssfang changed the title [AppConfig-BugFix] Fixed bug that FeatureFlagConfigurationSetting overrides all of value when updating property [AppConfig-BugFix] Preserve unknown properties in FeatureFlag and SecretReference ConfigurationSettings Sep 13, 2023
@alzimmermsft
Copy link
Member

alzimmermsft commented Sep 14, 2023

Just trying to understand this change a bit more before giving a detailed review. What we're trying to support is FeatureFlagFilters that don't conform to the standard {"name":"name","parameters":{...}}?

The service supports any custom values that the customer wants, or any additional fields the service team wants.

For example we are trying to add a requirement_type field that allows for switching between or/and logic for feature flags. Currently if any modifications are made to the feature flag object any unknown fields are erased which shouldn't happen.

This issue is present in most of the SDKs.

Okay, given this, I think we should add support for "additional properties" throughout the setting. This is fairly standard functionality where a Map<String, Object> is added to a model and handled fluently in serialization and deserialization scenarios.

- JavaDoc for JDK 20
- Spring Cloud AppConfig test fix
@mssfang
Copy link
Member Author

mssfang commented Sep 14, 2023

Just trying to understand this change a bit more before giving a detailed review. What we're trying to support is FeatureFlagFilters that don't conform to the standard {"name":"name","parameters":{...}}?

The service supports any custom values that the customer wants, or any additional fields the service team wants.
For example we are trying to add a requirement_type field that allows for switching between or/and logic for feature flags. Currently if any modifications are made to the feature flag object any unknown fields are erased which shouldn't happen.
This issue is present in most of the SDKs.

Okay, given this, I think we should add support for "additional properties" throughout the setting. This is fairly standard functionality where a Map<String, Object> is added to a model and handled fluently in serialization and deserialization scenarios.

Offline discussion with Alan. Adding public API to support "unknown properties" which isn't in the swagger is not a good approach. Will keep no change in the SDK public surface. If supporting "unknown properties" in the public API, it should go through the swagger API archiboard and discussion.

Copy link
Member

@mrm9084 mrm9084 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, I would like to test it's change before the release instead of my current work around.

@mssfang mssfang merged commit 395953a into Azure:main Sep 18, 2023
31 checks passed
@mssfang mssfang deleted the AppConfig-breakChange branch September 18, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] FeatureFlagConfigurationSetting overrides all of value when updating property
4 participants