Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HHH-18033 Fix LimitHandler detect wrong statement end if sql contains quoted semicolon #8270

Merged
merged 1 commit into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected boolean renderOffsetRowsKeyword() {
};

private static final Pattern WITH_OPTION_PATTERN =
Pattern.compile("\\s+with\\s+(" + String.join("|", WITH_OPTIONS) + ")\\b|\\s*(;|$)");
Pattern.compile("\\s+with\\s+(" + String.join("|", WITH_OPTIONS) + ")\\b|\\s*;?\\s*$");

/**
* The offset/fetch clauses must come before
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public int convertToFirstRowValue(int zeroBasedFirstResult) {
}

private static final Pattern FOR_UPDATE_PATTERN =
compile("\\s+for\\s+update\\b|\\s+with\\s+lock\\b|\\s*(;|$)", CASE_INSENSITIVE);
compile("\\s+for\\s+update\\b|\\s+with\\s+lock\\b|\\s*;?\\s*$", CASE_INSENSITIVE);

@Override
protected Pattern getForUpdatePattern() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.community.dialect;

import org.hibernate.community.dialect.pagination.IngresLimitHandler;
import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.orm.test.dialect.AbstractLimitHandlerTest;

/**
* @author Yanming Zhou
*/
public class IngresLimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return IngresLimitHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.community.dialect;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be in a test package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other tests already exist in this package, I think it's better to keep it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, well. That doesn't seem quite right to me.

@beikov is there a good reason the tests for the community dialects are in the same package as the dialect classes? Shouldn't they be somewhere like org.hibernate.community.dialect.test or whatever?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having tests in the same package as the Dialect under test allows to test package-private methods, which I suppose was the original reason for doing this. When we moved the classes to the community dialects module, we didn't change the package for tests apparently. Either is fine IMO, though if you want to change it, I think I'd prefer the package you are proposing.


import org.hibernate.community.dialect.pagination.RowsLimitHandler;
import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.orm.test.dialect.AbstractLimitHandlerTest;

/**
* @author Yanming Zhou
*/
public class RowsLimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return RowsLimitHandler.INSTANCE;
}

