You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/09/16 06:25:43 UTC
[GitHub] [lucene-solr] dsmiley opened a new pull request #1877: SOLR-13181: param macro expansion could throw
dsmiley opened a new pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877
StringIndexOutOfBoundsException on bad syntax
TODO run tests etc.
https://issues.apache.org/jira/browse/SOLR-13181
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492990280
##########
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:
Ah; thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gus-asf commented on pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#issuecomment-696867296
Did see your review request, but my life is doing that whole "it never rains but it pours" thing right now and I keep not getting to this. My primary concern wrt to this class is the need to ensure that default installations do not expand streaming expression parameters (but folks who need that for existing installs can turn it on with knowledge of the risks). I saw your comment on the ticket that fixed that originally, and yeah calling out a single parameter is yucky, but it was the least back compat breaking thing I could come up with. A more sensible, less backwards compatible solution for 9x is certainly a possibility. I may not have time to do any deep review here soon enough, so don't wait on me.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492226769
##########
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:
I'm not 100% clear what you suggest; let me try to guess. Let's say the parameter map has exactly one param == goodAnswer with value 42.
````
assertEquals("42", meSkipOnMissingParams.expand("${goodAnswer}"));
assertEquals("42", meFailOnMissingParams.expand("${goodAnswer}"));
````
Is that what you suggest?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
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<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:
`${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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
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<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:
`${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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#issuecomment-696943683
@gus-asf (in reference to you're fix in SOLR-12891): I can appreciate you didn't want to break back-compat. In this issue/PR, I'm not getting at that matter at all, it's a separate bug fix when the syntax is given wrong.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r490430100
##########
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:
IntelliJ pointed this out to me :-) It didn't know about the val.substring(start) (implied val.length()) so I did those manually. Yes, it'd be nice if everywhere we could do this in one go. Feel free to file an issue to do so if you have time. IntelliJ can make quick work of that.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r490480292
##########
File path: solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
##########
@@ -136,14 +136,15 @@ 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 "${"
+ final int matchedStart = idx;
idx += macroStart.length();
Review comment:
Can now remove idx manipulation until later when idx & start are changed in one line
```suggestion
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r490438268
##########
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:
```suggestion
// String in-between braces
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley merged pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#issuecomment-696943683
@gus-asf (in reference to you're fix in SOLR-12891): I can appreciate you didn't want to break back-compat. In this issue/PR, I'm not getting at that matter at all, it's a separate bug fix when the syntax is given wrong.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492990280
##########
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:
Ah; thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#discussion_r492226769
##########
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:
I'm not 100% clear what you suggest; let me try to guess. Let's say the parameter map has exactly one param == goodAnswer with value 42.
````
assertEquals("42", meSkipOnMissingParams.expand("${goodAnswer}"));
assertEquals("42", meFailOnMissingParams.expand("${goodAnswer}"));
````
Is that what you suggest?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gus-asf commented on pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877#issuecomment-696867296
Did see your review request, but my life is doing that whole "it never rains but it pours" thing right now and I keep not getting to this. My primary concern wrt to this class is the need to ensure that default installations do not expand streaming expression parameters (but folks who need that for existing installs can turn it on with knowledge of the risks). I saw your comment on the ticket that fixed that originally, and yeah calling out a single parameter is yucky, but it was the least back compat breaking thing I could come up with. A more sensible, less backwards compatible solution for 9x is certainly a possibility. I may not have time to do any deep review here soon enough, so don't wait on me.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley merged pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #1877:
URL: https://github.com/apache/lucene-solr/pull/1877
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org