Remove content type check for appconfig #235
Conversation
| } | ||
|
|
||
| private static IDictionary<string, string> ParseConfig(string contentType, Stream configuration) | ||
| public static IDictionary<string, string> ParseConfig(string contentType, Stream configuration) |
There was a problem hiding this comment.
made public for easier unit testing
There was a problem hiding this comment.
We should avoid making things public just for testing purposes because that adds the method to our public API contract. You can enable the InternalVisibleTo assembly attribute if you want and make the method internal.
There was a problem hiding this comment.
fixed in last revision. i had to also enable signing on the test project and include its public key as reference to the regular project https://stackoverflow.com/questions/30943342/how-to-use-internalsvisibleto-attribute-with-strongly-named-assembly
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| private static IDictionary<string, string> ParseConfig(string contentType, Stream configuration) | ||
| public static IDictionary<string, string> ParseConfig(string contentType, Stream configuration) |
There was a problem hiding this comment.
We should avoid making things public just for testing purposes because that adds the method to our public API contract. You can enable the InternalVisibleTo assembly attribute if you want and make the method internal.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the content type validation check for AWS AppConfig, allowing it to parse JSON content regardless of the reported Content-Type header. This fixes an issue where AppConfig backed by Parameter Store returns application/octet-stream instead of application/json, causing the configuration to fail loading.
- Replaces content type validation with direct JSON parsing attempt
- Adds comprehensive test coverage for various content types and error scenarios
- Includes integration test for the specific
application/octet-streamuse case
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Amazon.Extensions.Configuration.SystemsManager/AppConfig/AppConfigProcessor.cs | Removes content type switch statement and always attempts JSON parsing with better error handling |
| test/Amazon.Extensions.Configuration.SystemsManager.Tests/AppConfigProcessorTests.cs | Adds comprehensive unit tests for the new parsing logic across different content types |
| test/Amazon.Extensions.Configuration.SystemsManager.Integ/AppConfigEndToEndTests.cs | Adds integration test for octet-stream content type scenario |
| src/Amazon.Extensions.Configuration.SystemsManager/Amazon.Extensions.Configuration.SystemsManager.csproj | Adds InternalsVisibleTo attribute to allow test access to internal methods |
| test/Amazon.Extensions.Configuration.SystemsManager.Tests/Amazon.Extensions.Configuration.SystemsManager.Tests.csproj | Adds assembly signing configuration |
| .autover/changes/976aa8d4-d143-4f7b-9bf5-762b166d7fb5.json | Documents the change for automatic versioning |
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Net.Http; | ||
| using System.Text.Json; |
There was a problem hiding this comment.
[nitpick] The System.Text.Json using statement is added but JsonException is the only type being used from this namespace. Consider if this import is necessary or if the existing JsonConfigurationParser.Parse method already handles JSON exceptions appropriately.
| using System.Text.Json; |
Copilot uses AI. Check for mistakes.
Description
Right now if users configure app config with parameter store as the configuration source behind the scenes it fails because we are checking the content type to be application/json. But in this use case the user may have used free form json in parameter store but the content type is application/octet-stream when being sent back from app config. I dont see any reason why we needed this check and we can simply try to parse as json and it fails then throw error.
Motivation and Context
#117 (reply in thread))
Testing
configBuilder.AddAppConfighttps://github.com/aws/aws-dotnet-extensions-configuration/tree/gcbeatty/contentsample/samples/AppConfigParameterStoreTypes of changes
Checklist
License