Sdk: launchSettings.json no longer allows comments after System.Text.Json migration

[anurse]

As part of the migration to System.Text.Json, it looks like Comment handling in launchSettings.json was lost. This issue does not repro on SDK 2.2.203.

System.Text.Json supports comment handling, but it has to be enabled in the options. It looks like the current code does not pass in any options: https://github.com/dotnet/cli/blob/0cb36481ad4e628079374447c8ae313006e3f602/src/dotnet/commands/dotnet-run/LaunchSettings/LaunchSettingsManager.cs#L28

Steps to reproduce

1. dotnet new web
2. Edit .\Properties\launchSettings.json to include a comment: // Spicy Comments
3. dotnet run

Expected behavior

It runs, spicy comments and all. Any launch settings (environment variables, etc.) are applied.

Actual behavior

The launch profile "(Default)" could not be applied.
An error was encountered when reading launchSettings.json.
'/' is invalid after a value. Expected either ',', '}', or ']'. LineNumber: 1 | BytePositionInLine: 2

The application does launch, but launch settings are ignored.

😢

Environment data

dotnet --info output:

.NET Core SDK (reflecting any global.json):
 Version:   2.2.203
 Commit:    e5bab63eca

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18865
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.2.203\

Host (useful for support):
  Version: 3.0.0-preview6-27804-01
  Commit:  fdf81c6faf

.NET Core SDKs installed:
  2.1.602 [C:\Program Files\dotnet\sdk]
  2.1.603 [C:\Program Files\dotnet\sdk]
  2.1.700-preview-009601 [C:\Program Files\dotnet\sdk]
  2.1.700-preview-009618 [C:\Program Files\dotnet\sdk]
  2.1.700 [C:\Program Files\dotnet\sdk]
  2.1.800-preview-009677 [C:\Program Files\dotnet\sdk]
  2.1.800-preview-009696 [C:\Program Files\dotnet\sdk]
  2.2.203 [C:\Program Files\dotnet\sdk]
  3.0.100-preview6-012264 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0-preview6.19307.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0-preview6-27804-01 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0-preview6-27804-01 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

[AdmiralSnyder]

in my environment, the line number in the error message also was off by one.

[livarcocc]

@vijayrkn for launchsettings.json

@ericstj @ahsonkhan about the changes in json parsing.

[ericstj]

Comments are disallowed by default but can be enabled in the reader settings. If you’d like to permit comments enable this setting : https://github.com/dotnet/corefx/blob/1a26c1617216c078e725ed0a697b93e2cc5d3074/src/System.Text.Json/ref/System.Text.Json.cs#L34. Which component is responsible for reading LaunchSettings? That’s where this change should go. @bartonjs @grabyourpitchforks

[anurse]

Yep, we've done this for the Microsoft.Extensions configuration logic and other places where we previously got comment handling through Json.NET's defaults.

I think the S.T.J behavior of disallowing comments by default is more spec-compliant but the places where we've migrated need to enable it to avoid regressing existing customer scenarios.

[vijayrkn]

for launchsettings.json

@BillHiebert - can you please help with this?

[nguerrera]

Which component is responsible for reading LaunchSettings?

Just to clarify, the code is in this repo, and owned by @vijayrkn's team.

[ericstj]

@nguerrera I wonder if the SDK had other files that supported comments previously but don't with the new JSON library:
https://github.com/dotnet/cli/search?q=JsonDocument.Parse&unscoped_q=JsonDocument.Parse&type=Code
https://github.com/dotnet/cli/search?q=JsonSerializer.Deserialize&unscoped_q=JsonSerializer.Deserialize&type=Code

It might be worthwhile doing a review.

[nguerrera]

@wli3 Please review those. Some are generated or new in this release, but not all AFAICT.

[wli3]

I discussed all possible behavior breaking files with the team already. RuntimeConfig is generated, we agreed the change in a stand up. Local tools are still in preview, break it is ok. AddPackageParser read from nuget web api. we don't own launchSettings.json

[nguerrera]

RuntimeConfig is generated,

Which runtime configs do we read in this repo? It is possible that they could be hand-edited in general. It is a config file after all and a very simple one. I could see someone adding a comment.

[wli3]

@nguerrera it could also have trailing comma and all small mismatch behavior with system.text.json. we need to draw the line somewhere. The change is in very early preview 3.0, so far no complain. I think it is appropriate

[nguerrera]

@wli3 That is reasonable. Thanks.

[anurse]

Just a data point to add to the .runtimeconfig.json discussion: The native core-host logic does permit comments in .runtimeconfig.json (I just tested it :)).

If there are places where the SDK is reading JSON files, it seems appropriate to make them as compatible with the old logic as possible (for the places we know we can support) to encourage users to move to the latest SDK without reservation. Obviously there will be unintentional breaks and things that can't be aligned, but I don't see why we should arbitrarily block things we know are currently allowed and can be allowed in the new parser.

It is also definitely a valid customer scenario to edit the .runtimeconfig.json file. I was just talking to a customer who was doing so :). Having said that, they are usually edited after deployment so the SDK's behavior doesn't matter as much there.

The launchSettings.json file is more critical though, as it is a clear customer scenario, so I'm relatively unenthusiastic about .runtimeconfig.json. I just don't want the line we draw to be arbitrary.

For further reference, the native host does not permit trailing commas in the .runtimeconfig.json file 😄.

[nguerrera]

Why are we reading runtimeconfig at all in this repo?

Let's just allow comments when doing so. I see risk and no upside in not allowing it.

[nguerrera]

Also feels like I should be allowed to add comments to tools manifest even if it hasn't ever sbipped rtm with comment support. Will, I'm sorry to be changing my mind here and also sorry that I don't remember the discussion you mention. It's like a one line change to opt in to comments, right? So far I haven't heard any real reason not to do that. Just reasons why it's not likely to be a breaking change.

[nguerrera]

As far as drawing the line somewhere, let's draw it where s.t.json gives us no recourse. It lets us support comments easily.

[wli3]

The "new line" is still not very convening. If we want to be 100% safe, we should duplicate the native json parsing code or just revert the migration. At the same time, as long as it is editable, it is possible to have all the corner cases. Again, why not also allow trailing comma and other options. In the end, I am not convinced we should or should not change it. And I think as long as we do not revert it, we are in the "feeling" territory for "the line".

So I think we should make a decision that is practical. Let's enable everything that s.t.json has to make it less strict. Since after preview 7 the API should be locked down. We don't have to keep adding more when the new API is available. (by the time the change is made, there is nearly no other loose option).

I can change runtimeconfig's logic. @BillHiebert and @vijayrkn could work on launchSettings.json

[nguerrera]

The practical approach is exactly what I meant by drawing the line where s.t.json gives us no recourse.

[nguerrera]

Comments should be allowed in any config file that a user can edit. Strictness in trailing commas is a nuisance but not the same as forbidding comments in user hostility.

[wli3]

Create https://github.com/dotnet/cli/issues/11918 for runtimeconfig.json