[App Configuration] - Snapshot reference support #55211
Conversation
linglingye001
requested review from
avanigupta and
zhiyuanliang-ms
as code owners
github-actions
bot
added
the
App Configuration
There was a problem hiding this comment.
Pull request overview
This pull request adds snapshot reference support to the Azure App Configuration SDK, introducing a new SnapshotReferenceConfigurationSetting class that enables configuration settings to reference snapshots. This follows the established pattern of the existing SecretReferenceConfigurationSetting class.
Changes:
- Added
SnapshotReferenceConfigurationSettingclass for representing configuration settings that reference snapshots - Integrated snapshot reference detection into the deserialization logic
- Added comprehensive unit and integration tests following established patterns
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/appconfiguration/Azure.Data.AppConfiguration/src/Models/SnapshotReferenceConfigurationSetting.cs | New class implementing snapshot reference configuration settings with JSON serialization/deserialization |
| sdk/appconfiguration/Azure.Data.AppConfiguration/tests/SnapshotReferenceConfigurationSettingTests.cs | Comprehensive unit tests covering creation, serialization, edge cases, and error scenarios |
| sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs | Integration tests for adding, retrieving, updating, and filtering snapshot references |
| sdk/appconfiguration/Azure.Data.AppConfiguration/src/Models/ConfigurationServiceSerializer.cs | Added snapshot reference detection during deserialization based on content type |
| sdk/appconfiguration/Azure.Data.AppConfiguration/src/Models/ConfigurationModelFactory.cs | Added factory method for creating snapshot reference settings for mocking purposes |
| sdk/appconfiguration/Azure.Data.AppConfiguration/assets.json | Updated test asset tag for new test recordings |
| if (document.RootElement.TryGetProperty("snapshot_name", out JsonElement snapshotNameElement) && | ||
| snapshotNameElement.ValueKind == JsonValueKind.String) | ||
| { | ||
| _snapshotName = snapshotNameElement.GetString(); | ||
| return true; |
There was a problem hiding this comment.
There is an inconsistency in null snapshot name handling. The constructor and property setter allow null snapshot names (tests at lines 30-31 and 42-43 verify this). When GetValue() serializes a null snapshot name, it produces {"snapshot_name": null}. However, TryParseValue() rejects this JSON because line 103 checks for ValueKind == String, which excludes Null. This creates a roundtrip failure: construct with null → get Value → set Value back → now invalid. Consider either: (1) handling JsonValueKind.Null in TryParseValue() to accept null values, (2) preventing null in the constructor/setter, or (3) omitting the snapshot_name property from JSON when null (similar to how other optional properties are handled).
| if (document.RootElement.TryGetProperty("snapshot_name", out JsonElement snapshotNameElement) && | |
| snapshotNameElement.ValueKind == JsonValueKind.String) | |
| { | |
| _snapshotName = snapshotNameElement.GetString(); | |
| return true; | |
| if (document.RootElement.TryGetProperty("snapshot_name", out JsonElement snapshotNameElement)) | |
| { | |
| if (snapshotNameElement.ValueKind == JsonValueKind.String) | |
| { | |
| _snapshotName = snapshotNameElement.GetString(); | |
| return true; | |
| } | |
| if (snapshotNameElement.ValueKind == JsonValueKind.Null) | |
| { | |
| _snapshotName = null; | |
| return true; | |
| } | |
| return false; |
| internal override string GetValue() | ||
| { | ||
| // If the value wasn't valid, return it verbatim. | ||
| if (!_isValidValue) | ||
| { | ||
| return _originalValue; | ||
| } | ||
|
|
||
| using var memoryStream = new MemoryStream(); | ||
| using var writer = new Utf8JsonWriter(memoryStream); | ||
|
|
||
| writer.WriteStartObject(); | ||
| writer.WriteString("snapshot_name", _snapshotName); | ||
| writer.WriteEndObject(); | ||
| writer.Flush(); | ||
|
|
||
| _originalValue = Encoding.UTF8.GetString(memoryStream.ToArray()); | ||
| return _originalValue; | ||
| } |
There was a problem hiding this comment.
Unlike SecretReferenceConfigurationSetting, this implementation does not preserve unknown JSON properties when round-tripping values. SecretReferenceConfigurationSetting stores and re-serializes any additional properties beyond "uri", allowing for forward compatibility with future API changes. If the snapshot reference specification allows for additional properties, consider implementing the same preservation logic for consistency and forward compatibility. If additional properties are explicitly not allowed by the specification, this simplified implementation is correct.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
linglingye001
force-pushed
the
linglingye/snapshot-ref
branch
from
bc68615 to
3c678ea
Compare
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.