define RouteValuesAddress .ToString () #39753

yecril71pl · 2022-01-25T18:23:45Z

define RouteValuesAddress .ToString ()

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Override RouteValuesAddress .ToString () as Name(ExplicitValues) for tracing/debugging.

Fixes #39708

javiercn · 2022-01-25T18:47:49Z

src/Http/Routing/src/RouteValuesAddress.cs

+    /// <summary>
+    /// Formats the address as string "Name(ExplicitValues)" for tracing/debugging.
+    /// </summary>
+    public override string ToString () => $"{RouteName}(" + string.Join(',', from kv in ExplicitValues select $"{kv.Key}=[{kv.Value}]") + ")";


Can you use the method call syntax instead of LINQ? we don't use linq anywhere in the repo

javiercn · 2022-01-25T18:48:16Z

Looks good to me

/cc: @JamesNK in case he has thoughts

JamesNK · 2022-01-25T18:49:47Z

Was there a PR some time ago about adding ToString to many types of route metadata? I have some memory of one. Was that merged or not?

javiercn · 2022-01-25T18:51:15Z

@JamesNK I don't remember TBH

JamesNK · 2022-01-25T18:52:36Z

#35231

javiercn · 2022-01-25T19:12:09Z

@JamesNK I think they are different scenarios? In this case is about logging (and I checked it only happens in Debug or Trace) so I think is much more limited in scope.

For things like Endpoints I think I also agree that it should be DebuggerDisplay()

halter73 · 2022-01-26T19:56:26Z

I think we should discuss this in API review.

ghost · 2022-01-26T19:56:28Z

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

davidfowl · 2022-01-26T20:13:41Z

src/Http/Routing/src/RouteValuesAddress.cs

@@ -3,6 +3,7 @@

 #nullable enable

+using System.Collections.Immutable;


Suggested change

using System.Collections.Immutable;

I tried, it will not build.

davidfowl · 2022-01-26T20:13:54Z

Why not just fix the log?

halter73 · 2022-01-26T20:28:34Z

Why not just fix the log?

How much do we care about changing the type of objects in the LogValues state? I don't think it's likely to break anyone, but I feel the same way about changing the default ToString() behavior which might be helpful in more scenarios.

JamesNK · 2022-01-26T20:40:16Z

Consider all of #35231 along with this change in API review. Making endpoint metadata more debuggable is good.

src/Http/Routing/src/RouteValuesAddress.cs

halter73 · 2022-01-29T03:01:23Z

src/Http/Routing/src/RouteValuesAddress.cs

@@ -24,4 +25,7 @@ public class RouteValuesAddress
    /// Gets or sets ambient route values from the current HTTP request.
    /// </summary>
    public RouteValueDictionary? AmbientValues { get; set; }
+
+    /// <inheritdoc />
+    public override string? ToString() => $"{RouteName}({string.Join(',', ExplicitValues.Select(kv => $"{kv.Key}=[{kv.Value}]"))})";


If we end up doing this, we should cache it.

Why do you think we should cache it?

javiercn · 2022-01-31T10:43:40Z

src/Http/Routing/src/RouteValuesAddress.cs

+    readonly Lazy<string> asString;
+
+    /// <summary>
+    /// The constructor prepares the internal state of the object.
+    /// </summary>
+    public RouteValuesAddress()
+    {
+        asString = new Lazy<string>(() => $"{RouteName}({string.Join(',', ExplicitValues.Select(kv => $"{kv.Key}=[{kv.Value}]"))})");
+    }


This doesn't need a public constructor. Caching this on the constructor is wrong since values can change after the first ToString call has been issued.

We also don't need to use Lazy as this code is not used in a multi-threaded context.

Do you want to track changes to the values so that we could invalidate the cache? This gets more and more Byzantine 😱

halter73 · 2022-01-31T21:34:57Z

Thanks for the PR @yecril71pl. We still need to decide whether we want to approve the API along with the other ToString() overrides suggested in #39792.

We plan to discuss this next Monday. We may end up deciding not to approve the API or decide not to do caching. I recommend leaving the PR as-is until then. That way the whole team can get on the same page in terms of feedback and you don't have to do so many iterations.

yecril71pl · 2022-01-31T21:41:52Z

We may end up deciding not to approve the API or decide not to do caching.

The converter is linear, this is a debug API, caching is for mission-critical features.

src/Http/Routing/src/RouteValuesAddress.cs

adityamandaleeka

This went through API review today. Merging.

define RouteValuesAddress .ToString ()

4fce698

yecril71pl requested a review from javiercn as a code owner January 25, 2022 18:23

ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jan 25, 2022

yecril71pl marked this pull request as draft January 25, 2022 18:23

javiercn reviewed Jan 25, 2022

View reviewed changes

yecril71pl added 6 commits January 25, 2022 21:48

do not use Linq

340b37f

no, really, no not use Linq

da4085d

using System.Collections.Immutable

b26cd86

fix formatting

0c7aad7

add RouteValuesAddress.ToString()

debdd5e

ToString() -> string!

b310774

yecril71pl marked this pull request as ready for review January 25, 2022 23:39

yecril71pl requested a review from a team as a code owner January 25, 2022 23:39

mkArtakMSFT assigned JamesNK Jan 25, 2022

halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 26, 2022

ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 26, 2022

davidfowl reviewed Jan 26, 2022

View reviewed changes

JamesNK mentioned this pull request Jan 26, 2022

Improve endpoint metadata debugging by overriding ToString #39792

Closed

dotnet deleted a comment from davidfowl Jan 27, 2022

Update PublicAPI.Unshipped.txt

425908e

pranavkm reviewed Jan 28, 2022

View reviewed changes

src/Http/Routing/src/RouteValuesAddress.cs Outdated Show resolved Hide resolved

Update src/Http/Routing/src/RouteValuesAddress.cs

108bead

pranavkm reviewed Jan 28, 2022

View reviewed changes

src/Http/Routing/src/RouteValuesAddress.cs Outdated Show resolved Hide resolved

Update src/Http/Routing/src/RouteValuesAddress.cs

627f70e

pranavkm reviewed Jan 28, 2022

View reviewed changes

src/Http/Routing/src/RouteValuesAddress.cs Outdated Show resolved Hide resolved

Update src/Http/Routing/src/RouteValuesAddress.cs

9b85a8f

pranavkm enabled auto-merge (squash) January 28, 2022 23:54

halter73 reviewed Jan 29, 2022

View reviewed changes

cache ToString as asString

6f6ba5f

auto-merge was automatically disabled January 29, 2022 09:31
Head branch was pushed to by a user without write access

yecril71pl added 4 commits January 29, 2022 10:43

fix formatting

9023beb

a lazy value must be defined in a constructor

49cae96

a useless API comment for the constructor

3d79c12

fix formatting

29f43c8

pranavkm assigned halter73 and unassigned pranavkm Jan 29, 2022

javiercn reviewed Jan 31, 2022

View reviewed changes

make Lazy thread-unsafe

ef43d90

Update RouteValuesAddress.cs

da994fd

pranavkm reviewed Feb 1, 2022

View reviewed changes

src/Http/Routing/src/RouteValuesAddress.cs Outdated Show resolved Hide resolved

Update src/Http/Routing/src/RouteValuesAddress.cs

ac9379e

adityamandaleeka approved these changes Feb 7, 2022

View reviewed changes

adityamandaleeka merged commit c2cf4e9 into dotnet:main Feb 7, 2022

ghost added this to the 7.0-preview2 milestone Feb 7, 2022

yecril71pl deleted the patch-2 branch February 7, 2022 22:16

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

		@@ -3,6 +3,7 @@

		#nullable enable

		using System.Collections.Immutable;

define RouteValuesAddress .ToString () #39753

define RouteValuesAddress .ToString () #39753

Uh oh!

Conversation

yecril71pl commented Jan 25, 2022

define RouteValuesAddress .ToString ()

Description

Uh oh!

javiercn Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

javiercn commented Jan 25, 2022

Uh oh!

JamesNK commented Jan 25, 2022

Uh oh!

javiercn commented Jan 25, 2022

Uh oh!

JamesNK commented Jan 25, 2022

Uh oh!

javiercn commented Jan 25, 2022

Uh oh!

halter73 commented Jan 26, 2022

Uh oh!

ghost commented Jan 26, 2022

Uh oh!

davidfowl Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

yecril71pl Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jan 26, 2022

Uh oh!

halter73 commented Jan 26, 2022

Uh oh!

JamesNK commented Jan 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

javiercn Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

javiercn Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

yecril71pl Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

halter73 commented Jan 31, 2022

Uh oh!

yecril71pl commented Jan 31, 2022

Uh oh!

Uh oh!

adityamandaleeka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!