[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
cpoerschke commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492843243
##
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 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:
`${goodAnswer} ${noClose` is what I had in mind. Pushed a corresponding
commit to the PR's branch, hope you don't mind.
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
cpoerschke commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492843243
##
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 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:
`${goodAnswer} ${noClose` is what I had in mind. Pushed a corresponding
commit to the PR's branch, hope you don't mind.
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
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 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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
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 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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
cpoerschke commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r489465208
##
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##
@@ -136,11 +136,12 @@ private String _expand(String val) {
}
else if (idx < 0) {
if (sb == null) return val;
-sb.append(val.substring(start));
+sb.append(val, start, val.length());
return sb.toString();
}
// found unescaped "${"
+ int matchedStart = idx;
Review comment:
```suggestion
final int matchedStart = idx;
```
##
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##
@@ -154,14 +155,14 @@ else if (idx < 0) {
}
if (matchedStart > 0) {
-sb.append(val.substring(start, matchedStart));
+sb.append(val, start, matchedStart);
}
// update "start" to be at the end of ${...}
- start = rbrace + 1;
+ idx = start = rbrace + 1;
- // String inbetween = val.substring(idx, rbrace);
- StrParser parser = new StrParser(val, idx, rbrace);
+ // String in-between = val.substring(idx, rbrace);
Review comment:
How about removing rather than amending the `// String inbetween =
val.substring(idx, rbrace);` commented out code line?
##
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##
@@ -188,7 +189,7 @@ else if (failOnMissingParams) {
} catch (SyntaxError syntaxError) {
// append the part we would have skipped
-sb.append( val.substring(matchedStart, start) );
+sb.append(val, matchedStart, start);
Review comment:
observation: `foo.append(bar.substring(x,y));` is also found in other
places in the code base, not sure how it might work implementation wise but it
would be lovely if tooling would flag up that there's a more efficient
alternative
##
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##
@@ -136,11 +136,12 @@ private String _expand(String val) {
}
else if (idx < 0) {
if (sb == null) return val;
-sb.append(val.substring(start));
+sb.append(val, start, val.length());
return sb.toString();
}
// found unescaped "${"
+ int matchedStart = idx;
idx += macroStart.length();
int rbrace = val.indexOf('}', idx);
Review comment:
```suggestion
int rbrace = val.indexOf('}', matchedStart + macroStart.length());
```
similar to line 165 below and then the `idx += macroStart.length();`
assignment would not be needed since there would be no use of `idx` between
that incrementing assigment and the `idx = start = rbrace + 1;` resetting
assignment below.
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
