-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix assert tool url normalization #2778
Conversation
@janhoy thanks for taking a stab at this... I definitly have been playing "whack a mole" of adding and removing /solr/'s in various places. I will go through this today.... I appreciate your digging in with fresh eyes on this problem. |
I would love to have less classes touching the URL and base path. This was also one of the goals of the merge in #2756. Unifying the options that accept a URL should reduce the possible input variations, allowing us to simplify the logic internally. About the remaining failing test |
This is specific to our 9x branch. Not sure if it needs to be on other Tools as well.
Okay, so now the tests all passed @janhoy.. I kind of hacked in something to work for PostTool, I actually thought it would be needed on more other tools... |
hostContext = "/solr"; | ||
} | ||
|
||
solrUrl = SolrCLI.normalizeSolrUrl(solrUrl, true, hostContext) + hostContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of the same workaround, and guess I'm medium happy about it. Would of course prefer a SolrCLI
util method that hides this such as SolrCLI.getAndNormalizeSolrUrl(cli), and consider using it wherever we do raw
cli.getOptionValue("solr-url")`, since we'd potentially run into same issue elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I would like to find a better way of handling URL composition. Using the v1 API should also be discouraged in the long run.
Note that I am unable to manually test on Windows, as the script is currently broken. gradlew check
finishes without errors.
@epugh, @malliaridis Turns out this still fails half the time, when So we need some kind of extra check that we don't get two |
@janhoy For some reason I am not surprised. I guess a I would prefer to use a URL builder which we can feed the path segments with and without leading and trailing slashes and it will take care of everything. Otherwise we would have to add just another workaround, like if (hostContext.equals("/")) {
hostContext = "";
} right after the current workaround Using URL builders solrUpdateUrl = UriBuilder.fromUri(SolrCLI.normalizeSolrUrl(solrUrl, true, hostContext))
.path(hostContext)
.path(cli.getOptionValue("name")) // "testBasicRun"
.path("/update")
.path("/with/")
.path("slashes/")
.path("/again")
.build();
// generates http://127.0.0.1:52371/solr/testBasicRun/update/with/slashes/again would correctly generate the path. Maybe we could use a more robust solution like that? Note that using solrUpdateUrl = UriBuilder.fromUri(SolrCLI.normalizeSolrUrl(solrUrl, true, hostContext) + hostContext)
.segment(cli.getOptionValue("name"), "/update", "/with/", "slashes/", "/again")
.build();
// generates http://127.0.0.1:52215/solr/testBasicRun/%2Fupdate/%2Fwith%2F/slashes%2F/%2Fagain |
Of the two URI builders, I think we would want the jakarta since at some point Apache HTTP Client goes away. What about just looking for I remember when we looked at the hostContext a while ago and the reason we changed to just requiring |
Draft PR to get a green state in branch_9x
Tests used to pass on branch_9x a few days ago. Some of the many
SolrCLI
changes touched the urlNormalization stuff so that you needed to add/solr
at the end of some URLs passed to Solr.In particular this was true for
AssertTool
, which I fix here by getting the SolrClient from SolrCLI util method instead of constructing anHttp2HttpSolrClient
manually. The difference is that the former adds a /solr before constructing the client, and this is consistent with other client initializations in AssertTool.The whole Solr Base Path and
/solr
suffix is a mess, and it is different in main than in branch_9x. So this will keep cropping up for as long as we backport stuff from 10 to 9 :(