Repository navigation
Refactor preparePutOrPost in HttpJdkSolrClient - #2454
Conversation
| ModifiableSolrParams urlParams = calculateQueryParams(urlParamNames, queryParams); | ||
|
|
||
| // note this is only triggered if urlParamNames is set too - this this correct? | ||
| urlParams.add(calculateQueryParams(solrRequest.getQueryParams(), queryParams)); |
There was a problem hiding this comment.
I'm unclear when this would kick in - it can't trigger if urlParamNames is null.
There was a problem hiding this comment.
Indeed urlParamNames is never null (see HttpSolrClientBase constructor line 107ff), so the null check is spurious. Should you comment-out line 306, you will see testQueryString fail in the case where there are query parameters but nothing was set for urlParamNames (using the withTheseParamNamesInTheUrl option).
There was a problem hiding this comment.
queryParams can't be null either, can it? I've made this the else block - I added an UnsupportedOperationException in the original noBody() one and saw it was never triggered by tests. Does this change look good to you?
There was a problem hiding this comment.
Well, you are right, it does not seem urlParams or queryParams can ever be null. So you're correct, the "noBody" case never gets touched. I guess I had thought there would be a case where we would could post with an empty body but clearly this cannot happen.
11f4517 to
3e8700e
Compare
889fab4 to
c6c8ce4
Compare
andywebb1975
changed the title
andywebb1975
changed the title
a261185 to
70ac6cd
Compare
|
Shall this be merged? |
|
@jdyer1 does this look OK to you? The tests pass but I'm not totally familiar with the use case to know that this is definitely correct. (It's not a bug so I'd be happy to target 9.7.0 with this if we merge it.) |
I think your changes are valid and if all existing tests still pass then its a win. I am all in favor of no dead code branches! |
Description
This refactors the
preparePutOrPostmethod in HttpJdkSolrClient to make the code clearer, and remove the previous unreachableelseblock.Tests
All tests still pass.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.