jpountz commented on a change in pull request #1444:
URL: https://github.com/apache/lucene-solr/pull/1444#discussion_r414350099
##########
File path:
lucene/expressions/src/java/org/apache/lucene/expressions/SimpleBindings.java
##########
@@ -96,24 +90,51 @@ public DoubleValuesSource getDoubleValuesSource(String
name) {
case SCORE:
return DoubleValuesSource.SCORES;
default:
- throw new UnsupportedOperationException();
+ throw new UnsupportedOperationException();
}
}
- /**
- * Traverses the graph of bindings, checking there are no cycles or missing
references
- * @throws IllegalArgumentException if the bindings is inconsistent
+ @Override
+ public DoubleValuesSource getDoubleValuesSource(String name) {
+ if (map.containsKey(name) == false) {
+ throw new IllegalArgumentException("Invalid reference '" + name + "'");
+ }
+ return map.get(name).apply(this);
+ }
+
+ /**
+ * Traverses the graph of bindings, checking there are no cycles or missing
references
+ * @throws IllegalArgumentException if the bindings is inconsistent
*/
public void validate() {
- for (Object o : map.values()) {
- if (o instanceof Expression) {
- Expression expr = (Expression) o;
- try {
- expr.getDoubleValuesSource(this);
- } catch (StackOverflowError e) {
- throw new IllegalArgumentException("Recursion Error: Cycle detected
originating in (" + expr.sourceText + ")");
- }
+ for (String origin : map.keySet()) {
Review comment:
nit: use entrySet() since you consume both keys and values?
##########
File path:
lucene/expressions/src/test/org/apache/lucene/expressions/TestExpressionValidation.java
##########
@@ -110,4 +110,15 @@ public void testCoRecursion4() throws Exception {
});
assertTrue(expected.getMessage().contains("Cycle detected"));
}
+
+ public void testCoRecursion42() throws Exception {
Review comment:
I provided this test but I don't think we should add it as it relies on
iteration order and might get defeated on some JVMs or future versions of Java.
I'd suggest to not add this test, or to change the map in SimpleBindings from a
HashMap to a TreeMap to be able to better test such cases (in which case we'd
have to swap `cycle0`/`cycle2`), this could be done in a follow-up too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
