[CALCITE-1988] Various code quality issues * Fix error that SqlNode.clone does not call super.clone; deprecate it, and add SqlNode.clone(SqlNode) * Fix error that NlsString.clone does not call super.clone * Make AggregateNode.AccumulatorList static, because non-static inner classes must not implement Serializable * Change XmlOutput.content to not use LineNumberReader.readLine, which is susceptible to a DoS attack
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/8441e79c Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/8441e79c Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/8441e79c Branch: refs/heads/master Commit: 8441e79cdde01a901de894d44b597cf582a259d1 Parents: d173640 Author: Julian Hyde <[email protected]> Authored: Mon Sep 18 11:56:35 2017 -0700 Committer: Julian Hyde <[email protected]> Committed: Mon Oct 2 11:13:43 2017 -0700 ---------------------------------------------------------------------- .../calcite/interpreter/AggregateNode.java | 2 +- .../calcite/sql/SqlBinaryStringLiteral.java | 2 +- .../calcite/sql/SqlCharStringLiteral.java | 2 +- .../org/apache/calcite/sql/SqlDateLiteral.java | 2 +- .../apache/calcite/sql/SqlIntervalLiteral.java | 7 ++-- .../java/org/apache/calcite/sql/SqlLiteral.java | 2 +- .../java/org/apache/calcite/sql/SqlNode.java | 12 ++++++- .../apache/calcite/sql/SqlNumericLiteral.java | 10 ++---- .../org/apache/calcite/sql/SqlTimeLiteral.java | 2 +- .../apache/calcite/sql/SqlTimestampLiteral.java | 2 +- .../calcite/sql/fun/SqlCoalesceFunction.java | 11 +++--- .../calcite/sql/fun/SqlNullifFunction.java | 5 ++- .../calcite/sql/validate/SqlValidatorImpl.java | 11 +++--- .../calcite/sql/validate/SqlValidatorUtil.java | 8 ++--- .../java/org/apache/calcite/util/NlsString.java | 10 ++++-- .../java/org/apache/calcite/util/XmlOutput.java | 28 +++++++++------ .../java/org/apache/calcite/util/UtilTest.java | 37 ++++++++++++++++++++ 17 files changed, 98 insertions(+), 55 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java b/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java index b25a268..47b933b 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java +++ b/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java @@ -356,7 +356,7 @@ public class AggregateNode extends AbstractSingleNode<Aggregate> { /** * A list of accumulators used during grouping. */ - private class AccumulatorList extends ArrayList<Accumulator> { + private static class AccumulatorList extends ArrayList<Accumulator> { public void send(Row row) { for (Accumulator a : this) { a.send(row); http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlBinaryStringLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlBinaryStringLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlBinaryStringLiteral.java index 01ad109..71380e1 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlBinaryStringLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlBinaryStringLiteral.java @@ -56,7 +56,7 @@ public class SqlBinaryStringLiteral extends SqlAbstractStringLiteral { return (BitString) value; } - public SqlNode clone(SqlParserPos pos) { + @Override public SqlBinaryStringLiteral clone(SqlParserPos pos) { return new SqlBinaryStringLiteral((BitString) value, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlCharStringLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlCharStringLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlCharStringLiteral.java index 9f15a22..b74176c 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlCharStringLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlCharStringLiteral.java @@ -63,7 +63,7 @@ public class SqlCharStringLiteral extends SqlAbstractStringLiteral { return getNlsString().getCollation(); } - public SqlNode clone(SqlParserPos pos) { + @Override public SqlCharStringLiteral clone(SqlParserPos pos) { return new SqlCharStringLiteral((NlsString) value, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlDateLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlDateLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlDateLiteral.java index 1a757f6..27cc8b4 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlDateLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlDateLiteral.java @@ -42,7 +42,7 @@ public class SqlDateLiteral extends SqlAbstractDateTimeLiteral { return (DateString) value; } - public SqlNode clone(SqlParserPos pos) { + @Override public SqlDateLiteral clone(SqlParserPos pos) { return new SqlDateLiteral((DateString) value, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlIntervalLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlIntervalLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlIntervalLiteral.java index bd2969c..246e673 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlIntervalLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlIntervalLiteral.java @@ -64,11 +64,8 @@ public class SqlIntervalLiteral extends SqlLiteral { //~ Methods ---------------------------------------------------------------- - public SqlNode clone(SqlParserPos pos) { - return new SqlIntervalLiteral( - (IntervalValue) value, - getTypeName(), - pos); + @Override public SqlIntervalLiteral clone(SqlParserPos pos) { + return new SqlIntervalLiteral((IntervalValue) value, getTypeName(), pos); } public void unparse( http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlLiteral.java index 4fcafb9..4cf88d7 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlLiteral.java @@ -234,7 +234,7 @@ public class SqlLiteral extends SqlNode { } } - public SqlNode clone(SqlParserPos pos) { + public SqlLiteral clone(SqlParserPos pos) { return new SqlLiteral(value, typeName, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlNode.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlNode.java b/core/src/main/java/org/apache/calcite/sql/SqlNode.java index e7c77fb..1d22f96 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlNode.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlNode.java @@ -62,10 +62,20 @@ public abstract class SqlNode implements Cloneable { //~ Methods ---------------------------------------------------------------- + /** @deprecated Please use {@link #clone(SqlNode)}; this method brings + * along too much baggage from early versions of Java */ + @Deprecated + @SuppressWarnings("MethodDoesntCallSuperMethod") public Object clone() { return clone(getParserPosition()); } + /** Creates a copy of a SqlNode. */ + public static <E extends SqlNode> E clone(E e) { + //noinspection unchecked + return (E) e.clone(e.pos); + } + /** * Clones a SqlNode with a different position. */ @@ -104,7 +114,7 @@ public abstract class SqlNode implements Cloneable { for (int i = 0; i < clones.length; i++) { SqlNode node = clones[i]; if (node != null) { - clones[i] = (SqlNode) node.clone(); + clones[i] = SqlNode.clone(node); } } return clones; http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java index b754a36..45f9dfb 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java @@ -65,13 +65,9 @@ public class SqlNumericLiteral extends SqlLiteral { return isExact; } - public SqlNode clone(SqlParserPos pos) { - return new SqlNumericLiteral( - (BigDecimal) value, - getPrec(), - getScale(), - isExact, - pos); + @Override public SqlNumericLiteral clone(SqlParserPos pos) { + return new SqlNumericLiteral((BigDecimal) value, getPrec(), getScale(), + isExact, pos); } public void unparse( http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlTimeLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlTimeLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlTimeLiteral.java index 4327e2a..be3a04d 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlTimeLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlTimeLiteral.java @@ -44,7 +44,7 @@ public class SqlTimeLiteral extends SqlAbstractDateTimeLiteral { return (TimeString) value; } - public SqlNode clone(SqlParserPos pos) { + @Override public SqlTimeLiteral clone(SqlParserPos pos) { return new SqlTimeLiteral((TimeString) value, precision, hasTimeZone, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/SqlTimestampLiteral.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/SqlTimestampLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlTimestampLiteral.java index cc659d5..41d9fe4 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlTimestampLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlTimestampLiteral.java @@ -39,7 +39,7 @@ public class SqlTimestampLiteral extends SqlAbstractDateTimeLiteral { //~ Methods ---------------------------------------------------------------- - public SqlNode clone(SqlParserPos pos) { + @Override public SqlTimestampLiteral clone(SqlParserPos pos) { return new SqlTimestampLiteral((TimestampString) value, precision, hasTimeZone, pos); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java index fc8d3c8..e2efc47 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java @@ -70,17 +70,14 @@ public class SqlCoalesceFunction extends SqlFunction { // todo: optimize when know operand is not null. - for (int i = 0; (i + 1) < operands.size(); ++i) { + for (SqlNode operand : Util.skipLast(operands)) { whenList.add( - SqlStdOperatorTable.IS_NOT_NULL.createCall( - pos, - operands.get(i))); - thenList.add(operands.get(i).clone(operands.get(i).getParserPosition())); + SqlStdOperatorTable.IS_NOT_NULL.createCall(pos, operand)); + thenList.add(SqlNode.clone(operand)); } SqlNode elseExpr = Util.last(operands); assert call.getFunctionQuantifier() == null; - return SqlCase.createSwitched( - pos, null, whenList, thenList, elseExpr); + return SqlCase.createSwitched(pos, null, whenList, thenList, elseExpr); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/fun/SqlNullifFunction.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlNullifFunction.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlNullifFunction.java index 0aa310e..63cc774 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlNullifFunction.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlNullifFunction.java @@ -67,9 +67,8 @@ public class SqlNullifFunction extends SqlFunction { SqlNodeList thenList = new SqlNodeList(pos); whenList.add(operands.get(1)); thenList.add(SqlLiteral.createNull(SqlParserPos.ZERO)); - return SqlCase.createSwitched( - pos, operands.get(0), whenList, thenList, operands.get(0).clone( - operands.get(0).getParserPosition())); + return SqlCase.createSwitched(pos, operands.get(0), whenList, thenList, + SqlNode.clone(operands.get(0))); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java index cd83186..90bdb88 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java +++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java @@ -1191,8 +1191,7 @@ public class SqlValidatorImpl implements SqlValidatorWithHints { selectList.add(SqlIdentifier.star(SqlParserPos.ZERO)); final SqlNodeList orderList; if (getInnerSelect(node) != null && isAggregate(getInnerSelect(node))) { - orderList = - orderBy.orderList.clone(orderBy.orderList.getParserPosition()); + orderList = SqlNode.clone(orderBy.orderList); // We assume that ORDER BY item does not have ASC etc. // We assume that ORDER BY item is present in SELECT list. for (int i = 0; i < orderList.size(); i++) { @@ -1281,9 +1280,7 @@ public class SqlValidatorImpl implements SqlValidatorWithHints { // from the update statement's source since it's the same as // what we want for the select list of the merge source -- '*' // followed by the update set expressions - selectList = - (SqlNodeList) updateStmt.getSourceSelect().getSelectList() - .clone(); + selectList = SqlNode.clone(updateStmt.getSourceSelect().getSelectList()); } else { // otherwise, just use select * selectList = new SqlNodeList(SqlParserPos.ZERO); @@ -1305,7 +1302,7 @@ public class SqlValidatorImpl implements SqlValidatorWithHints { SqlNode sourceTableRef = call.getSourceTableRef(); SqlInsert insertCall = call.getInsertCall(); JoinType joinType = (insertCall == null) ? JoinType.INNER : JoinType.LEFT; - SqlNode leftJoinTerm = (SqlNode) sourceTableRef.clone(); + final SqlNode leftJoinTerm = SqlNode.clone(sourceTableRef); SqlNode outerJoin = new SqlJoin(SqlParserPos.ZERO, leftJoinTerm, @@ -1331,7 +1328,7 @@ public class SqlValidatorImpl implements SqlValidatorWithHints { new SqlNodeList( rowCall.getOperandList(), SqlParserPos.ZERO); - SqlNode insertSource = (SqlNode) sourceTableRef.clone(); + final SqlNode insertSource = SqlNode.clone(sourceTableRef); select = new SqlSelect(SqlParserPos.ZERO, null, selectList, insertSource, null, null, null, null, null, null, null); http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java index c56c0ec..2200e01 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java +++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java @@ -1005,7 +1005,7 @@ public class SqlValidatorUtil { } public SqlNode visit(SqlLiteral literal) { - return (SqlNode) literal.clone(); + return SqlNode.clone(literal); } public SqlNode visit(SqlIdentifier id) { @@ -1020,15 +1020,15 @@ public class SqlValidatorUtil { } public SqlNode visit(SqlDataTypeSpec type) { - return (SqlNode) type.clone(); + return SqlNode.clone(type); } public SqlNode visit(SqlDynamicParam param) { - return (SqlNode) param.clone(); + return SqlNode.clone(param); } public SqlNode visit(SqlIntervalQualifier intervalQualifier) { - return (SqlNode) intervalQualifier.clone(); + return SqlNode.clone(intervalQualifier); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/util/NlsString.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/util/NlsString.java b/core/src/main/java/org/apache/calcite/util/NlsString.java index eee7884..fbb9386 100644 --- a/core/src/main/java/org/apache/calcite/util/NlsString.java +++ b/core/src/main/java/org/apache/calcite/util/NlsString.java @@ -36,7 +36,7 @@ import static org.apache.calcite.util.Static.RESOURCE; * A string, optionally with {@link Charset character set} and * {@link SqlCollation}. It is immutable. */ -public class NlsString implements Comparable<NlsString> { +public class NlsString implements Comparable<NlsString>, Cloneable { //~ Instance fields -------------------------------------------------------- private final String charsetName; @@ -47,7 +47,7 @@ public class NlsString implements Comparable<NlsString> { //~ Constructors ----------------------------------------------------------- /** - * Creates a string in a specfied character set. + * Creates a string in a specified character set. * * @param value String constant, must not be null * @param charsetName Name of the character set, may be null @@ -91,7 +91,11 @@ public class NlsString implements Comparable<NlsString> { //~ Methods ---------------------------------------------------------------- public Object clone() { - return new NlsString(value, charsetName, collation); + try { + return super.clone(); + } catch (CloneNotSupportedException e) { + throw new AssertionError(); + } } public int hashCode() { http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/main/java/org/apache/calcite/util/XmlOutput.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/util/XmlOutput.java b/core/src/main/java/org/apache/calcite/util/XmlOutput.java index ba1e5c8..e58cae0 100644 --- a/core/src/main/java/org/apache/calcite/util/XmlOutput.java +++ b/core/src/main/java/org/apache/calcite/util/XmlOutput.java @@ -18,10 +18,7 @@ package org.apache.calcite.util; import com.google.common.collect.Lists; -import java.io.IOException; -import java.io.LineNumberReader; import java.io.PrintWriter; -import java.io.StringReader; import java.io.Writer; import java.util.ArrayDeque; import java.util.ArrayList; @@ -397,19 +394,28 @@ public class XmlOutput { * Writes content. */ public void content(String content) { + // This method previously used a LineNumberReader, but that class is + // susceptible to a form of DoS attack. It uses lots of memory and CPU if a + // malicious client gives it input with very long lines. if (content != null) { indent++; - LineNumberReader - in = new LineNumberReader(new StringReader(content)); - try { - String line; - while ((line = in.readLine()) != null) { + final char[] chars = content.toCharArray(); + int prev = 0; + for (int i = 0; i < chars.length; i++) { + if (chars[i] == '\n' + || chars[i] == '\r' + && i + 1 < chars.length + && chars[i + 1] == '\n') { displayIndent(out, indent); - out.println(line); + out.println(content.substring(prev, i)); + if (chars[i] == '\r') { + ++i; + } + prev = i + 1; } - } catch (IOException ex) { - throw new AssertionError(ex); } + displayIndent(out, indent); + out.println(content.substring(prev, chars.length)); indent--; out.flush(); } http://git-wip-us.apache.org/repos/asf/calcite/blob/8441e79c/core/src/test/java/org/apache/calcite/util/UtilTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/util/UtilTest.java b/core/src/test/java/org/apache/calcite/util/UtilTest.java index 080ad4c..3711654 100644 --- a/core/src/test/java/org/apache/calcite/util/UtilTest.java +++ b/core/src/test/java/org/apache/calcite/util/UtilTest.java @@ -30,6 +30,7 @@ import org.apache.calcite.runtime.FlatLists; import org.apache.calcite.runtime.Resources; import org.apache.calcite.runtime.SqlFunctions; import org.apache.calcite.runtime.Utilities; +import org.apache.calcite.sql.SqlCollation; import org.apache.calcite.sql.SqlDialect; import org.apache.calcite.sql.util.SqlBuilder; import org.apache.calcite.sql.util.SqlString; @@ -2007,6 +2008,42 @@ public class UtilTest { assertThat(map.range("BAZ", true).size(), is(0)); assertThat(map.range("Baz", true).size(), is(1)); } + + @Test public void testNlsStringClone() { + final NlsString s = new NlsString("foo", "LATIN1", SqlCollation.IMPLICIT); + assertThat(s.toString(), is("_LATIN1'foo'")); + final Object s2 = s.clone(); + assertThat(s2, instanceOf(NlsString.class)); + assertThat(s2, not(sameInstance((Object) s))); + assertThat(s2.toString(), is(s.toString())); + } + + @Test public void testXmlOutput() { + final StringWriter w = new StringWriter(); + final XmlOutput o = new XmlOutput(w); + o.beginBeginTag("root"); + o.attribute("a1", "v1"); + o.attribute("a2", null); + o.endBeginTag("root"); + o.beginTag("someText", null); + o.content("line 1 followed by empty line\n" + + "\n" + + "line 3 with windows line ending\r\n" + + "line 4 with no ending"); + o.endTag("someText"); + o.endTag("root"); + final String s = w.toString(); + final String expected = "" + + "<root a1=\"v1\">\n" + + "\t<someText>\n" + + "\t\t\tline 1 followed by empty line\n" + + "\t\t\t\n" + + "\t\t\tline 3 with windows line ending\n" + + "\t\t\tline 4 with no ending\n" + + "\t</someText>\n" + + "</root>\n"; + assertThat(s, is(expected)); + } } // End UtilTest.java
