Skip to content
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

Merged
merged 2 commits into from
May 17, 2024

Conversation

andywebb1975
Copy link
Contributor

@andywebb1975 andywebb1975 commented May 10, 2024

Description

This refactors the preparePutOrPost method in HttpJdkSolrClient to make the code clearer, and remove the previous unreachable else block.

Tests

All tests still pass.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@andywebb1975 andywebb1975 changed the title Solr 17263 refactor SOLR-17263 fix (plus refactoring) May 10, 2024
ModifiableSolrParams urlParams = calculateQueryParams(urlParamNames, queryParams);

// note this is only triggered if urlParamNames is set too - this this correct?
urlParams.add(calculateQueryParams(solrRequest.getQueryParams(), queryParams));
Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@andywebb1975 andywebb1975 force-pushed the solr-17263-refactor branch 3 times, most recently from 889fab4 to c6c8ce4 Compare May 11, 2024 16:10
@andywebb1975 andywebb1975 changed the title SOLR-17263 fix (plus refactoring) Refactore preparePutOrPost in HttpJdkSolrClient May 11, 2024
@andywebb1975 andywebb1975 changed the title Refactore preparePutOrPost in HttpJdkSolrClient Refactor preparePutOrPost in HttpJdkSolrClient May 11, 2024
@andywebb1975 andywebb1975 marked this pull request as ready for review May 11, 2024 16:55
@andywebb1975 andywebb1975 requested a review from jdyer1 May 11, 2024 16:56
@dsmiley
Copy link
Contributor

dsmiley commented May 16, 2024

Shall this be merged?

@andywebb1975
Copy link
Contributor Author

@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.)

@jdyer1
Copy link
Contributor

jdyer1 commented May 17, 2024

@jdyer1 does this look OK to you? The tests pass...

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!

@andywebb1975 andywebb1975 merged commit ad0a797 into apache:main May 17, 2024
4 checks passed
@andywebb1975 andywebb1975 deleted the solr-17263-refactor branch May 17, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants