-
Notifications
You must be signed in to change notification settings - Fork 700
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear when this would kick in - it can't trigger if urlParamNames
is null.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
preparePutOrPost
method in HttpJdkSolrClient to make the code clearer, and remove the previous unreachableelse
block.Tests
All tests still pass.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.