Re-enabling Windows tests

[mjcarroll]

Fix tests around Windows' handling (or not handling) of : and @ in filesystem paths.

To address this, I did a few things.

1. If a URI has an authority (that is a username or a port in the format username@host:port), then we URL encode that to get a "safe" path.

2. Make sure that we are passing hasAuthority = true for all of our URIs that use https. This may be a behavior that we want to change in gz-common eventually, but is sufficient for here.

3. Fixes UniqueName to generate valid paths for both ModelIdentifier and WorldIdentifier. I also choose to make UniqueName always return unix-delimited paths, which cleans up some windows-specific test logic.

I believe that this resolves the test failures in FuelClientTest.CachedWorld and FuelClientTest.PatchModelFail as described in #373.

When forward ported to main, this should resolve #231

This (finally) resolves #106

[azeey]

LGTM! Just a couple of questions.

Also, have you seen gazebosim/gz-common#38? I'm wondering if we did proper encoding in gz-common, we wouldn't need some of the changes in this PR.

[azeey]

Do we need to do this for CacheWorld or is it not necessary since we don't include worlds in SDF files?

[mjcarroll]

It wasn't covered by existing tests, so I probably didn't notice it. I suppose it's unnecessary without include-ing in SDF.

[azeey]

I noticed that UniqueName uses Server().Url().Str(), while the code here previously had Server().Url().Path().Str(). Is this intentional?

[mjcarroll]

This is because previously when we had a web URL, we weren't setting the authority and treating the whole thing as a path, when really the domain name is the authority in this case

# Before (authority = false)
https://fuel.gazebosim.com/user/model
  Authority: ""
  Path: "fuel.gazebosim.com/user/model"

https://foo@localhost:8080/user/model -> 
  Authority: ""
  Path: "foo@localhost:8080/user/model"

# After (authority = true)
https://fuel.gazebosim.com/user/model
  Authority: "fuel.gazebosim.com"
  Path: "user/model"

https://foo@localhost:8080/user/model -> 
  Authority: "foo@localhost:8080"
  Path: "user/model"

So we need the "full" URL returned by Url() to get the Authority as part of the unique name.

[azeey]

Ah, I misunderstood Path as a function that somehow converts the URL into a file path, not returning the formal Path of a URL. This makes sense. Thanks!

[mjcarroll]

I'm wondering if we did proper encoding in gz-common, we wouldn't need some of the changes in this PR.

I think that's likely the case, but when we discussed it earlier, I think we decided that making that change in gz-common5 would be too much and potentially be an API breakage? This was some time ago, so we may be able to revisit that.

Additionally, with bumping gz-common, it may be that we can just fix it there and most of this goes away in a forward port to main?

[azeey]

Additionally, with bumping gz-common, it may be that we can just fix it there and most of this goes away in a forward port to main?

Okay, I've added to the list of things to consider working on for Ionic. I think this is good to merge!

[Crola1702]

Could this be backported to fix #391?

[mjcarroll]

Could this be backported to fix #391?

No, we determined that the behavior change couldn't be ported back, so the tests should be disabled as in #387