Skip to content

Commit

Permalink
Fix de/serialization bugs in FeatureFlagConfigurationSetting (Azure#3…
Browse files Browse the repository at this point in the history
  • Loading branch information
YalinLi0312 authored Mar 22, 2024
1 parent b564177 commit 4b0a56d
Show file tree
Hide file tree
Showing 12 changed files with 149 additions and 81 deletions.
6 changes: 6 additions & 0 deletions .vscode/cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,12 @@
"kvset"
]
},
{
"filename": "sdk/appconfiguration/azure-appconfiguration/tests/**",
"words": [
"adfsasdfzsd"
]
},
{
"filename": "sdk/personalizer/test-resources.json",
"words": [
Expand Down
10 changes: 3 additions & 7 deletions sdk/appconfiguration/azure-appconfiguration/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
# Release History

## 1.6.0b2 (Unreleased)

### Features Added

### Breaking Changes
## 1.6.0b2 (2024-03-21)

### Bugs Fixed

### Other Changes
- Changed invalid default value `None` to `False` for property `enabled` in `FeatureFlagConfigurationSetting`.
- Fixed the issue that `description`, `display_name` and other customer fields are missing when de/serializing `FeatureFlagConfigurationSetting` objects.

## 1.6.0b1 (2024-03-14)

Expand Down
2 changes: 1 addition & 1 deletion sdk/appconfiguration/azure-appconfiguration/assets.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"AssetsRepo": "Azure/azure-sdk-assets",
"AssetsRepoPrefixPath": "python",
"TagPrefix": "python/appconfiguration/azure-appconfiguration",
"Tag": "python/appconfiguration/azure-appconfiguration_8137b21bd0"
"Tag": "python/appconfiguration/azure-appconfiguration_27c8f82a12"
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import binascii
import functools
from datetime import datetime
from typing import Any, Dict, List, Mapping, Optional, Union, cast, overload
from typing import Any, Dict, List, Optional, Union, cast, overload
from typing_extensions import Literal
from azure.core import MatchConditions
from azure.core.paging import ItemPaged
Expand All @@ -21,7 +21,6 @@
ResourceNotModifiedError,
)
from azure.core.rest import HttpRequest, HttpResponse
from azure.core.utils import CaseInsensitiveDict
from ._azure_appconfiguration_error import ResourceReadOnlyError
from ._azure_appconfiguration_requests import AppConfigRequestsCredentialsPolicy
from ._generated import AzureAppConfiguration
Expand Down Expand Up @@ -321,16 +320,15 @@ def add_configuration_setting(self, configuration_setting: ConfigurationSetting,
added_config_setting = client.add_configuration_setting(config_setting)
"""
key_value = configuration_setting._to_generated()
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map = {412: ResourceExistsError}
try:
key_value_added = self._impl.put_key_value(
entity=key_value,
key=key_value.key, # type: ignore
label=key_value.label,
if_none_match="*",
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_added)
except binascii.Error as exc:
Expand Down Expand Up @@ -358,9 +356,9 @@ def set_configuration_setting(
Will use the value from param configuration_setting if not set.
:return: The ConfigurationSetting returned from the service
:rtype: ~azure.appconfiguration.ConfigurationSetting
:raises: :class:`~azure.core.exceptions.HttpResponseError`, \
:raises: :class:`~azure.appconfiguration.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.HttpResponseError`, \
:class:`~azure.core.exceptions.ClientAuthenticationError`, \
:class:`~azure.core.exceptions.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.ResourceModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotFoundError`, \
Expand All @@ -380,7 +378,6 @@ def set_configuration_setting(
returned_config_setting = client.set_configuration_setting(config_setting)
"""
key_value = configuration_setting._to_generated()
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map: Dict[int, Any] = {409: ResourceReadOnlyError}
if match_condition == MatchConditions.IfNotModified:
error_map.update({412: ResourceModifiedError})
Expand All @@ -398,8 +395,8 @@ def set_configuration_setting(
label=key_value.label,
if_match=prep_if_match(configuration_setting.etag, match_condition),
if_none_match=prep_if_none_match(etag or configuration_setting.etag, match_condition),
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_set)
except binascii.Error as exc:
Expand All @@ -414,7 +411,7 @@ def delete_configuration_setting( # pylint:disable=delete-operation-wrong-retur
etag: Optional[str] = None,
match_condition: MatchConditions = MatchConditions.Unconditionally,
**kwargs,
) -> ConfigurationSetting:
) -> Union[None, ConfigurationSetting]:
"""Delete a ConfigurationSetting if it exists
:param key: key used to identify the ConfigurationSetting
Expand All @@ -426,9 +423,9 @@ def delete_configuration_setting( # pylint:disable=delete-operation-wrong-retur
:paramtype match_condition: ~azure.core.MatchConditions
:return: The deleted ConfigurationSetting returned from the service, or None if it doesn't exist.
:rtype: ~azure.appconfiguration.ConfigurationSetting
:raises: :class:`~azure.core.exceptions.HttpResponseError`, \
:raises: :class:`~azure.appconfiguration.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.HttpResponseError`, \
:class:`~azure.core.exceptions.ClientAuthenticationError`, \
:class:`~azure.core.exceptions.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.ResourceModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotFoundError`, \
Expand All @@ -442,7 +439,6 @@ def delete_configuration_setting( # pylint:disable=delete-operation-wrong-retur
key="MyKey", label="MyLabel"
)
"""
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map: Dict[int, Any] = {409: ResourceReadOnlyError}
if match_condition == MatchConditions.IfNotModified:
error_map.update({412: ResourceModifiedError})
Expand All @@ -458,10 +454,12 @@ def delete_configuration_setting( # pylint:disable=delete-operation-wrong-retur
key=key,
label=label,
if_match=prep_if_match(etag, match_condition),
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_deleted) # type: ignore
if key_value_deleted:
return ConfigurationSetting._from_generated(key_value_deleted)
return None
except binascii.Error as exc:
raise binascii.Error("Connection string secret has incorrect padding") from exc

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@
SnapshotStatus,
)

PolymorphicConfigurationSetting = Union[
"ConfigurationSetting", "SecretReferenceConfigurationSetting", "FeatureFlagConfigurationSetting"
]


class ConfigurationSetting(Model):
"""A setting, defined by a unique combination of a key and label."""
Expand Down Expand Up @@ -70,25 +66,23 @@ def __init__(self, **kwargs: Any) -> None:
self.tags = kwargs.get("tags", {})

@classmethod
def _from_generated(cls, key_value: KeyValue) -> PolymorphicConfigurationSetting:
if key_value is None:
return key_value
def _from_generated(cls, key_value: KeyValue) -> "ConfigurationSetting":
# pylint:disable=protected-access
if key_value.content_type is not None:
try:
if key_value.content_type.startswith(
FeatureFlagConfigurationSetting._feature_flag_content_type # pylint:disable=protected-access
FeatureFlagConfigurationSetting._feature_flag_content_type
) and key_value.key.startswith( # type: ignore
FeatureFlagConfigurationSetting._key_prefix # pylint: disable=protected-access
FeatureFlagConfigurationSetting._key_prefix
):
return FeatureFlagConfigurationSetting._from_generated( # pylint: disable=protected-access
key_value
)
config_setting = FeatureFlagConfigurationSetting._from_generated(key_value)
if key_value.value:
config_setting.value = key_value.value
return config_setting
if key_value.content_type.startswith(
SecretReferenceConfigurationSetting._secret_reference_content_type # pylint:disable=protected-access
SecretReferenceConfigurationSetting._secret_reference_content_type
):
return SecretReferenceConfigurationSetting._from_generated( # pylint: disable=protected-access
key_value
)
return SecretReferenceConfigurationSetting._from_generated(key_value)
except (KeyError, AttributeError):
pass

