Fixed assignment to catch block parameter

[dk2k]

Fixed assignment to catch block parameter

[github-actions]

✅ Gradle check result for d140044: SUCCESS

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.99%. Comparing base (abb1041) to head (d140044).
Report is 306 commits behind head on main.

Files with missing lines	Patch %	Lines
...main/java/org/opensearch/search/SearchService.java	50.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15384      +/-   ##
============================================
+ Coverage     71.93%   71.99%   +0.06%     
- Complexity    63223    63285      +62     
============================================
  Files          5214     5214              
  Lines        295938   295939       +1     
  Branches      42754    42754              
============================================
+ Hits         212876   213074     +198     
+ Misses        65601    65376     -225     
- Partials      17461    17489      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dblock]

Could really use a test on this path.

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[andrross]

@dbwiddis Does this logic make sense? If exception.getCause() is null, then we assign null to exception, which is going to NPE when we do throw exception; three lines later. Am I reading this right?

[dbwiddis]

@andrross I didn't fully study the logic, as it already existed.

The only change here was really making a copy of the e argument passed to the catch block and renaming all the other e to exception. It actually would have made a smaller diff had it been catch (Exception ex) followed by Exception e = ex and then no other changes.

That said, I see the former logic does include casting null as an Exception and then rethrowing it later. But all that said, will the cause of an ExecutionException ever be null in this situation? I don't think so.

[andrross]

Agree this change is good by itself.

The existing logic seems to be quite wonky, but I think you're right that the contract of ExecutionException is that the cause will never be null.

[dbwiddis]

the contract of ExecutionException is that the cause will never be null.

I think you can explicitly create one with the no-param constructor. But I can't find any instances of that in the OpenSearch codebase, and most other handling of this instanceof don't even bother to check for null. So we could probably remove the null check. It's an NPE in either case, just a different spot in the code.