BaseUrl has been significantly changed to handle base URLs correctly and account for the case where the baseUrl on a mutated client should be changed. See https://github.com/spring-projects/spring-boot/issues/47659 and https://github.com/spring-projects/spring-boot/issues/47674.
While we have to do a late resolution of the url, I think it should be cached.
I also think that we should rename this to something else. Perhaps something more tied to the concept of a running test server. BaseUrl is very low-level and this class isn't. It would also help with the DEFAULT constant that's a bit strange at the moment.
Comment From: philwebb
I've applied some simplifications and the caching. I'm not sure what we should change the name to. BaseUrl makes sense in the context of the package, but isn't very clear in isolation.
Comment From: snicoll
I'm not sure what we should change the name to.
I don't think I understand the current name or why BaseUrl#LOCAHOST exists in the first place.
Looking at TestRestTemplateAutoConfiguration it creates a TestRestTemplate and defaults to LOCALHOST if no BaseUrl was found. Compared to 3.5.x it means that such an instance is created irrespective of the context having started an embedded web server. This doesn't look like we did this in 3.5.x. I didn't find a test for this either. I have removed the fallback in that auto-configuration and added a throw ISE locally if baseUrl is null to see what would happe. No tests seem to break.
HtmlUnit and Selenium are weird. The code we had in 3.5.x would replace any URL starting with / by localhost and the local port. UserVehicleControllerSeleniumTests and UserVehicleControllerHtmlUnitTests demonstrate a case where there isn't a running server and /sboot/vehicle.html is mapped to http://localhost:8080/sboot/vehicle.html (with no web server running on 8080). So weird, and I guess something treats localhost with any port as a mocked resource.
Looking at the implementations, they don't need a BaseUrl, they need a UriBuilderFactory and they should stop doing that "if starts with /" check. The check that verifies if BaseUrl is not null is also irrelevant given the fallback always gives LOCALHOST.
IMO BaseUrl is the wrong concept, I think it should be TestEmbeddedServer or something like that. And none should be provided if an embedded server isn't running. We could expose a getPort that could be a replacement/alternative for @LocalServerPort and the getter for UriBuilderFactory is very important as users must use this mechanism going forward where previously changing the baseUrl on the client would have worked. Auto-configuration for the HttpGraphQlTester demonstrates that already.
I also don't quite understand why BaseUrlProvider exists. Can't we contribute a BaseUrl if a servlet or reactive web environment with a server exists? Also, the pattern of injecting the ApplicationContext to a class defined in spring.factories worries quite a bit because that's a pattern where we offer "the world" and implementations are free to do whatever they want outside of the ApplicationContext lifecycle.
Comment From: philwebb
I don't think I understand the current name or why BaseUrl#LOCAHOST exists in the first place.
This might be wrong, it was done as part of the test restructuring without too much analysis. I think I was trying to copy the logic in LocalHostUriTemplateHandler. I seem to remember that in a mock MVC environment calling http://localhost:8080/foo will work but calling /foo will not.
I also don't quite understand why BaseUrlProvider exists. Can't we contribute a BaseUrl if a servlet or reactive web environment with a server exists?
We have a dependency issue because spring-boot-test needs to know about the concept of a BaseUrl, but it can't depend on spring-boot-web-server. That's why the the provider exists and things are named as they are. Code in spring-boot-test knows that there's the possibility something can provide a base URL, but it doesn't know what. Then spring-boot-web-server knows that it can provide one.
Comment From: snicoll
I've been brainstorming some more today and it more and more looks like something tied to the concept of an HTTP client calling the test embedded server. So it's both signaling the fact that there is indeed a web server running, and how to reach out to it. I know LOCALHOST right now can be used but I think it should be removed as it makes BaseUrlProviders#get with or without the feedback confusing. If you call it with a fallback, then you get something that may or may not talk to an actual server. We use this signal for the HTTP client in integration tests scenario.
If we ignore the SSL flag that can be added, it is literally what UriBuilderFactory does. The updated code in the issues I've referenced mostly deleted the custom code we had and delegate to framewok. From that perspective I wonder if we shouldn't find a name that's a bit more tied to UriBuilderFactory. Perhaps it could even implement the interface and BaseUrlUriBuilderFactory would be merged into it?
Unrelated to this, the time I spent this week on finding out how to delegate more to Framework, I came to the conclusion that HtmlUnit and Selenium could use to call UriBuilderFactory regardless of it being a mock or a running server. In the former case, we can create an instance as you described without exposing LOCALHOST. Once we do that, the responsibility of what's called now BaseUrl is more clear and we delegate more to framework.
We have a dependency issue
Right, I did connect the dots after I wrote my first comment. I still stand with my concern about loading things via spring.factories this way. I don't have a better solution to offer right now but, looking at the Javadoc of BaseUrlProvider, it looks like a third-party may contribute an implementation of it and I don't really see what the use case for that is. If there is none (at least immediately) can't we at least document that the interface is for internal use?
Code in spring-boot-test knows that there's the possibility something can provide a base URL, but it doesn't know what.
Is that a justification of why things are named this way? BaseURL is http and literally state it can be used to "connect to the running server". Given that, I think any naming that refers to "a running server" and "an HTTP client" is fine.