Expand Down Expand Up @@ -125,7 +119,7 @@ class FeatureFlagConfigurationSetting(ConfigurationSetting): # pylint: disable=
"""The identity of the configuration setting."""
key: str
"""The key of the configuration setting."""
enabled: Optional[bool]
enabled: bool
"""The value indicating whether the feature flag is enabled. A feature is OFF if enabled is false.
If enabled is true, then the feature is ON if there are no conditions or if all conditions are satisfied."""
filters: Optional[List[Dict[str, Any]]]
Expand Down Expand Up @@ -164,7 +158,7 @@ def __init__( # pylint: disable=super-init-not-called
self,
feature_id: str,
*,
enabled: Optional[bool] = None,
enabled: bool = False,
filters: Optional[List[Dict[str, Any]]] = None,
**kwargs: Any,
) -> None:
Expand All @@ -173,8 +167,8 @@ def __init__( # pylint: disable=super-init-not-called
:type feature_id: str
:keyword enabled: The value indicating whether the feature flag is enabled.
A feature is OFF if enabled is false. If enabled is true, then the feature is ON
if there are no conditions or if all conditions are satisfied.
:paramtype enabled: bool or None
if there are no conditions or if all conditions are satisfied. Default value of this property is False.
:paramtype enabled: bool
:keyword filters: Filters that must run on the client and be evaluated as true for the feature
to be considered enabled.
:paramtype filters: list[dict[str, Any]] or None
Expand Down Expand Up @@ -207,6 +201,8 @@ def value(self) -> str:
temp = json.loads(self._value)
temp["id"] = self.feature_id
temp["enabled"] = self.enabled
temp["display_name"] = self.display_name
temp["description"] = self.description
if "conditions" not in temp.keys():
temp["conditions"] = {}
temp["conditions"]["client_filters"] = self.filters
Expand All @@ -221,26 +217,30 @@ def value(self, new_value: str) -> None:
temp = json.loads(new_value)
temp["id"] = self.feature_id
self._value = json.dumps(temp)
self.enabled = temp.get("enabled", None)
self.enabled = temp.get("enabled", False)
self.display_name = temp.get("display_name", None)
self.description = temp.get("description", None)
self.filters = None
conditions = temp.get("conditions", None)
if conditions:
self.filters = conditions.get("client_filters", None)
except (json.JSONDecodeError, ValueError):
self._value = new_value
self.enabled = None
self.enabled = False
self.filters = None

@classmethod
def _from_generated(cls, key_value: KeyValue) -> Union["FeatureFlagConfigurationSetting", ConfigurationSetting]:
if key_value is None:
return key_value
enabled = None
def _from_generated(cls, key_value: KeyValue) -> "FeatureFlagConfigurationSetting":
enabled = False
filters = None
display_name = None
description = None
try:
temp = json.loads(key_value.value) # type: ignore
if isinstance(temp, dict):
enabled = temp.get("enabled")
enabled = temp.get("enabled", False)
display_name = temp.get("display_name")
description = temp.get("description")
if "conditions" in temp.keys():
filters = temp["conditions"].get("client_filters")
except (ValueError, json.JSONDecodeError):
Expand All @@ -256,6 +256,8 @@ def _from_generated(cls, key_value: KeyValue) -> Union["FeatureFlagConfiguration
etag=key_value.etag,
enabled=enabled,
filters=filters,
display_name=display_name,
description=description,
)

def _to_generated(self) -> KeyValue:
Expand Down Expand Up @@ -349,8 +351,6 @@ def value(self, new_value: str) -> None:

@classmethod
def _from_generated(cls, key_value: KeyValue) -> "SecretReferenceConfigurationSetting":
if key_value is None:
return key_value
secret_uri = None
try:
temp = json.loads(key_value.value) # type: ignore
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import binascii
import functools
from datetime import datetime
from typing import Any, Dict, List, Mapping, Optional, Union, cast, overload
from typing import Any, Dict, List, Optional, Union, cast, overload
from typing_extensions import Literal
from azure.core import MatchConditions
from azure.core.async_paging import AsyncItemPaged
Expand All @@ -23,7 +23,6 @@
ResourceNotModifiedError,
)
from azure.core.rest import AsyncHttpResponse, HttpRequest
from azure.core.utils import CaseInsensitiveDict
from ._sync_token_async import AsyncSyncTokenPolicy
from .._azure_appconfiguration_error import ResourceReadOnlyError
from .._azure_appconfiguration_requests import AppConfigRequestsCredentialsPolicy
Expand Down Expand Up @@ -334,7 +333,6 @@ async def add_configuration_setting(
added_config_setting = await async_client.add_configuration_setting(config_setting)
"""
key_value = configuration_setting._to_generated()
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map = {412: ResourceExistsError}

try:
Expand All @@ -343,8 +341,8 @@ async def add_configuration_setting(
key=key_value.key, # type: ignore
label=key_value.label,
if_none_match="*",
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_added)
except binascii.Error as exc:
Expand Down Expand Up @@ -373,9 +371,9 @@ async def set_configuration_setting(
Will use the value from param configuration_setting if not set.
:return: The ConfigurationSetting returned from the service
:rtype: ~azure.appconfiguration.ConfigurationSetting
:raises: :class:`~azure.core.exceptions.HttpResponseError`, \
:raises: :class:`~azure.appconfiguration.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.HttpResponseError`, \
:class:`~azure.core.exceptions.ClientAuthenticationError`, \
:class:`~azure.core.exceptions.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.ResourceModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotFoundError`, \
Expand All @@ -396,7 +394,6 @@ async def set_configuration_setting(
returned_config_setting = await async_client.set_configuration_setting(config_setting)
"""
key_value = configuration_setting._to_generated()
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map: Dict[int, Any] = {409: ResourceReadOnlyError}
if match_condition == MatchConditions.IfNotModified:
error_map.update({412: ResourceModifiedError})
Expand All @@ -414,8 +411,8 @@ async def set_configuration_setting(
label=key_value.label,
if_match=prep_if_match(configuration_setting.etag, match_condition),
if_none_match=prep_if_none_match(etag or configuration_setting.etag, match_condition),
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_set)
except binascii.Error as exc:
Expand All @@ -430,7 +427,7 @@ async def delete_configuration_setting(
etag: Optional[str] = None,
match_condition: MatchConditions = MatchConditions.Unconditionally,
**kwargs,
) -> ConfigurationSetting:
) -> Union[None, ConfigurationSetting]:
"""Delete a ConfigurationSetting if it exists
:param key: key used to identify the ConfigurationSetting
Expand All @@ -442,9 +439,9 @@ async def delete_configuration_setting(
:paramtype match_condition: ~azure.core.MatchConditions
:return: The deleted ConfigurationSetting returned from the service, or None if it doesn't exist.
:rtype: ~azure.appconfiguration.ConfigurationSetting
:raises: :class:`~azure.core.exceptions.HttpResponseError`, \
:raises: :class:`~azure.appconfiguration.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.HttpResponseError`, \
:class:`~azure.core.exceptions.ClientAuthenticationError`, \
:class:`~azure.core.exceptions.ResourceReadOnlyError`, \
:class:`~azure.core.exceptions.ResourceModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotModifiedError`, \
:class:`~azure.core.exceptions.ResourceNotFoundError`, \
Expand All @@ -459,7 +456,6 @@ async def delete_configuration_setting(
key="MyKey", label="MyLabel"
)
"""
custom_headers: Mapping[str, Any] = CaseInsensitiveDict(kwargs.get("headers"))
error_map: Dict[int, Any] = {409: ResourceReadOnlyError}
if match_condition == MatchConditions.IfNotModified:
error_map.update({412: ResourceModifiedError})
Expand All @@ -475,10 +471,12 @@ async def delete_configuration_setting(
key=key,
label=label,
if_match=prep_if_match(etag, match_condition),
headers=custom_headers,
error_map=error_map,
**kwargs,
)
return ConfigurationSetting._from_generated(key_value_deleted) # type: ignore
if key_value_deleted:
return ConfigurationSetting._from_generated(key_value_deleted)
return None
except binascii.Error as exc:
raise binascii.Error("Connection string secret has incorrect padding") from exc

Expand Down
Loading

0 comments on commit 4b0a56d

Please sign in to comment.