@Override
protected String getLimitClause() {
return " rows ?";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ public abstract class AbstractLimitHandler implements LimitHandler {
compile( "^\\s*select(\\s+(distinct|all))?\\b", CASE_INSENSITIVE );

private static final Pattern END_PATTERN =
compile("\\s*(;|$)", CASE_INSENSITIVE);
compile("\\s*;?\\s*$", CASE_INSENSITIVE);

private static final Pattern FOR_UPDATE_PATTERN =
compile("\\s+for\\s+update\\b|\\s*(;|$)", CASE_INSENSITIVE);
compile("\\s+for\\s+update\\b|\\s*;?\\s*$", CASE_INSENSITIVE);


@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public DerbyLimitHandler(boolean variableLimit) {
}

private static final Pattern FOR_UPDATE_WITH_LOCK_PATTERN =
Pattern.compile("\\s+for\\s+(update|read|fetch)\\b|\\s+with\\s+(rr|rs|cs|ur)\\b|\\s*(;|$)");
Pattern.compile("\\s+for\\s+(update|read|fetch)\\b|\\s+with\\s+(rr|rs|cs|ur)\\b|\\s*;?\\s*$");

/**
* The offset/fetch clauses must come before the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ protected String offsetOnlyClause() {
}

private static final Pattern FOR_UPDATE_PATTERN =
compile("\\s+for\\s+update\\b|\\s+lock\\s+in\\s+shared\\s+mode\\b|\\s*(;|$)", CASE_INSENSITIVE);
compile("\\s+for\\s+update\\b|\\s+lock\\s+in\\s+shared\\s+mode\\b|\\s*;?\\s*$", CASE_INSENSITIVE);

@Override
protected Pattern getForUpdatePattern() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ else if ( hasFirstRow ) {
offsetFetchString = " fetch first ? rows only";
}

return sql + offsetFetchString;
return insertAtEnd(offsetFetchString, sql);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.LimitHandler;
import org.hibernate.dialect.pagination.OffsetFetchLimitHandler;
import org.hibernate.query.spi.Limit;
import org.hibernate.query.spi.QueryOptions;
import org.junit.jupiter.api.Test;

import static org.hibernate.dialect.pagination.AbstractLimitHandler.hasFirstRow;
import static org.hibernate.dialect.pagination.AbstractLimitHandler.hasMaxRows;
import static org.junit.jupiter.api.Assertions.assertEquals;

/**
* @author Yanming Zhou
*/
public abstract class AbstractLimitHandlerTest {

@Test
public void testSqlWithSemicolonInsideQuotedString() {
String sql = "select * from Person p where p.name like ';'";
String expected = "select * from Person p where p.name like ';'" + getLimitClause();
assertGenerateExpectedSql(expected, sql);

sql = "select * from Person p where p.name like ';' ";
expected = "select * from Person p where p.name like ';'" + getLimitClause() + " ";
assertGenerateExpectedSql(expected, sql);
}

@Test
public void testSqlWithSemicolonInsideQuotedStringAndEndsWithSemicolon() {
String sql = "select * from Person p where p.name like ';';";
String expected = "select * from Person p where p.name like ';'" + getLimitClause() + ";";
assertGenerateExpectedSql(expected, sql);

sql = "select * from Person p where p.name like ';' ; ";
expected = "select * from Person p where p.name like ';'" + getLimitClause() + " ; ";
assertGenerateExpectedSql(expected, sql);
}

protected void assertGenerateExpectedSql(String expected, String sql) {
assertEquals(expected, getLimitHandler().processSql(sql, getLimit(), QueryOptions.NONE));
}

protected abstract LimitHandler getLimitHandler();

protected Limit getLimit() {
return new Limit(0, 10);
}

protected String getLimitClause() {
LimitHandler handler = getLimitHandler();
if (handler instanceof OffsetFetchLimitHandler) {
OffsetFetchLimitHandler oflh = (OffsetFetchLimitHandler) handler;
Limit limit = getLimit();
if (hasFirstRow(limit) && hasMaxRows(limit)) {
return " offset " + (oflh.supportsVariableLimit() ? "?" : String.valueOf(limit.getFirstRow()))
+ " rows fetch next " + (oflh.supportsVariableLimit() ? "?" : String.valueOf(limit.getMaxRows())) + " rows only";
}
else if (hasFirstRow(limit)) {
return " offset " + (oflh.supportsVariableLimit() ? "?" : String.valueOf(limit.getFirstRow())) + " rows";
} else {
return " fetch first " + (oflh.supportsVariableLimit() ? "?" : String.valueOf(limit.getMaxRows())) + " rows only";
}
}
return " limit ?";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.dialect.pagination.DB2LimitHandler;

/**
* @author Yanming Zhou
*/
public class DB2LimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return DB2LimitHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.dialect.pagination.DerbyLimitHandler;

/**
* @author Yanming Zhou
*/
public class DerbyLimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return DerbyLimitHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.dialect.pagination.LimitLimitHandler;

/**
* @author Yanming Zhou
*/
public class LimitLimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return LimitLimitHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.AbstractLimitHandler;
import org.hibernate.dialect.pagination.OffsetFetchLimitHandler;

/**
* @author Yanming Zhou
*/
public class OffsetFetchLimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return OffsetFetchLimitHandler.INSTANCE;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,57 +6,69 @@
*/
package org.hibernate.orm.test.dialect;

import org.hibernate.dialect.pagination.AbstractLimitHandler;

import org.hibernate.dialect.pagination.Oracle12LimitHandler;
import org.hibernate.query.spi.Limit;
import org.hibernate.query.spi.QueryOptions;

import org.hibernate.testing.TestForIssue;

import org.junit.jupiter.api.Test;

import static org.junit.Assert.assertEquals;
import static org.hibernate.dialect.pagination.AbstractLimitHandler.hasFirstRow;
import static org.hibernate.dialect.pagination.AbstractLimitHandler.hasMaxRows;

@TestForIssue( jiraKey = "HHH-14649")
public class Oracle12LimitHandlerTest {
public class Oracle12LimitHandlerTest extends AbstractLimitHandlerTest {

@Override
protected AbstractLimitHandler getLimitHandler() {
return Oracle12LimitHandler.INSTANCE;
}

@Override
protected String getLimitClause() {
Limit limit = getLimit();
if ( hasFirstRow(limit) && hasMaxRows(limit) ) {
return " offset ? rows fetch next ? rows only";
}
else if ( hasFirstRow(limit) ) {
return " offset ? rows";
}
else {
return " fetch first ? rows only";
}
}

@Test
public void testSqlWithSpace() {
final String sql = "select p.name from Person p where p.id = 1 for update";
final String expected = "select * from (select p.name from Person p where p.id = 1) where rownum<=? for update";

final String processedSql = Oracle12LimitHandler.INSTANCE.processSql( sql, new Limit( 0, 5 ), QueryOptions.NONE );

assertEquals( expected, processedSql );
assertGenerateExpectedSql(expected, sql);
}

@Test
public void testSqlWithSpaceInsideQuotedString() {
final String sql = "select p.name from Person p where p.name = ' this is a string with spaces ' for update";
final String expected = "select * from (select p.name from Person p where p.name = ' this is a string with spaces ') where rownum<=? for update";

final String processedSql = Oracle12LimitHandler.INSTANCE.processSql( sql, new Limit( 0, 5 ), QueryOptions.NONE );

assertEquals( expected, processedSql );
assertGenerateExpectedSql(expected, sql);
}

@Test
public void testSqlWithForUpdateInsideQuotedString() {
final String sql = "select a.prop from A a where a.name = 'this is for update '";
final String expected = "select a.prop from A a where a.name = 'this is for update ' fetch first ? rows only";

final String processedSql = Oracle12LimitHandler.INSTANCE.processSql( sql, new Limit( 0, 5 ), QueryOptions.NONE );

assertEquals( expected, processedSql );
assertGenerateExpectedSql(expected, sql);
}

@Test
public void testSqlWithForUpdateInsideAndOutsideQuotedStringA() {
final String sql = "select a.prop from A a where a.name = 'this is for update ' for update";
final String expected = "select * from (select a.prop from A a where a.name = 'this is for update ') where rownum<=? for update";

final String processedSql = Oracle12LimitHandler.INSTANCE.processSql( sql, new Limit( 0, 5 ), QueryOptions.NONE );

assertEquals( expected, processedSql );
assertGenerateExpectedSql(expected, sql);
}

}