Refit: NullReferenceException if BaseAddress not set on specified HttpClient

[martincostello]

I've just been playing with refit in an existing application that already had HttpClient wired up through DI, and initially made a mistake during the implementation where HttpClient.BaseAddress wasn't being set before being passed to RestService.For<T>(HttpClient).

https://github.com/paulcbetts/refit/blob/afacf7061b96581c4761f5484d70defb432a57e4/Refit/RequestBuilderImplementation.cs#L366

This resulted in a NullReferenceException being thrown. I was able to track this down by the line numbers being present in the refit stack trace, so I could look it up in GitHub, but troubleshooting this would have been simpler if this had been validated by the call to RestService.For<T>(HttpClient) with a custom exception message.

[clairernovotny]

The only concern here is that this could be a breaking change. You can get at the HttpClient instance today by putting an HttpClient Client {get;} member on your interface. Then you can set/change the BaseAddress property.

I think it's probably a bad idea to be changing the BaseAddress on an instance of the service, but it is allowed today. Granted, it'd still be allowed, but this would force someone to set a bogus BaseUrl first before setting/changing it later.

Thoughts @paulcbetts @Cheesebaron @bennor?

[wekempf]

If you can't require BaseAddress when you call For() (since BaseAddress is mutable, I'd say you can't) then in code you can throw an InvalidOperationException when you try to use BaseAddress and it's null. That's a far better debugging experience then dealing with NullReferenceException.

[bennor]

I can't properly look at the code from on my phone to see how it might break anything, but I'm not opposed to throwing a more useful exception. It _sounds_ like this is something that's already not working and all we need to do is make the problem clearer to users.

If that's all it is then I think it's a great idea. 👍

[clairernovotny]

Fixed