From cf7fe157de372f37eca716daaa394ec5ed60b144 Mon Sep 17 00:00:00 2001 From: Andreas Reichel Date: Sat, 20 Apr 2024 19:58:41 +0700 Subject: [PATCH] fix: `FOR UPDATE` clause should come after the select body - fixes #1995 - disable faulty Oracle test `for_update08` Signed-off-by: Andreas Reichel --- .../statement/select/PlainSelect.java | 109 ++---------------- .../jsqlparser/statement/select/Select.java | 99 +++++++++++++++- .../util/deparser/AbstractDeParser.java | 4 +- .../util/deparser/SelectDeParser.java | 34 +++--- .../statement/select/ForUpdateTest.java | 38 ++++++ .../statement/select/SelectTest.java | 10 +- .../statement/select/SpecialOracleTest.java | 2 +- .../select/oracle-tests/for_update08.sql | 2 +- 8 files changed, 165 insertions(+), 133 deletions(-) create mode 100644 src/test/java/net/sf/jsqlparser/statement/select/ForUpdateTest.java diff --git a/src/main/java/net/sf/jsqlparser/statement/select/PlainSelect.java b/src/main/java/net/sf/jsqlparser/statement/select/PlainSelect.java index 72648eb8d..c2c61c7e2 100644 --- a/src/main/java/net/sf/jsqlparser/statement/select/PlainSelect.java +++ b/src/main/java/net/sf/jsqlparser/statement/select/PlainSelect.java @@ -9,7 +9,12 @@ */ package net.sf.jsqlparser.statement.select; -import static java.util.stream.Collectors.joining; +import net.sf.jsqlparser.expression.Alias; +import net.sf.jsqlparser.expression.Expression; +import net.sf.jsqlparser.expression.OracleHierarchicalExpression; +import net.sf.jsqlparser.expression.OracleHint; +import net.sf.jsqlparser.expression.WindowDefinition; +import net.sf.jsqlparser.schema.Table; import java.util.ArrayList; import java.util.Arrays; @@ -18,12 +23,8 @@ import java.util.Iterator; import java.util.List; import java.util.Optional; -import net.sf.jsqlparser.expression.Alias; -import net.sf.jsqlparser.expression.Expression; -import net.sf.jsqlparser.expression.OracleHierarchicalExpression; -import net.sf.jsqlparser.expression.OracleHint; -import net.sf.jsqlparser.expression.WindowDefinition; -import net.sf.jsqlparser.schema.Table; + +import static java.util.stream.Collectors.joining; @SuppressWarnings({"PMD.CyclomaticComplexity"}) public class PlainSelect extends Select { @@ -45,15 +46,10 @@ public class PlainSelect extends Select { private Top top; private OracleHierarchicalExpression oracleHierarchical = null; private OracleHint oracleHint = null; - private ForMode forMode = null; - private Table forUpdateTable = null; - private boolean skipLocked; - private Wait wait; private boolean mySqlSqlCalcFoundRows = false; private MySqlSqlCacheFlags mySqlCacheFlag = null; private String forXmlPath; private KSQLWindow ksqlWindow = null; - private boolean noWait = false; private boolean emitChanges = false; private List windowDefinitions; @@ -360,22 +356,6 @@ public void setOracleHierarchical(OracleHierarchicalExpression oracleHierarchica this.oracleHierarchical = oracleHierarchical; } - public ForMode getForMode() { - return forMode; - } - - public void setForMode(ForMode forMode) { - this.forMode = forMode; - } - - public Table getForUpdateTable() { - return forUpdateTable; - } - - public void setForUpdateTable(Table forUpdateTable) { - this.forUpdateTable = forUpdateTable; - } - public OracleHint getOracleHint() { return oracleHint; } @@ -384,24 +364,6 @@ public void setOracleHint(OracleHint oracleHint) { this.oracleHint = oracleHint; } - /** - * Sets the {@link Wait} for this SELECT - * - * @param wait the {@link Wait} for this SELECT - */ - public void setWait(final Wait wait) { - this.wait = wait; - } - - /** - * Returns the value of the {@link Wait} set for this SELECT - * - * @return the value of the {@link Wait} set for this SELECT - */ - public Wait getWait() { - return wait; - } - public String getForXmlPath() { return forXmlPath; } @@ -434,14 +396,6 @@ public void setWindowDefinitions(List windowDefinitions) { this.windowDefinitions = windowDefinitions; } - public boolean isSkipLocked() { - return skipLocked; - } - - public void setSkipLocked(boolean skipLocked) { - this.skipLocked = skipLocked; - } - @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.ExcessiveMethodLength", "PMD.NPathComplexity"}) public StringBuilder appendSelectBodyTo(StringBuilder builder) { @@ -538,25 +492,6 @@ public StringBuilder appendSelectBodyTo(StringBuilder builder) { if (emitChanges) { builder.append(" EMIT CHANGES"); } - if (getForMode() != null) { - builder.append(" FOR "); - builder.append(getForMode().getValue()); - - if (forUpdateTable != null) { - builder.append(" OF ").append(forUpdateTable); - } - - if (wait != null) { - // Wait's toString will do the formatting for us - builder.append(wait); - } - - if (isNoWait()) { - builder.append(" NOWAIT"); - } else if (isSkipLocked()) { - builder.append(" SKIP LOCKED"); - } - } } else { // without from if (where != null) { @@ -616,14 +551,6 @@ public MySqlSqlCacheFlags getMySqlSqlCacheFlag() { return this.mySqlCacheFlag; } - public void setNoWait(boolean noWait) { - this.noWait = noWait; - } - - public boolean isNoWait() { - return this.noWait; - } - public PlainSelect withDistinct(Distinct distinct) { this.setDistinct(distinct); return this; @@ -679,16 +606,6 @@ public PlainSelect withOracleSiblings(boolean oracleSiblings) { return this; } - public PlainSelect withForMode(ForMode forMode) { - this.setForMode(forMode); - return this; - } - - public PlainSelect withForUpdateTable(Table forUpdateTable) { - this.setForUpdateTable(forUpdateTable); - return this; - } - public PlainSelect withForXmlPath(String forXmlPath) { this.setForXmlPath(forXmlPath); return this; @@ -704,21 +621,11 @@ public PlainSelect withNoWait(boolean noWait) { return this; } - public PlainSelect withSkipLocked(boolean skipLocked) { - this.setSkipLocked(skipLocked); - return this; - } - public PlainSelect withHaving(Expression having) { this.setHaving(having); return this; } - public PlainSelect withWait(Wait wait) { - this.setWait(wait); - return this; - } - public PlainSelect addSelectItems(Collection> selectItems) { List> collection = Optional.ofNullable(getSelectItems()).orElseGet(ArrayList::new); diff --git a/src/main/java/net/sf/jsqlparser/statement/select/Select.java b/src/main/java/net/sf/jsqlparser/statement/select/Select.java index 2216fada9..6e2dbb660 100644 --- a/src/main/java/net/sf/jsqlparser/statement/select/Select.java +++ b/src/main/java/net/sf/jsqlparser/statement/select/Select.java @@ -12,6 +12,7 @@ import net.sf.jsqlparser.expression.Expression; import net.sf.jsqlparser.expression.ExpressionVisitor; import net.sf.jsqlparser.parser.ASTNodeAccessImpl; +import net.sf.jsqlparser.schema.Table; import net.sf.jsqlparser.statement.Statement; import net.sf.jsqlparser.statement.StatementVisitor; @@ -23,6 +24,7 @@ import java.util.Optional; public abstract class Select extends ASTNodeAccessImpl implements Statement, Expression { + protected Table forUpdateTable = null; List withItemsList; Limit limitBy; Limit limit; @@ -34,6 +36,10 @@ public abstract class Select extends ASTNodeAccessImpl implements Statement, Exp ForClause forClause = null; List orderByElements; + ForMode forMode = null; + private boolean skipLocked; + private Wait wait; + private boolean noWait = false; public static String orderByToString(List orderByElements) { return orderByToString(false, orderByElements); @@ -52,8 +58,8 @@ public static String getFormattedList(List list, String expression, boolean u boolean useBrackets) { String sql = getStringList(list, useComma, useBrackets); - if (sql.length() > 0) { - if (expression.length() > 0) { + if (!sql.isEmpty()) { + if (!expression.isEmpty()) { sql = " " + expression + " " + sql; } else { sql = " " + sql; @@ -152,6 +158,14 @@ public void setOracleSiblings(boolean oracleSiblings) { this.oracleSiblings = oracleSiblings; } + public void setNoWait(boolean noWait) { + this.noWait = noWait; + } + + public boolean isNoWait() { + return this.noWait; + } + public Select withOracleSiblings(boolean oracleSiblings) { this.setOracleSiblings(oracleSiblings); return this; @@ -255,6 +269,48 @@ public Select withIsolation(WithIsolation isolation) { return this; } + public ForMode getForMode() { + return this.forMode; + } + + public void setForMode(ForMode forMode) { + this.forMode = forMode; + } + + public Table getForUpdateTable() { + return this.forUpdateTable; + } + + public void setForUpdateTable(Table forUpdateTable) { + this.forUpdateTable = forUpdateTable; + } + + /** + * Sets the {@link Wait} for this SELECT + * + * @param wait the {@link Wait} for this SELECT + */ + public void setWait(final Wait wait) { + this.wait = wait; + } + + /** + * Returns the value of the {@link Wait} set for this SELECT + * + * @return the value of the {@link Wait} set for this SELECT + */ + public Wait getWait() { + return wait; + } + + public boolean isSkipLocked() { + return skipLocked; + } + + public void setSkipLocked(boolean skipLocked) { + this.skipLocked = skipLocked; + } + public abstract StringBuilder appendSelectBodyTo(StringBuilder builder); @SuppressWarnings({"PMD.CyclomaticComplexity"}) @@ -294,6 +350,25 @@ public StringBuilder appendTo(StringBuilder builder) { if (isolation != null) { builder.append(isolation); } + if (forMode != null) { + builder.append(" FOR "); + builder.append(forMode.getValue()); + + if (getForUpdateTable() != null) { + builder.append(" OF ").append(forUpdateTable); + } + + if (wait != null) { + // Wait's toString will do the formatting for us + builder.append(wait); + } + + if (isNoWait()) { + builder.append(" NOWAIT"); + } else if (isSkipLocked()) { + builder.append(" SKIP LOCKED"); + } + } return builder; } @@ -334,4 +409,24 @@ public SetOperationList getSetOperationList() { public E as(Class type) { return type.cast(this); } + + public Select withForMode(ForMode forMode) { + this.setForMode(forMode); + return this; + } + + public Select withForUpdateTable(Table forUpdateTable) { + this.setForUpdateTable(forUpdateTable); + return this; + } + + public Select withSkipLocked(boolean skipLocked) { + this.setSkipLocked(skipLocked); + return this; + } + + public Select withWait(Wait wait) { + this.setWait(wait); + return this; + } } diff --git a/src/main/java/net/sf/jsqlparser/util/deparser/AbstractDeParser.java b/src/main/java/net/sf/jsqlparser/util/deparser/AbstractDeParser.java index 4a31a7ace..4ef4d047c 100644 --- a/src/main/java/net/sf/jsqlparser/util/deparser/AbstractDeParser.java +++ b/src/main/java/net/sf/jsqlparser/util/deparser/AbstractDeParser.java @@ -28,8 +28,8 @@ protected AbstractDeParser(StringBuilder buffer) { public static void deparseUpdateSets(List updateSets, StringBuilder buffer, ExpressionVisitor visitor) { - ExpressionListDeParser expressionListDeParser = - new ExpressionListDeParser(visitor, buffer); + ExpressionListDeParser expressionListDeParser = + new ExpressionListDeParser<>(visitor, buffer); int j = 0; if (updateSets != null) { for (UpdateSet updateSet : updateSets) { diff --git a/src/main/java/net/sf/jsqlparser/util/deparser/SelectDeParser.java b/src/main/java/net/sf/jsqlparser/util/deparser/SelectDeParser.java index 3728fdee4..fba4cdc76 100644 --- a/src/main/java/net/sf/jsqlparser/util/deparser/SelectDeParser.java +++ b/src/main/java/net/sf/jsqlparser/util/deparser/SelectDeParser.java @@ -276,23 +276,6 @@ public void visit(PlainSelect plainSelect) { buffer.append(plainSelect.getWindowDefinitions().stream() .map(WindowDefinition::toString).collect(joining(", "))); } - if (plainSelect.getForMode() != null) { - buffer.append(" FOR "); - buffer.append(plainSelect.getForMode().getValue()); - - if (plainSelect.getForUpdateTable() != null) { - buffer.append(" OF ").append(plainSelect.getForUpdateTable()); - } - if (plainSelect.getWait() != null) { - // wait's toString will do the formatting for us - buffer.append(plainSelect.getWait()); - } - if (plainSelect.isNoWait()) { - buffer.append(" NOWAIT"); - } else if (plainSelect.isSkipLocked()) { - buffer.append(" SKIP LOCKED"); - } - } if (plainSelect.getForClause() != null) { plainSelect.getForClause().appendTo(buffer); } @@ -319,6 +302,23 @@ public void visit(PlainSelect plainSelect) { if (plainSelect.getIsolation() != null) { buffer.append(plainSelect.getIsolation().toString()); } + if (plainSelect.getForMode() != null) { + buffer.append(" FOR "); + buffer.append(plainSelect.getForMode().getValue()); + + if (plainSelect.getForUpdateTable() != null) { + buffer.append(" OF ").append(plainSelect.getForUpdateTable()); + } + if (plainSelect.getWait() != null) { + // wait's toString will do the formatting for us + buffer.append(plainSelect.getWait()); + } + if (plainSelect.isNoWait()) { + buffer.append(" NOWAIT"); + } else if (plainSelect.isSkipLocked()) { + buffer.append(" SKIP LOCKED"); + } + } if (plainSelect.getOptimizeFor() != null) { deparseOptimizeFor(plainSelect.getOptimizeFor()); } diff --git a/src/test/java/net/sf/jsqlparser/statement/select/ForUpdateTest.java b/src/test/java/net/sf/jsqlparser/statement/select/ForUpdateTest.java new file mode 100644 index 000000000..8643fb1fd --- /dev/null +++ b/src/test/java/net/sf/jsqlparser/statement/select/ForUpdateTest.java @@ -0,0 +1,38 @@ +package net.sf.jsqlparser.statement.select; + +import net.sf.jsqlparser.JSQLParserException; +import net.sf.jsqlparser.test.TestUtils; +import org.junit.jupiter.api.Test; + +public class ForUpdateTest { + + @Test + void testOracleForUpdate() throws JSQLParserException { + String sqlStr = "SELECT e.employee_id, e.salary, e.commission_pct\n" + + " FROM employees e, departments d\n" + + " WHERE job_id = 'SA_REP'\n" + + " AND e.department_id = d.department_id\n" + + " AND location_id = 2500\n" + + " ORDER BY e.employee_id\n" + + " FOR UPDATE;\n"; + + TestUtils.assertSqlCanBeParsedAndDeparsed(sqlStr, true); + + sqlStr = "SELECT e.employee_id, e.salary, e.commission_pct\n" + + " FROM employees e JOIN departments d\n" + + " USING (department_id)\n" + + " WHERE job_id = 'SA_REP'\n" + + " AND location_id = 2500\n" + + " ORDER BY e.employee_id\n" + + " FOR UPDATE OF e.salary;"; + + TestUtils.assertSqlCanBeParsedAndDeparsed(sqlStr, true); + } + + @Test + void testMySqlIssue1995() throws JSQLParserException { + String sqlStr = "select * from t_demo where a = 1 order by b asc limit 1 for update"; + + TestUtils.assertSqlCanBeParsedAndDeparsed(sqlStr, true); + } +} diff --git a/src/test/java/net/sf/jsqlparser/statement/select/SelectTest.java b/src/test/java/net/sf/jsqlparser/statement/select/SelectTest.java index 01e31b959..8e34ac422 100644 --- a/src/test/java/net/sf/jsqlparser/statement/select/SelectTest.java +++ b/src/test/java/net/sf/jsqlparser/statement/select/SelectTest.java @@ -4818,7 +4818,7 @@ public void testIssue1878ViaJava() throws JSQLParserException { // Step 1: generate the Java Object Hierarchy for Table table = new Table().withName("MY_TABLE1"); - PlainSelect select = new PlainSelect().addSelectItem(new AllColumns()) + Select select = new PlainSelect().addSelectItem(new AllColumns()) .withFromItem(table).withForMode(ForMode.KEY_SHARE).withForMode(ForMode.SHARE); Assertions.assertEquals(expectedSQLStr, select.toString()); @@ -5639,14 +5639,6 @@ void testSetOperationListWithBracketsIssue1737() throws JSQLParserException { assertSqlCanBeParsedAndDeparsed(sqlStr, true); } - @Test - void subJoinTest() throws JSQLParserException { - String sqlStr = - "select su.d\n" + "from sku su\n" + "for update of su.up\n" + "order by su.d"; - - TestUtils.assertSqlCanBeParsedAndDeparsed(sqlStr, true); - } - @Test void testNestedWithItems() throws JSQLParserException { String sqlStr = diff --git a/src/test/java/net/sf/jsqlparser/statement/select/SpecialOracleTest.java b/src/test/java/net/sf/jsqlparser/statement/select/SpecialOracleTest.java index 6334afe07..f4cb8637d 100644 --- a/src/test/java/net/sf/jsqlparser/statement/select/SpecialOracleTest.java +++ b/src/test/java/net/sf/jsqlparser/statement/select/SpecialOracleTest.java @@ -84,7 +84,7 @@ public class SpecialOracleTest { "datetime02.sql", "datetime04.sql", "datetime05.sql", "datetime06.sql", "dblink01.sql", "for_update01.sql", "for_update02.sql", "for_update03.sql", "function04.sql", "function05.sql", "for_update04.sql", "for_update05.sql", "for_update06.sql", - "for_update08.sql", "function01.sql", "function02.sql", "function03.sql", + "function01.sql", "function02.sql", "function03.sql", "function06.sql", "groupby01.sql", "groupby02.sql", "groupby03.sql", "groupby04.sql", "groupby05.sql", "groupby06.sql", diff --git a/src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/for_update08.sql b/src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/for_update08.sql index 008209eef..ff5b7f79b 100644 --- a/src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/for_update08.sql +++ b/src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/for_update08.sql @@ -13,4 +13,4 @@ where (nvl(su.up,'n')='n' and su.ttype=:b0) for update of su.up order by su.d ---@SUCCESSFULLY_PARSED_AND_DEPARSED first on Aug 3, 2021, 7:20:08 AM \ No newline at end of file +--@FAILURE: select su.ttype,su.cid,su.s_id,sessiontimezone from sku su where(nvl(su.up,'n')='n' and su.ttype=:b0)order by su.d for update of su.up recorded first on 20 Apr 2024, 15:59:32 \ No newline at end of file