Add System.Web.VirtualPathUtility #179

CZEMacLeod · 2022-08-28T11:59:27Z

Add System.Web.VirtualPathUtility implementation for AspNet Core
Add an implementation of System.Web.VirtualPathUtility based on the codebase for net framework

Adds System.Web.VirtualPathUtility based (in part) on
System.Web.Util.UrlPath code at https://github.com/microsoft/referencesource/blob/master/System.Web/Util/UrlPath.cs
System.Web.Util.StringUtil at https://github.com/microsoft/referencesource/blob/master/System.Web/Util/StringUtil.cs

Addresses #83

src/Microsoft.AspNetCore.SystemWebAdapters/Ref.Standard.cs

src/Microsoft.AspNetCore.SystemWebAdapters/VirtualPathUtility.cs

twsouthwick

This looks really good! It's reassuring seeing those tests that behavior is aligned :)

src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCore.SystemWebAdapters.csproj

test/Microsoft.AspNetCore.SystemWebAdapters.Tests/VirtualPathUtilityTests.cs

twsouthwick

nit: some whitepace issues

test/Microsoft.AspNetCore.SystemWebAdapters.Tests/VirtualPathUtilityTests.cs

src/Microsoft.AspNetCore.SystemWebAdapters/VirtualPathUtility.cs

CZEMacLeod · 2022-08-29T19:29:42Z

nit: some whitepace issues

I think all of these are covered in the latest update.

src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCore.SystemWebAdapters.csproj

twsouthwick · 2022-08-30T19:28:19Z

@CZEMacLeod there may be a delay in merging this per the comment here #83 (comment). @adityamandaleeka can provide an update here

twsouthwick · 2022-08-30T19:29:10Z

While we're waiting - do you think that #185 would help with some of the testing of non-default virtual directories?

CZEMacLeod · 2022-08-31T13:52:27Z

@twsouthwick I'm not sure it does anything here. All the tests pass a fixed app virtual path rather than using the overload that defaults to the HttpRuntime value.
The difficulty is actually running these tests on full framework, rather than running them here.
Certainly, for MapPath #184, it would be useful to be able to mock AppDomainAppPath with a known value, as this would, by default, vary depending on the project path.
I didn't add any direct tests on that, as it wasn't really possible to mock httpruntime and the request, but I guess with #185 it would be possible to do this without actually running it on a project (as I do in samples/ClassLibrary/RequestInfo.cs)
I might need some help putting that together though as I'm not entirely familiar with the mocking classes/process you have.

twsouthwick · 2022-08-31T16:27:53Z

The difficulty is actually running these tests on full framework, rather than running them here.

I'd say that's a relatively low priority. The policy I've followed is to verify certain behavior happens on framework, and then create tests to mimic that as closely as possible using what we need to mock it up locally

I might need some help putting that together though as I'm not entirely familiar with the mocking classes/process you have.

We've been using Moq for mocking and Autofixture for generating test data (if needed).

I didn't add any direct tests on that, as it wasn't really possible to mock httpruntime and the request

Yup, that's the point of #185 to add the ability to do that. The VirtualPathUtility can follow a similar pattern and be an instance class internally that takes in an IHttpRuntime and is set by the IApplicationBuilder similar to the runtime.

twsouthwick · 2022-09-02T21:31:15Z

@CZEMacLeod Can you please include the following file in your PR? https://github.com/dotnet/systemweb-adapters/blob/tasou/license/THIRD-PARTY-NOTICES.txt

Once that's in we can move forward with the PR. Thanks for your patience.

CZEMacLeod · 2022-09-03T12:48:04Z

I don't understand why these tests failed this time round - they worked previously, and the only change was adding the text file.

twsouthwick · 2022-09-03T17:18:56Z

Try merging locally and see if it changes things. I believe the builds merge with main and build/test that

twsouthwick

@CZEMacLeod In comparing with the framework sources, looks like you pulled in the implementation needed. Would it be much work to keep the implementation using UrlPath, VirtualPath, StringUtil and the other helper classes as is? Additional infrastructure we may want to bring in will be using those as well and it would be nice to keep the similar shape so as we bring in more stuff we don't start duplicating stuff.

CZEMacLeod · 2022-09-05T18:09:18Z

Try merging locally and see if it changes things. I believe the builds merge with main and build/test that

Ah - because #185 has been merged, the tests now fail because HttpRuntime is not set. I'll have to try and rebase after #185 and update the tests to initialize HttpRuntime.

CZEMacLeod · 2022-09-05T18:15:35Z

@CZEMacLeod In comparing with the framework sources, looks like you pulled in the implementation needed. Would it be much work to keep the implementation using UrlPath, VirtualPath, StringUtil and the other helper classes as is? Additional infrastructure we may want to bring in will be using those as well and it would be nice to keep the similar shape so as we bring in more stuff we don't start duplicating stuff.

I only brought in a small part of UrlPath and StringUtil, and didn't bring in VirtualPath at all - I didn't see any need for it and it seems to just result in a lot of allocations and mem copying which aren't needed.
There have also been a number of updates/tweaks to the code afterwards.
Those classes also use resource items for error messages etc. which have all been removed.
Bringing in the whole of those classes would probably be an exercise on its own. If you do decide to bring them in, in the future, you could check if any of the functions in VirtualPathUtility match and swap out to use the shared code.
For now, I would prefer to keep this PR limited to the original scope of implementing just that class.

twsouthwick · 2022-09-06T18:41:22Z

I only brought in a small part of UrlPath and StringUtil, and didn't bring in VirtualPath at all - I didn't see any need for it and it seems to just result in a lot of allocations and mem copying which aren't needed.

Yeah, the challenge is that a lot of stuff internally ends up relying on these methods/classes. It will simplify future work to bring stuff in in the same shape and then we can deal with any perf impacts en masse (i.e. VirtualPath could be converted to use spans/be a struct/etc). I'd really prefer that this happens as part of this PR, otherwise, I'll probably end up needing to redo it to support that scenario. No need to bring in the whole classes - just the parts you used.

When you say "didn't bring in VirtualPath" at all, you mean you inlined it, right? All the implementation at https://referencesource.microsoft.com/#System.Web/VirtualPathUtility.cs,2b829f9059f552ca shows things being delegated to VirtualPath/StringUtil/UrlPath for the work. Those methods that are required are what I'd like to see brought in with this PR (nothing beyond what is needed - that easily can balloon beyond what the original scope here is).

Merge weirdness

Looks like your merge may have done something weird. It's showing files that are in the project already as being additions. Maybe try rebasing and force pushing?

CZEMacLeod · 2022-09-06T22:03:00Z

@twsouthwick The implementation of VirtualPathUtility is partly my own, based on the utility methods. It doesn't use VirtualPath at all, nor did I inline any of that. I did move some of the code up from the UrlPath and StringUtil classes up to the main methods when nothing else was needed, but skipped all the VirtualPath stuff entirely. It is mainly using some of the utility methods combined with implementing what the docs say.
I could certainly move some of the support methods back into those classes, although - e.g. for the MapPath implementation in #184 it just uses the functions of VirtualPathUtility directly rather than hitting the underlying stuff.

As for the merge stuff - yeah. I had to pull in the changes from #185 due to the changes to (I)HttpRuntime, then it thought there were merge conflicts for a file I didn't touch, so I brought in the other changes from #186 so it would merge correctly. I think it is fine now, but I can try rebasing it again if needed.

twsouthwick · 2022-09-06T22:06:59Z

Ok - I think I understand now. For now, then, can you put the methods you pulled from UrlPath and StringUtil back into static class with those names and we can skip the VirtualPath stuff for now?

Merge stuff

I'm still seeing a bunch of erroneous changes. It may be GitHub showing the diff weird, but can you try rebasing?

CZEMacLeod · 2022-09-07T11:16:58Z

I'm looking at breaking out the UrlPath stuff and found that some of the methods rely directly or indirectly on HttpRuntime - we had previously discussed potentially making VirtualPathUtility an instance class like HttpRuntime and injecting IHttpRuntime. This is definitely going to make that process more complicated.
There is only a single method in StringUtil that is used - StringStartsWithIgnoreCase.

…n net6+

…dotnet#181

…e.SystemWebAdapters.csproj Co-authored-by: Taylor Southwick <[email protected]>

This reverts commit 4f07395.

CZEMacLeod · 2022-09-07T11:25:04Z

@twsouthwick I've broken out the methods into the util classes, and rebased. Hopefully this will a) merge correctly without whatever weirdness you were seeing and b) build, and test correctly (it does locally).

twsouthwick

LGTM! Thanks @CZEMacLeod for the contribution and working to get it ready to merge.

There's definitely room for improvement as you pointed out in the StringUtil and UrlPath classes, but it will allow to bring in more functionality more easily as so much code in System.Web ends up relying on these helpers. These methods can receive some targeted fixes in the future. (some of them look even like they're probably built-in functions that could do it now)

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/StringUtil.cs

twsouthwick · 2022-09-07T15:37:56Z

Hopefully this will a) merge correctly without whatever weirdness you were seeing and b) build, and test correctly (it does locally).

Yup on both accounts :) I've seen that weirdness with GH when doing merges. Not exactly sure why, but rebasing seems to fix it.

Tratcher · 2022-09-07T16:08:57Z

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/UrlPath.cs

+
+    internal static bool HasTrailingSlash(string virtualPath) => virtualPath[^1] == '/';
+
+    internal static bool IsRooted(string basepath) => string.IsNullOrEmpty(basepath) || basepath[0] == '/' || basepath[0] == '\\';


Empty is rooted?

It is according to the original code - https://github.com/microsoft/referencesource/blob/master/System.Web/Util/UrlPath.cs#L33
Null or Empty tends to get caught further up in the calls in VirtualPathUtility and either returns something explicit or throws.

FYI These methods are taken as-is (mostly) from System.Web and is how they handled things. Not sure how well we can reason about some of these things :)

Yes - UrlPath and StringUtil (other than code optimizations based on newer c# / .net functions and error strings) are pretty much a direct port.

Tratcher · 2022-09-07T16:12:33Z

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/UrlPath.cs

+        // e.g c:\foo
+        if (path[1] == ':' && IsDirectorySeparatorChar(path[2])) return true;


This allows \:\. What restrictions should be checked for the first character?

Again - this is based on the original code. I'm not sure there is ever a reason that something like \:\ would be allowed as a virtual path - the use of this function is to throw if it is not a valid virtual path in CheckValidVirtualPath.

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/UrlPath.cs

Tratcher · 2022-09-07T16:51:50Z

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/UrlPath.cs

+    }
+
+    // Same as Reduce, but for a virtual path that is known to be well formed
+    internal static string ReduceVirtualPath(string path)


could this path contain escaped dots or slashes?

Unless the original call did, I don't think so. This is mainly used with the result of MakeVirtualPathAppAbsolute or MakeRelative which would normally be called with hardcoded strings from the developer, and possibly the current request path, which I think should also be 'clean'.
As it says in the description - only use on a virtual path that is known to be well formed.
This code is pretty much untouched from the original .Net implementation, other than some changes to substring handling and error messages.
If it needs to be made more robust than the original, then I think that is a separate issue?

Style: always use the brackets Co-authored-by: Chris Ross <[email protected]>

twsouthwick reviewed Aug 28, 2022

View reviewed changes

src/Microsoft.AspNetCore.SystemWebAdapters/Ref.Standard.cs Outdated Show resolved Hide resolved

twsouthwick reviewed Aug 28, 2022

View reviewed changes

src/Microsoft.AspNetCore.SystemWebAdapters/VirtualPathUtility.cs Show resolved Hide resolved

twsouthwick reviewed Aug 29, 2022

View reviewed changes

CZEMacLeod force-pushed the czemacleod/virtualpathutility branch from 588fe95 to f03fed1 Compare August 29, 2022 19:27

twsouthwick reviewed Aug 29, 2022

View reviewed changes

src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCore.SystemWebAdapters.csproj Outdated Show resolved Hide resolved

twsouthwick mentioned this pull request Aug 30, 2022

Implement MapPath and AppDomainAppPath #184

Merged

twsouthwick reviewed Sep 3, 2022

View reviewed changes

