[core-http] - URLBuilder replaceAll drops existing path

[joheredi]

When the URL already has a path, and then a replaceAll is performed, the existing path is dropped. Here's an example:

import { URLBuilder } from "@azure/core-http";

const baseUri = "https://localhost/api/";

const url = URLBuilder.parse("{$host}");

// Note: If replaceAll is performed before append path, path won't be dropped
url.appendPath("test");
url.replaceAll("{$host}", baseUri);

console.log(url.toString());
// actual: https://localhost/api/
// expected: https://localhost/api/test/

This problem surfaces on ServiceClient.ts where the path from the operation spec is appended before replacing parametrized values in the URL, such as {$host}.

One quick way to fix ServiceClient.ts would be to move appendPath right after we've done replaceAll (here)

Root cause
The root cause seems to be here where if a path is present in the replace value, the current path is dropped.

Possible Fixes:

1. We could just change the order we call these in ServiceClient.ts, this would be the quickest option
2. Fix the URLBuilder state machine to make sure the existing path is preserved correctly

The obvious downside of (1) is that we'd be just fixing a symptom, however it would be substantially less risky. (2) would be the complete fix.

@xirzec, @daviwil, @chradek - I'm thinking we can try and do (1) to unblock impacted generated libraries, and then proceed with (2). What do you think?

Something to consider before tackling (2), though, would be to evaluate if URLBuilder will continue to be used as-is in the new @azure/core-https package, if so it would probably make sense to fix. If we'd re-write or drop it eventually, maybe (1) alone would be the best way to go.

For reference Azure/autorest.typescript/issues/673 is hitting this problem

[daviwil]

If this is the only place we know of that the issue appears, I'd be fine with the quick fix for now. We've discussed using some other library for URL handling in core-https so it might not be worth making big changes to our own right now.

[joheredi]

@daviwil, this is the only place I have seen this issue. Also I just did a quick search and seems like replaceAll is only used once in ServiceClient.ts, no other references in this repo

[daviwil]

I'd say it's fine to fix just this case, but I'd also be happy to defer to the wisdom of others if they object.

[joheredi]

Okay so I tried moving appendPath after replaceAll and it looks like the order on which they are called actually makes a difference. Autorest test suite reported ~70 404 failures with this change 😞

I think there may be a way to work around this issue by a change only in Autorest, I'll see if I can get it working there.

So (1) is not really a valid option, and if we are planning to use a different library for handling URLs, probably doesn't make sense to try fixing this at this time. (Assuming I can work around the issue in Autorest)

[xirzec]

To be clear the "library" we're going to use to manipulate URLs is the browser and Node primitive named URL. :)

Docs: https://developer.mozilla.org/en-US/docs/Web/API/URL

[ramya-rao-a]

We are moving to @azure/core-rest-pipeline across packages as part of #15594
Closing as we are not planning on further investments in @azure/core-http