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

SOLR-16483: Fix IndexOutOfBounds in RecursiveNumericEvaluator #1054

Merged
merged 6 commits into from Oct 20, 2022

Conversation

acsbendi
Copy link
Contributor

@acsbendi acsbendi commented Oct 7, 2022

https://issues.apache.org/jira/browse/SOLR-16483

Description

This change fixes the following exception:

2022-10-04 12:21:26.033 ERROR (...) [...] o.a.s.c.s.i.s.ExceptionStream java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0 => java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Unknown Source)
java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
        at jdk.internal.util.Preconditions.outOfBounds(Unknown Source) ~[?:?]
        at jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Unknown Source) ~[?:?]
        at jdk.internal.util.Preconditions.checkIndex(Unknown Source) ~[?:?]
        at java.util.Objects.checkIndex(Unknown Source) ~[?:?]
        at java.util.ArrayList.get(Unknown Source) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).eval.RecursiveNumericEvaluator.normalizeInputType(RecursiveNumericEvaluator.java:55) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).eval.RecursiveEvaluator.recursivelyEvaluate(RecursiveEvaluator.java:214) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).eval.RecursiveEvaluator.evaluate(RecursiveEvaluator.java:202) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).stream.LetStream.open(LetStream.java:195) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).stream.ExceptionStream.open(ExceptionStream.java:49) ~[?:?]
        at org.apache.solr.handler.StreamHandler$TimerStream.open(StreamHandler.java:486) ~[?:?]
        at [org.apache.solr.client.solrj.io](http://org.apache.solr.client.solrj.io/).stream.TupleStream.writeMap(TupleStream.java:77) ~[?:?]
        at org.apache.solr.common.util.JsonTextWriter.writeMap(JsonTextWriter.java:168) ~[?:?]
        at org.apache.solr.common.util.TextWriter.writeMap(TextWriter.java:241) ~[?:?]
        at org.apache.solr.common.util.TextWriter.writeVal(TextWriter.java:85) ~[?:?]
        at org.apache.solr.response.TextResponseWriter.writeVal(TextResponseWriter.java:205) ~[?:?]
        at org.apache.solr.common.util.TextWriter.writeVal(TextWriter.java:45) ~[?:?]
        at org.apache.solr.common.util.JsonTextWriter.writeNamedListAsMapWithDups(JsonTextWriter.java:387) ~[?:?]
        at org.apache.solr.common.util.JsonTextWriter.writeNamedList(JsonTextWriter.java:295) ~[?:?]
        at org.apache.solr.response.JSONWriter.writeResponse(JSONWriter.java:77) ~[?:?]
        at org.apache.solr.response.JSONResponseWriter.write(JSONResponseWriter.java:63) ~[?:?]
        at org.apache.solr.response.QueryResponseWriterUtil.writeQueryResponse(QueryResponseWriterUtil.java:71) ~[?:?]
        at org.apache.solr.servlet.HttpSolrCall.writeResponse(HttpSolrCall.java:999) ~[?:?]
        at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:631) ~[?:?]
        at org.apache.solr.servlet.SolrDispatchFilter.dispatch(SolrDispatchFilter.java:239) ~[?:?]
        at org.apache.solr.servlet.SolrDispatchFilter.lambda$doFilter$0(SolrDispatchFilter.java:207) ~[?:?]
        at org.apache.solr.servlet.ServletUtils.traceHttpRequestExecution2(ServletUtils.java:257) ~[?:?]
        at org.apache.solr.servlet.ServletUtils.rateLimitRequest(ServletUtils.java:227) ~[?:?]
        at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:202) ~[?:?]

Solution

I check if the list if empty before accessing its first item.

Tests

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

@sonatype-lift
Copy link

sonatype-lift bot commented Oct 7, 2022

⚠️ 314 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acsbendi can you add a test case for this? looks like its a streaming expression potentially?

@acsbendi
Copy link
Contributor Author

@risdenk Sure, where should I add one? Yes, this happened in a streaming expression, should I add a test for that streaming expression?

@risdenk
Copy link
Contributor

risdenk commented Oct 17, 2022

I do see a https://github.com/apache/solr/blob/main/solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/eval/RecursiveEvaluatorTest.java, but no RecursiveNumericEvaluatorTest so take a look and see where your test case might go.

@acsbendi
Copy link
Contributor Author

@risdenk I've added a test for it, and checked that it fails without my fix:)

@acsbendi acsbendi requested a review from risdenk October 18, 2022 08:25
Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @acsbendi - running ./gradlew check -x test would surface a few errors.

  • Assert.... shouldn't be needed. just use assertEquals
  • Add license header that matches other tests

@acsbendi
Copy link
Contributor Author

My pleasure:) The problems are fixed now

@acsbendi acsbendi requested a review from risdenk October 18, 2022 13:59
@risdenk risdenk self-assigned this Oct 18, 2022
@risdenk
Copy link
Contributor

risdenk commented Oct 18, 2022

./gradlew tidy should fix up any formatting stuff that was found.

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acsbendi this looks good. I'm going to create a jira ticket and make a minor change (add solr/CHANGES.txt entry)

@risdenk risdenk changed the title Fix IndexOutOfBounds in RecursiveNumericEvaluator SOLR-16483: Fix IndexOutOfBounds in RecursiveNumericEvaluator Oct 20, 2022
@risdenk risdenk merged commit b50367a into apache:main Oct 20, 2022
@risdenk
Copy link
Contributor

risdenk commented Oct 20, 2022

Thanks @acsbendi

@acsbendi
Copy link
Contributor Author

@risdenk You're welcome, glad to be able to contribute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants