[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1877: SOLR-13181: param macro expansion could throw

[GitBox]

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