cpoerschke commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492216575
##########
File path:
solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java
##########
@@ -143,15 +142,31 @@ public void testMapExprExpandOn() {
String oldVal = System.getProperty("StreamingExpressionMacros","false");
System.setProperty("StreamingExpressionMacros", "true");
try {
- @SuppressWarnings({"rawtypes"})
- Map expanded = MacroExpander.expand(request);
- assertEquals("zero", ((String[])expanded.get("fq"))[0]);
- assertEquals("one", ((String[])expanded.get("fq"))[1]);
- assertEquals("two", ((String[]) expanded.get("fq"))[2]);
- assertEquals("three", ((String[]) expanded.get("fq"))[3]);
- assertEquals("one", ((String[])expanded.get("expr"))[0]);
+ Map<String, String[]> expanded = MacroExpander.expand(request);
+ assertEquals("zero", expanded.get("fq")[0]);
+ assertEquals("one", expanded.get("fq")[1]);
+ assertEquals("two", expanded.get("fq")[2]);
+ assertEquals("three", expanded.get("fq")[3]);
+ assertEquals("one", expanded.get("expr")[0]);
} finally {
System.setProperty("StreamingExpressionMacros", oldVal);
}
}
+
+ @Test
+ public void testUnbalanced() { // SOLR-13181
+ final MacroExpander meSkipOnMissingParams = new
MacroExpander(Collections.emptyMap());
+ final MacroExpander meFailOnMissingParams = new
MacroExpander(Collections.emptyMap(), true);
+ assertEquals("${noClose", meSkipOnMissingParams.expand("${noClose"));
Review comment:
How about adding (say) `${goodAnswer} ${noClose` as another case and it
turning into `42 ${noClose` if there is a `goodAnswer=42` mapping i.e. to
demonstrate that absence of closing curly bracket partially affects expansion
(as opposed to no expansion at all happening)?
##########
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##########
@@ -135,33 +135,34 @@ private String _expand(String val) {
}
}
else if (idx < 0) {
- if (sb == null) return val;
- sb.append(val.substring(start));
- return sb.toString();
+ break;
}
// found unescaped "${"
- idx += macroStart.length();
+ final int matchedStart = idx;
- int rbrace = val.indexOf('}', idx);
+ int rbrace = val.indexOf('}', matchedStart + macroStart.length());
if (rbrace == -1) {
// no matching close brace...
- continue;
Review comment:
> ... fix infinite loop when no close ...
Ah, yes, good catch!
And since the logic in the loop is quite complex the "infinite loop" wording
inspired me to go lookup again about the _loop invariants_ and _loop variants_
concepts and work it through here:
* `val.size() - idx` looks like the loops variant and to ensure loop
termination it must decrease i.e. `idx` must advance (or we must break or
return out of the loop)
* `idx > 0` leads to advancing
* `idx < 0` previously returned and now it breaks i.e. either gets us out of
the loop
* `idx == 0` if combined with `rbrace == -1` i.e. no closing would get us
stuck in the loop if `continue` is used but both `return null` or `break` get
us out of the loop
----------------------------------------------------------------
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
