Skip to content
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

Update C# code for #29448 #56086

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paulomorgado
Copy link
Contributor

Update C# code for #29448

  • 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

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2024
@paulomorgado paulomorgado force-pushed the paulomorgado/issue-28905 branch from 357eca4 to 2c7a6f8 Compare June 5, 2024 17:02
@amcasey
Copy link
Member

amcasey commented Jun 5, 2024

CI failures look legit. Probably need some #ifs.

@BrennanConroy
Copy link
Member

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}?

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jun 7, 2024

@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 #55611 (comment):

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.

@stephentoub
Copy link
Member

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.

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jun 7, 2024

@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?

@amcasey
Copy link
Member

amcasey commented Jun 7, 2024

@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?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 14, 2024
@paulomorgado
Copy link
Contributor Author

@amcasey, all builds seem to have succeeded.

@amcasey
Copy link
Member

amcasey commented Jun 14, 2024

All good, @BrennanConroy?

@paulomorgado
Copy link
Contributor Author

Hi @adityamandaleeka,

Anything blocking this PR?

This commit includes a significant refactor of the `UriHelper.cs` file. Unused namespaces `System.Buffers` and `System.Runtime.CompilerServices` have been removed. The `InitializeAbsoluteUriStringSpanAction` delegate and `InitializeAbsoluteUriString` method, as well as the `CopyTextToBuffer` method, have been removed.

A new struct `UriComponents` has been introduced to encapsulate the components of a URI: `Scheme`, `Host`, `PathBase`, `Path`, `Query`, and `Fragment`. 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.
@paulomorgado paulomorgado force-pushed the paulomorgado/issue-28905 branch from ee9a23a to 67f8239 Compare November 15, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants