Optimize UrlEncode in Utils #3307
Conversation
…that can be executed
| @@ -23,6 +23,8 @@ | |||
| <NoWarn>$(NoWarn);CS1591;CA1822</NoWarn> | |||
|
|
|||
| <SignAssembly>True</SignAssembly> | |||
|
|
|||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
There was a problem hiding this comment.
Like discussed before this is required for some of the shim APIs.
| /// Encoding extensions used to provide a compatibility shim for encoding methods that are missing in .NET Standard | ||
| /// 2.0. | ||
| /// </summary> | ||
| internal static class EncodingExtensions |
There was a problem hiding this comment.
Let me know if you prefer another place to put it
| { | ||
| /// <summary>Indicates to the compiler that the .locals init flag should not be set in nested method headers when emitting to metadata.</summary> | ||
| [AttributeUsage(AttributeTargets.Module | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Event | AttributeTargets.Interface, Inherited = false)] | ||
| internal sealed class SkipLocalsInitAttribute : Attribute |
There was a problem hiding this comment.
Let me know if you prefer another place to put it
| @@ -75,12 +77,6 @@ public static partial class AWSSDKUtils | |||
| // Default value of progress update interval for streaming is 100KB. | |||
| public const long DefaultProgressUpdateInterval = 102400; | |||
|
|
|||
| internal static Dictionary<int, string> RFCEncodingSchemes = new Dictionary<int, string> | |||
There was a problem hiding this comment.
I did check the entire SDK and couldn't find another reference so I got rid of it. Is it possible to entirely get rid of the different RFC implementations? If that is possible we could potentially even get rid of the custom code and replace it with the System.Net implementation?
https://learn.microsoft.com/en-us/dotnet/api/system.net.webutility.urlencode?view=net-8.0
| @@ -6,7 +6,7 @@ This project file should not be used as part of a release pipeline. | |||
| --> | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>netstandard2.0;netcoreapp3.1;net8.0</TargetFrameworks> | |||
| <TargetFrameworks>netcoreapp3.1;net8.0</TargetFrameworks> | |||
There was a problem hiding this comment.
Netstandard is not a valid target for unit tests
|
|
||
| [Theory] | ||
| [InlineData("value, with special chars!", "value%2C%20with%20special%20chars!")] | ||
| [InlineData("value, with special chars and path {/+:}", "value%2C%20with%20special%20chars%20and%20path%20%7B/%2B:%7D")] |
There was a problem hiding this comment.
Should I add more data from the valid path chars?
| if (!TryGetRFCEncodingSchemes(rfcNumber, out var validUrlCharacters)) | ||
| validUrlCharacters = ValidUrlCharacters; | ||
|
|
||
| var unreservedChars = string.Concat(validUrlCharacters, path ? ValidPathCharacters : ""); |
There was a problem hiding this comment.
Given the encoding schemes are only used here the string concat could also go by already have the concatenated string available as costs and returning them by TryGetRFCEncodingSchemes
|
@normj @dscpinheiro I know this is still draft but would you mind having a look at it at some point? I have added a bunch of inline comments that might influence this PR. |
|
Ping @normj |
TBD
Description
TBD
Motivation and Context
https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/UrlEncode.cs
Testing
Added tests to verify the code still does the same as before.
Screenshots (if appropriate)
Types of changes
Checklist
License