Implicitly set UserSecretsId for file-based apps #50783
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements implicit UserSecretsId setting for file-based applications to support user secrets functionality. This addresses a limitation where file-based apps couldn't use user secrets without explicit configuration.
- Automatically sets UserSecretsId as a hash of the entry point file path for file-based programs
- Adds comprehensive test coverage for user secrets functionality with both explicit and implicit ID scenarios
- Updates documentation to reflect the new implicit UserSecretsId behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props | Adds MSBuild property to implicitly set UserSecretsId for file-based apps |
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Adds test cases for user secrets functionality with both ID argument and file-based scenarios |
| documentation/general/dotnet-run-file.md | Documents the new implicit UserSecretsId behavior for file-based apps |
|
@RikkiGibson @333fred for reviews, thanks |
| var directives = VirtualProjectBuildingCommand.FindDirectives(sourceFile, reportAllErrors: !_force, DiagnosticBag.ThrowOnFirst()); | ||
|
|
||
| // Create a project instance for evaluation. | ||
| string entryPointFileDirectory = PathUtility.EnsureTrailingSlash(Path.GetDirectoryName(file)!); |
There was a problem hiding this comment.
Consider making this just a parameter to FindIncludedItems, in order to make things more clear. It feels unnecessary to rely on capture, when the variable is not used in the declaring method itself.
There was a problem hiding this comment.
The most straightforward thing might be to delete the declaration here and restore the declaration in FindIncludedItems().
There was a problem hiding this comment.
Agreed with Rikki, that this is only referenced via capture is somewhat confusing. I'd personally just pass this as a parameter where needed.
There was a problem hiding this comment.
The most straightforward thing might be to delete the declaration here and restore the declaration in
FindIncludedItems().
Yes, thanks, I extracted this line unintentionally (because it was in the block of code that I wanted to extract).
There was a problem hiding this comment.
I don't think the latest commit really addresses the concern, am I missing something? Or were Rikki and I commenting on different things? I assumed Rikki meant the entire projectCollection/projectInstance dual locals; I'm not a fan of how they're only referenced via capture.
There was a problem hiding this comment.
I thought about pushing on the capture thing on more than just entryPointFileDirectory but decided I didn’t feel too strongly. However I would generally prefer that locals are not used as “capture only” even if multiple local functions are capturing them.
There was a problem hiding this comment.
Are you saying we should never use non-static local functions?
I think some capturing makes for a cleaner code, although for code reviews I would recommend using VSCode with C# extension where you can actually hover over the function to see the captured variables.
I also don't like functions with a lot of "weakly" typed arguments (like bools and strings) as it's easy to pass the wrong thing to them. Capturing also helps with this, because you just reference the original thing by name.
Given this is a pre-existing issue (the existing local functions are already capturing variables) and you both signed off, I'm going to merge this, but happy to discuss further and change things in a follow up PR if needed.
| } | ||
|
|
||
| [Fact] | ||
| public void UserSecretsId_Overridden_ViaDirectoryBuildProps() |
There was a problem hiding this comment.
The following are corner cases, but it may be worthwhile to test them, just so we know the behavior.
- UserSecretsId is specified via
#:propertyand the value is exactly the same as the one we determined implicitly. - UserSecretsId is specified via
Directory.Build.propsand the value is exactly the same as the one we determined implicitly.
| var testInstance = _testAssetsManager.CreateTestDirectory(); | ||
|
|
||
| string code = $""" | ||
| #:package Microsoft.Extensions.Configuration.UserSecrets@*-* |
There was a problem hiding this comment.
I am unsure if it is best to use a wildcard version here. For example, it could be disruptive if someone were PRing into a servicing branch, and some interaction with a newer version of the package, was causing the old branch version of this test to fail.
There was a problem hiding this comment.
That's a good point, thanks.
| var directives = VirtualProjectBuildingCommand.FindDirectives(sourceFile, reportAllErrors: !_force, DiagnosticBag.ThrowOnFirst()); | ||
|
|
||
| // Create a project instance for evaluation. | ||
| string entryPointFileDirectory = PathUtility.EnsureTrailingSlash(Path.GetDirectoryName(file)!); |
There was a problem hiding this comment.
Agreed with Rikki, that this is only referenced via capture is somewhat confusing. I'd personally just pass this as a parameter where needed.
4 participants
Part of dotnet/aspnetcore#63440.
Limited to file-based apps only to alleviate the concerns raised in #50597 (comment).