-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add debugging attributes to System.Security.Claims types #86424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones Issue DetailsFixes #86421 After changes: I think it might be worth adding debugger proxy views for Thoughts do far?
|
cc @ViktorHofer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it LGTM, I just need a confirmation from @ViktorHofer about how [DebuggerDisplayAttribute]
should be used in ref
files.
Thank you @JamesNK !
src/libraries/System.Security.Claims/ref/System.Security.Claims.cs
Outdated
Show resolved
Hide resolved
DebuggerDisplay is not part of the API contract. It's an implementation detail found via reflection by tools at execution time, and thus they're only relevant to the implementation assemblies. The genapi tool is told not to add them to ref assemblies: runtime/eng/DefaultGenApiDocIds.txt Line 12 in 2f18df4
|
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs
Outdated
Show resolved
Hide resolved
Thanks. Yes, I just tested removing them, and for some reason now the attributes work without being in the ref. I'm not sure what I did wrong before. |
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsIdentity.cs
Show resolved
Hide resolved
I just added type proxies. Original issue updated with screenshots. |
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsIdentity.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsIdentity.cs
Outdated
Show resolved
Hide resolved
"runtime (Build ios-arm64 Release AllSubsets_Mono)" is failing. It seems unrelated. Can I merge? Should I rebase to try to get a green build? |
It's unrelated. Merging. Thanks. |
Fixes #86421
After changes:
I have a question about
DebuggerDisplayAttribute
and ref assemblies. I added them to the implementation classes and ran the ref update command -dotnet msbuild /t:GenerateReferenceAssemblySource
- but they weren't added to the reference file. I had to do it manually. What should I do here?Extra work: It's worth adding debugger proxy views for
ClaimsPrincipal
andClaimsIdentity
. The proxy can remove the private/internal fields and display the counts from the claims collections. Proxies are more work so I'll hold off on that for now.Thoughts so far?
Update:
With type proxies:
ClaimsPrincipal:
ClaimsIdentity: