aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Update C# code for #29448

Open paulomorgado opened this issue 1 year ago • 10 comments

Update C# code for #29448

  • [x] You've read the Contributor Guide and Code of Conduct.
  • [x] You've included unit or integration tests for your change, where applicable.
  • [x] You've included inline docs for your change, where applicable.
  • [x] 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

When #29448 was merged, there were no static lambdas in C# and AOT was not a thing.

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}. The string.Create method call has been updated to use a new lambda function that leverages the UriComponents struct to copy each component of the URI into the buffer, adjusting the buffer slice as needed. This function also handles the case where both PathBase and Path components have a slash, removing the trailing slash from PathBase to avoid duplication.

Finally, the BuildAbsolute method has been updated to use the new UriComponents struct and the updated string.Create call.

Related to #29448 and #28905

paulomorgado avatar Jun 05 '24 16:06 paulomorgado

CI failures look legit. Probably need some #ifs.

amcasey avatar Jun 05 '24 21:06 amcasey

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}.

I'm not an AOT expert, but isn't creating a new struct going to do basically the same thing in AOT as depending on ValueTuple{6}?

BrennanConroy avatar Jun 05 '24 23:06 BrennanConroy

@BrennanConroy,

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}.

I'm not an AOT expert, but isn't creating a new struct going to do basically the same thing in AOT as depending on ValueTuple{6}?

From @stephentoub on https://github.com/dotnet/aspnetcore/pull/55611#issuecomment-2143672330:

And it's going to contribute to a larger NativeAOT footprint, due to the use of the ValueTuple`5, which may not be otherwise used in the app and which brings with it a non-trivial footprint.

paulomorgado avatar Jun 07 '24 11:06 paulomorgado

ValueTuple implements multiple interfaces and as a result contains a lot of code that generally can't be trimmed. If it's not otherwise used in the app, it's unnecessary bloat. At the app level, not a big deal, but at the runtime level, we pay attention to such things.

stephentoub avatar Jun 07 '24 12:06 stephentoub

@amcasey,

CI failures look legit. Probably need some #ifs.

There was a name collision between System.UriComponent and Microsoft.AspNetCore.Http.Extensions.UriHelper+UriComponent (introduced in this PR) that I have solved.

The other errors look to me as environmental and not related to this change. Am I missing anything?

paulomorgado avatar Jun 07 '24 15:06 paulomorgado

@amcasey,

CI failures look legit. Probably need some #ifs.

There was a name collision between System.UriComponent and Microsoft.AspNetCore.Http.Extensions.UriHelper+UriComponent (introduced in this PR) that I have solved.

The other errors look to me as environmental and not related to this change. Am I missing anything?

Nope, I was referring to the name collision. The new failure is probably unrelated, though it's not one I've seen before. A nuget restore failure could have been a transient network issue?

amcasey avatar Jun 07 '24 16:06 amcasey

@amcasey, all builds seem to have succeeded.

paulomorgado avatar Jun 14 '24 23:06 paulomorgado

All good, @BrennanConroy?

amcasey avatar Jun 14 '24 23:06 amcasey

Hi @adityamandaleeka,

Anything blocking this PR?

paulomorgado avatar Aug 28 '24 12:08 paulomorgado