tmenier
Provide immutable options
Great job with the library!
It'd be nice to have some or all of these capabilities:
- New Url constructor overload that takes a Url instance. This overload avoids a Url instance from being casted to string/Uri. Sort of like the Clone() method.
- New ReadOnlyUrl class This one may be more difficult to implement. Make class Url inherit from ReadOnlyUrl; or provide an interface IReadOnlyUrl and implement that interface in class Url. The relationship would be similar to List<> and IReadOnlyList<>. Perfect for method parameters to prevent methods from accidentally changing a Url instance. Also, this prevents the unnecessary cloning of Url instances simply to fake immutability. An interface IReadOnlyUrl is even better because a parameter with this type can receive a Url or ReadOnlyUrl instance. Being immutable, code may only call ToString() or ToUri() or Clone() or create a new Url instance passing in the IReadOnlyUrl instance. Code may also access the different parts of the URL.
The problem with the current immutability option is that you're forced to cast Url to a string/Uri. Then the called code has to recreate a Url instance.
For example, in my instance, the URL is created by a piece of code. I want to send the URL to a method via parameter so the method can perform some logic. Some of this logic is controlled by segments, fragments, and query string parameters.
Currently, the calling code builds the URL, casts to string, and sends that to the method. The method has to re-parse the URL and then access the URL components.
If an IReadOnlyUrl existed, the calling code can send the Url instance in the parameter and the method wouldn't need to create anything.
Another plus of IReadOnlyUrl is code security. A method that receives a Url instance is dangerous because the dev may add mutability incorrectly. In contrast, if the method receives IReadOnlyUrl, the dev can't add mutability, even inadvertedly.
New Url constructor overload that takes a Url instance...Sort of like the Clone() method.
Why not just use Clone? I think it's more explicit in what it does.
The problem with the current immutability option is that you're forced to cast Url to a string/Uri. Then the called code has to recreate a Url instance.
You don't have to do either of those things explicitly. Implicit conversions and string extension methods exist specifically to make this frictionless. Just define a method that takes a string URL. The caller can pass a Url object without casting it to a string, and the method can build on it as a new Url object (via extension methods) without explicitly creating one.
But despite it being frictionless, I do recognize that using strings as a go-between to achieve immutability isn't particularly intuitive. I wouldn't say it's a huge problem - I often point out the pattern and people get it and move on. Sometimes I think it should have been named UrlBuilder instead of Url, because it's actually quite similar to StringBuilder in nature. Both are mutable builder objects where the end result you want is a string. Your ideas are interesting and I'll give them some thought, but it would likely be a 4.0 thing.
I see that the current Clone() implementation is simply constructing a new Url instances by passing "this" into the constructor. Implicit casting takes place here. You could copy the fields instead to avoid the string/Uri middleman.
I get it that casting is frictionless, but it's still casting nonetheless. A string/Uri instance is still being created as a middleman. That's unnecessary resource consumption if all I want is pass a read-only version of the URL.
I agree, if the name was UrlBuilder, then this whole thing would be moot. However, I like the Url name. It's more intuitive. Being able to access URL parts is MUCH simpler with this library than with System.Uri.
Adding IReadOnlyUrl implementation would make the library perfect.
Performance and resource-management gains may not be much compared to any pain from implementing any of this. So I'll leave it in your capable hands.
I agree with @eoneillcarrillo that the Url should be immutable by default. I just wrote a bug where I got a base url in string format in my ctor and appended part of the path to it. In subsequent methods I'd add the last part of the url and query params. To my surprise, the field Url got changed when I added path segments.
Unit test to verify the scenario:
Url @base = "https://example.com".AppendPathSegments("api", "user");
Url details = @base.AppendPathSegments("details", 1);
Assert.Equal("https://example.com/api/user/details/1", details); // OK
Url roles = @base.AppendPathSegment("roles");
Assert.Equal("https://example.com/api/user/roles", roles); // Fail, actual value: https://example.com/api/user/details/1/roles
This is counter intuitive to what most other builders are doing and what I would expect will happen to the Url instance. I agree that this is a serious change, but I would expect this to be in v4 of Flurl. 🙂
@KenBonny
This is counter intuitive to what most other builders are doing
I think you have a misunderstanding of the builder pattern. Builders are mutable by definition. If base is a member variable, simply declare it as a string instead of a Url and with no other changes the bug should be fixed. I go back my point about naming being perhaps my biggest regret - UrlBuilder instead of Url probably would have been better. :)
Agree with @tmenier. Url class behaves more like a builder, so I'm fine with it mutating the internal value.
Would've been nice to have Url and UrlBuilder; with Url being an immutable instance.
UrlBuilder is, well, your builder with all the mutation it entails. It's perfect right now. I wouldn't change a thing.
Url gives you immutable access to the URL components, 'cause let's be honest, System.Uri kinda sucks in certain scenarios, specially when you want a dictionary out of the query parameters. Hell, I'd update all my code to use such a Url class instead of System.Uri. LOL I'd make it use lazy loading, though.
You could always move on to a major version with breaking changes. wink, wink
Or maybe, leave Url as it is now. Just provide extension methods to System.Uri for easy component retrieval. Something like GetQuery() or ParseQuery() which will return an IReadOnlyDictionary. You probably already have this code given you have to parse the query string for Url.
I did understand builder wrong. The builder changes with each call (I tried this with StringBuilder), that is on me. And I agree that the name should change to UrlBuilder. Been experimenting with a lot immutable code (F#) and I really like that an instance cannot change its properties. Eliminates a lot of bugs. The name Url confuses me, I thought each AppendPathSegment returned a new instance of Url.
And yes, I've updated the code to store @base as a string.
You're right. AppendPathSegment returns the same Url instance, not a new one. It's awesome for chaining calls.
I can imagine tmenier is pulling his hair about the name though. "Why didn't I name it UrlBuilder!!!!!"
Naming things... Not always the easiest thing, is it? LOL
First off, sorry for the late reply, I got swamped with work after I solved my problem here.
@eoneillcarrillo I don't agree with it being awesome for chaining calls. I would much rather have an immutable Url. See the example above. I thought I have a @base that referred to https://example.com/api/user and I appended /details/1 to that and afterwards I added /roles to the original url, not the newer /details/1. So chaining calls (like I did in my example above) did not work as I imagined it.
Second, I fixed a bug another developer wrote this week with Flurl. This was the situation:
var @base = "https://example.com".AppendPathSegments("api", "user");
foreach (var user in users)
{
var details = @base.AppendPathSegment("details", user.Id).GetJsonAsync<Details>();
// use details
}
To his surprise, this did not create a list of
-
https://example.com/api/user/details/1 -
https://example.com/api/user/details/2 -
https://example.com/api/user/details/3
But did
-
https://example.com/api/user/details/1 -
https://example.com/api/user/details/1/details/2 -
https://example.com/api/user/details/1/details/2/detail/3
Again, changing the @base to string fixed the problem, but in my opinion it does not do what it advertises on the box. Maybe it's me, but from such things, I expect immutability.
Just to be clear, this is still a great Nuget package and I still love Flurl for it's testing and ease of sending http calls. I just need to be aware of the builder pattern when appending segments in the future. 😀
I respect the opinions here but the fact is there are a LOT of users who like it the way it is and would have to deal with major pains if it changes. Take a simple (and very common) example:
var url = new Url(...);
if (something)
url.SetQueryParam("x", 1);
var result = await url.GetAsync();
If Url were to become immutable, that query param wouldn't survive to the end unless you enhance it with an assignment (i.e. url = url.SetQueryParam(...)). So this is breaking in a subtle way that the compiler and even tests might not catch.
I may someday consider adding an immutable alternative, but for now, even in the next major version, I don't want to change the nature of the existing Url object. I think it'll cause too many pains for too many people.
Agree. Any renaming would break a lot of code.
That's why you can introduce IReadOnlyUrl, like in my original post.
Leave all the existing classes alone, no need to touch them.
Have Url implement the new interface, which should be low effort given Url already supports everything.
The idea behind IReadOnlyUrl is to only expose read-only values and prevent devs from being tempted of mutating Url instances.
This way, there are no breaking changes while still supporting immutability for code that needs it.
No need to this now of course. I'm more than happy using Url as it is.