CZEMacLeod added 8 commits September 7, 2022 12:20

Add System.Web.VirtualPathUtility

6d524b3

Add tests and correct a few edge cases in VirtualPathUtility

5eaa215

Fix null reference issues in tests

22d52e1

Align exceptions with framework and remove resource file

c259ae3

Add comment detailing reason why some data for tests is conditional o…

cbd601f

…n net6+

Regenerate Ref.Standard.cs and TypeForwards.Framework.cs after merging …

c27720e

…dotnet#181

Clean up tabs/spaces in tests and remove some blank lines

bc4a3fd

Work around issue with netstandard and nullability

0cb34b1

CZEMacLeod and others added 6 commits September 7, 2022 12:20

Update src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCor…

71b9b91

…e.SystemWebAdapters.csproj Co-authored-by: Taylor Southwick <[email protected]>

add excluded attributes

c3b6975

Revert "Work around issue with netstandard and nullability"

77cd972

This reverts commit 4f07395.

Add THIRD-PARTY-NOTICES.tx

2df2800

Update VPP Tests to support (I)HttpRuntime changes from dotnet#185

cca5264

Break methods back out into StringUtil and UrlPath

bd6cf61

CZEMacLeod force-pushed the czemacleod/virtualpathutility branch from 78f4a21 to bd6cf61 Compare September 7, 2022 11:22

twsouthwick approved these changes Sep 7, 2022

View reviewed changes

src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/StringUtil.cs Show resolved Hide resolved

twsouthwick requested a review from Tratcher September 7, 2022 15:37

CZEMacLeod added 2 commits September 7, 2022 17:41

Add summaries and attribution for StringUtil and UrlPath classes

650e446

Add licence headers

533e351

Tratcher approved these changes Sep 7, 2022

View reviewed changes

CZEMacLeod and others added 2 commits September 7, 2022 18:26

Update src/Microsoft.AspNetCore.SystemWebAdapters/Utilities/UrlPath.cs

f60a864

Style: always use the brackets Co-authored-by: Chris Ross <[email protected]>

Removed legacy bug references from code

7d4b4b0

CZEMacLeod force-pushed the czemacleod/virtualpathutility branch from 9a5d3ca to 7d4b4b0 Compare September 7, 2022 17:33

twsouthwick merged commit f7ef75a into dotnet:main Sep 7, 2022

twsouthwick mentioned this pull request Sep 7, 2022

Add AspNet Core side shim for System.Web.VirtualPathUtility #83

Closed


		internal static bool HasTrailingSlash(string virtualPath) => virtualPath[^1] == '/';

		internal static bool IsRooted(string basepath) => string.IsNullOrEmpty(basepath) \|\| basepath[0] == '/' \|\| basepath[0] == '\\';

		// e.g c:\foo
		if (path[1] == ':' && IsDirectorySeparatorChar(path[2])) return true;

Add System.Web.VirtualPathUtility #179

Add System.Web.VirtualPathUtility #179

Uh oh!

Conversation

CZEMacLeod commented Aug 28, 2022

Uh oh!

Uh oh!

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CZEMacLeod commented Aug 29, 2022

Uh oh!

Uh oh!

twsouthwick commented Aug 30, 2022

Uh oh!

twsouthwick commented Aug 30, 2022

Uh oh!

CZEMacLeod commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twsouthwick commented Aug 31, 2022

Uh oh!

twsouthwick commented Sep 2, 2022

Uh oh!

CZEMacLeod commented Sep 3, 2022

Uh oh!

twsouthwick commented Sep 3, 2022

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

CZEMacLeod commented Sep 5, 2022

Uh oh!

CZEMacLeod commented Sep 5, 2022

Uh oh!

twsouthwick commented Sep 6, 2022

Uh oh!

CZEMacLeod commented Sep 6, 2022

Uh oh!

twsouthwick commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CZEMacLeod commented Sep 7, 2022

Uh oh!

CZEMacLeod commented Sep 7, 2022

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

twsouthwick commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CZEMacLeod commented Aug 31, 2022 •

edited

Loading

twsouthwick commented Sep 6, 2022 •

edited

Loading