fix(IsValidFilePath): Can now validate Windows UNC paths on Unix runtime

[Aditya Niraula (AdityaNPL)]

Before, Path.IsPathRooted(meta.PhysicalDeviceName); would return false for Windows UNC paths on Unix runtime due to a difference in dotnet's implementation of the function for the diff runtimes:

Windows Runtime: https://github.com/dotnet/runtime/blob/44f8f0faee42367bdd39277d8295f6e914ad7f4a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs#L194
vs.
Unix Runtime: https://github.com/dotnet/runtime/blob/44f8f0faee42367bdd39277d8295f6e914ad7f4a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs#L113

This would lead to backup files being filtered out and an exception being thrown during the creation of the backup chain.

Now we use the IsUnc property of the Uri class to detect UNC paths and since UNCs have to be fully-qualified by design, we can validate them straight away (just like how we do for URLs).

Have also made a few refactors to ease testing of this new approach.

[Aditya Niraula (AdityaNPL)]

Alex Irion (@alexirion10) Ryan Clare (@Kaneraz)
Wondering if we can use Uri for this function too and simplify it so that the UNC check goes there instead?

AgDatabaseMove/src/SmoFacade/BackupFileTools.cs

Lines 16 to 19 in babf28a

	public static bool IsUrl(string path)
	{
	return Regex.IsMatch(path, @"(http(|s):\/)(\/[^\s]+)+\.([a-zA-Z]+)$");
	}

[Alex Irion (alexirion10)]

Looks good. Ya you should be able to use new Uri constructor on URLs instead of the regex.
I thought we were done with DFS backup paths by now...

[Aditya Niraula (AdityaNPL)]

Cool, will change that function too.
We are migrating away from it, but looks like we do need to support some DBs with it for the time being.

[Aditya Niraula (AdityaNPL)]

Alex Irion (@alexirion10) I'm just wondering now, do we really need all these checks for the url/path.
Can't we just use it as it is and let the consumer deal with any issues that arise from it (at the end of the day the issue was because of the metadata we got from their MSSQL instance)?
I don't see why this module should be in charge of validating them?

I believe invalid, relative and incorrect paths/urls would error out naturally when you try to use it?

Moreover, I believe having these silent filters here could lead to a head scratching problem for users as their files would mysteriously disappear in the backup chain without any warnings in some cases.

What's your view on this?

[Alex Irion (alexirion10)]

Aditya Niraula (@AdityaNPL) - for FireFish that might be the case, but not for the rest of the usecase in this repository.
See https://github.com/factset/AgDatabaseMove/blob/master/src/AgDatabaseMove.cs#L76

[Aditya Niraula (AdityaNPL)]

My opinion here is that we can treat our functions in this module (like _options.Destination.Restore) as consumers of the backup chain too. So we let it handle any issues with incorrect paths however it wants to.
(basically the BackupChain class takes no responsibility for checking if meta-data is correct, just returns it as is but in the right order)

I believe, this will keep things loosely-coupled and we won't have to handle edge-cases like these in the future.
Let me know what you think.

---

To address this particular example:

AgDatabaseMove/src/AgDatabaseMove.cs

Line 76 in babf28a

_options.Destination.Restore(backupList, _options.FileRelocator);

looks like under the hood it calls this SMO function: Microsoft.SqlServer.Management.Smo.Restore.SqlRestore(Server):

AgDatabaseMove/src/SmoFacade/Server.cs

Line 164 in babf28a

restore.SqlRestore(_server);

So I was thinking a better design would be to let the SMO function raise an exception during the restore process if it can't get the file for any reason. We can then propagate the error back to the user.
(same for any other function, we let the underlying tool throw the error when it can't get the file and we propagate it back)

[Alex Irion (alexirion10)]

You're then going to need to check file validity in every consuming location, hence duplicating that code and logic everywhere. I don't want to go down that path.
SMO.restore is already going to throw if the file doesn't exist or it can't restore for whatever reason.
This "tightly coupled" file validity check is just to assure us that the path is somewhat reasonable, not that it'll reachable or that it's actually a SQL backup file

[Aditya Niraula (AdityaNPL)]

So we let it handle any issues with incorrect paths however it wants to.

Just re-read my comment above and looks like it was a bit confusing here.
What I mean to say is that consumers (i.e. end-users of this tool) can then handle errors with wrong paths in any way they want.

For the functions in this tool that consume the backup chain, the functions can just return the same error they get from the underlying SMO functions - so no need to duplicate any logic anywhere.

[Aditya Niraula (AdityaNPL)]

Had a conversation with Alex Irion (@alexirion10) offline about this.

We came to the conclusion that there may be some aspects of AgDatabaseMove module that may restrict us in getting rid of the path/url checks all together right now. Mainly the error recovery design if the restore process fails because of incorrect paths (it might not be re-runnable).

For this PR we decided to keep it simple and just change the url/path checks and the respective tests.

However, I want to see if we can remove these checks easily.
Will need some investigating before we decide, so filed an issue to be placed in the backlog to be looked at a later time - #65