[release/13.2] Fix config boolean handling: crash on read, wrong types on write#15383
[release/13.2] Fix config boolean handling: crash on read, wrong types on write#15383joperezr merged 1 commit intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15383Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15383" |
There was a problem hiding this comment.
Pull request overview
Fixes a crash when reading boolean feature flags from aspire.config.json by (a) making AspireConfigFile.Features tolerant of legacy "true"/"false" string values on read, and (b) preserving JSON value types when normalizing colon-separated keys, plus updating config-writing behavior.
Changes:
- Add
FlexibleBooleanDictionaryConvertertoAspireConfigFile.Featuresto support both JSON booleans and legacy string booleans. - Update settings normalization to preserve JSON types (e.g., booleans) when converting colon-separated root keys into nested objects.
- Update
ConfigurationServiceto write booleans (and integers) as JSON primitives instead of always writing strings; add tests covering these scenarios.
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 |
|---|---|
| tests/Aspire.Cli.Tests/Configuration/ConfigurationServiceTests.cs | Adds tests verifying SetConfigurationAsync writes booleans/integers as JSON primitives and round-trips via AspireConfigFile.Load(). |
| tests/Aspire.Cli.Tests/Configuration/ConfigurationHelperTests.cs | Adds a test ensuring normalization preserves boolean JSON types and remains loadable. |
| tests/Aspire.Cli.Tests/Configuration/AspireConfigFileTests.cs | Adds tests for loading features when values are booleans or legacy string booleans, and for save/load round-tripping. |
| src/Aspire.Cli/Utils/ConfigurationHelper.cs | Preserves JSON node types during normalization via JsonNode.DeepClone() instead of ToString(). |
| src/Aspire.Cli/Configuration/ConfigurationService.cs | Writes typed JSON values via a new conversion helper instead of always writing strings. |
| src/Aspire.Cli/Configuration/AspireConfigFile.cs | Applies FlexibleBooleanDictionaryConverter to Features for backward-compatible parsing. |
You can also share your feedback on Copilot code review. Take the survey.
| /// Converts a string value to its natural JSON type when possible. | ||
| /// Boolean strings ("true"/"false") become JSON booleans, | ||
| /// integer strings become JSON numbers, and everything else stays as a string. | ||
| /// </summary> | ||
| private static JsonNode? ConvertToTypedJsonValue(string value) | ||
| { | ||
| if (bool.TryParse(value, out var boolValue)) | ||
| { | ||
| return JsonValue.Create(boolValue); | ||
| } | ||
|
|
||
| if (long.TryParse(value, System.Globalization.NumberStyles.Integer, System.Globalization.CultureInfo.InvariantCulture, out var longValue)) | ||
| { | ||
| return JsonValue.Create(longValue); | ||
| } | ||
|
|
||
| return value; |
mitchdenny
changed the title
dotnet-policy-service
bot
added
the
needs-author-action
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
The core fixes are correct — FlexibleBooleanDictionaryConverter on Features, DeepClone() in TryNormalizeSettingsFile, and writing typed JSON values all address the root causes well, and the test coverage is solid. Two concerns flagged inline.
| { | ||
| // Convert dot notation to colon notation for IConfiguration access | ||
| var configKey = key.Replace('.', ':'); | ||
| return Task.FromResult(configuration[configKey]); | ||
| var value = configuration[configKey]; | ||
|
|
||
| // IConfiguration converts JSON booleans true/false to .NET strings "True"/"False". | ||
| // Normalize to lowercase so CLI output matches standard JSON conventions and is | ||
| // consistent with what aspire config set writes. | ||
| if (string.Equals(value, "True", StringComparison.Ordinal)) | ||
| { | ||
| value = "true"; | ||
| } | ||
| else if (string.Equals(value, "False", StringComparison.Ordinal)) | ||
| { | ||
| value = "false"; | ||
| } | ||
|
|
||
| return Task.FromResult(value); |
There was a problem hiding this comment.
The normalization here for GetConfigurationAsync (config get) is good. Note that FlattenJsonObject (used by GetAllConfigurationAsync → aspire config list) still calls kvp.Value.ToString() on leaf values and was not updated. For files written by the old code where booleans were stored as JSON strings ("true"), JsonNode.ToString() returns "\"true\"" (with quotes), while for proper JSON booleans it returns "true" (no quotes). So config get and config list can produce different representations for the same key on pre-fix files. Minor, but worth noting if you want the CLI output to be consistent.
mitchdenny
force-pushed
the
fix/config-bool-crash
branch
from
2b0a62f to
97e0fe3
Compare
Review feedback addressedAll changes from the review have been incorporated in the force-pushed commit:
|
dotnet-policy-service
bot
removed
the
needs-author-action
|
@JamesNK Changed approach here based on your feedback. We'll just emit "true" ourselves - but we'll be forgiving on parsing. |
|
|
||
| // Value is written as a JSON string "true", not a JSON boolean true. | ||
| // The FlexibleBooleanConverter handles parsing "true" -> bool on read. | ||
| Assert.Contains("\"true\"", result); |
There was a problem hiding this comment.
I'm not a fan of doing contains like this.
Instead, read result into JsonNode/JsonDocument, and get the value, and test its JSON type is String.
| var result = File.ReadAllText(settingsFilePath); | ||
|
|
||
| // Must be a JSON string "true", not a JSON boolean true | ||
| Assert.Contains("\"true\"", result); |
There was a problem hiding this comment.
I'm not a fan of doing contains like this.
Instead, read result into JsonNode/JsonDocument, and get the value, and test its JSON type is String.
| var result = File.ReadAllText(settingsFilePath); | ||
|
|
||
| // Non-boolean/numeric values remain as strings | ||
| Assert.Contains("\"daily\"", result); |
There was a problem hiding this comment.
I'm not a fan of doing contains like this.
Instead, read result into JsonNode/JsonDocument, and get the value, and test its JSON type is String.
| Assert.DoesNotContain("\"true\"", content); | ||
| Assert.DoesNotContain("\"false\"", content); |
There was a problem hiding this comment.
I'm not a fan of doing contains like this.
Instead, read result into JsonNode/JsonDocument, and get the value, and test its JSON type is String.
JamesNK
force-pushed
the
fix/config-bool-crash
branch
from
97e0fe3 to
2b0a62f
Compare
JamesNK
changed the title
mitchdenny
force-pushed
the
fix/config-bool-crash
branch
from
1a4f208 to
f9989df
Compare
When 'aspire config set features.X true' writes a JSON string "true" instead of a JSON boolean, AspireConfigFile.Load() fails to deserialize the Dictionary<string, bool> Features property. Fix by adding a FlexibleBooleanConverter that tolerates both JSON booleans and JSON string representations of booleans. The converter is registered globally via JsonSourceGenerationOptions.Converters so all bool properties benefit. Also fix TryNormalizeSettingsFile to use DeepClone() instead of ToString() when normalizing colon-separated keys, which was corrupting non-string types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
fix/config-bool-crash
branch
from
f9989df to
fec66fa
Compare
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
🎬 CLI E2E Test Recordings — 53 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23323666114 |
…ig.json (microsoft#15383)" This reverts commit aad1601.
Description
Fix crash when reading boolean config values from
aspire.config.json, and fix the write path to emit correct JSON types.ConfigurationService.SetNestedValuewrote all values as JSON strings (e.g.,"true"instead oftrue). WhenAspireConfigFile.Load()later deserialized the file,Dictionary<string, bool>could not parse the string values, causing aJsonExceptioncrash.Four root causes fixed:
FlexibleBooleanDictionaryConvertertoAspireConfigFile.Features— tolerates bothtrueand"true"on read, providing backward compatibility with config files written by older CLI versions.SetNestedValueto write proper JSON types — addedConvertToTypedJsonValueso booleans are written astrue/false, integers as numbers, and only non-parseable values remain as strings.TryNormalizeSettingsFileto preserve JSON value types — usesJsonNode.DeepClone()instead ofToString()which corrupted booleans to strings when normalizing colon-separated keys.GetConfigurationAsync—IConfigurationconverts JSON booleanstrue/falseto .NET strings"True"/"False". The read path now normalizes to lowercase soaspire config getoutput matches JSON conventions and is consistent with whataspire config setwrites.Validation
AspireConfigFileTests,ConfigurationServiceTests, andConfigurationHelperTests.Checklist