Skip to content

Commit

Permalink
Revert "Fix for JPQL queries where identifier clashes with function (e…
Browse files Browse the repository at this point in the history 
…clipse-ee4j#2218)"

This reverts commit 5fe6040.
  • Loading branch information
@lukasj
lukasj committed Aug 14, 2024
1 parent 8c9a38a commit 6acfbd8
Show file tree
Hide file tree
Showing 17 changed files with 89 additions and 343 deletions.
62 changes: 10 additions & 52 deletions .../eclipse/persistence/testing/tests/jpa/jpql/advanced/JUnitJPQLJakartaDataNoAliasTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,19 @@
import junit.framework.Test;
import junit.framework.TestSuite;
import org.eclipse.persistence.queries.ReadObjectQuery;
import org.eclipse.persistence.sessions.server.ServerSession;
import org.eclipse.persistence.testing.framework.jpa.junit.JUnitTestCase;
import org.eclipse.persistence.testing.models.jpa.advanced.AdvancedTableCreator;
import org.eclipse.persistence.testing.models.jpa.advanced.EmployeePopulator;
import org.eclipse.persistence.testing.models.jpa.advanced.Room;
import org.eclipse.persistence.testing.models.jpa.datatypes.DataTypesTableCreator;
import org.eclipse.persistence.testing.models.jpa.datatypes.WrapperTypes;
import org.eclipse.persistence.testing.tests.jpa.jpql.JUnitDomainObjectComparer;
import org.junit.Assert;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.List;
import java.util.stream.Stream;
import org.eclipse.persistence.sessions.server.ServerSession;
import org.eclipse.persistence.testing.models.jpa.advanced.AdvancedTableCreator;
import org.eclipse.persistence.testing.models.jpa.advanced.Room;

/**
* <p>
Expand All @@ -53,14 +52,14 @@ public class JUnitJPQLJakartaDataNoAliasTest extends JUnitTestCase {

private static final String STRING_DATA = "A String";
private static final String STRING_DATA_LIKE_EXPRESSION = "A%"; // should match STRING_DATA
private static final Room[] ROOMS = new Room[]{
null, // Skip array index 0
aRoom(1, 1, 1, 1),
aRoom(2, 1, 1, 1),
aRoom(3, 1, 1, 1),
aRoom(4, 1, 1, 1)
private static final Room[] ROOMS = new Room[] {
null, // Skip array index 0
aRoom(1, 1, 1, 1),
aRoom(2, 1, 1, 1),
aRoom(3, 1, 1, 1),
aRoom(4, 1, 1, 1)
};
private static final long ROOMS_COUNT = ROOMS.length - 1; // we ignore the first one with index 0
private static final long ROOMS_COUNT = ROOMS.length -1; // we ignore the first one with index 0

private static int wrapperId;

Expand Down Expand Up @@ -102,11 +101,7 @@ public static Test suite() {
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testNoAliasFromWhereAndUPPER"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testGeneratedSelectNoAliasFromWhere"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testGeneratedSelect"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testUpdateQueryLengthInAssignmentAndExpression"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryLengthInAssignmentAndExpression"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testUpdateImplicitVariableInArithmeticExpression"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testDeleteQueryLengthInExpressionOnLeft"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testDeleteQueryLengthInExpressionOnRight"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("tesUpdateQueryWithThisVariable"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInPathExpressionUpdate"));
suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInPathExpressionDelete"));
Expand Down Expand Up @@ -265,25 +260,6 @@ public void testGeneratedSelectNoAliasFromWhere() {
Assert.assertTrue("GeneratedSelectNoAliasFromWhere Test Failed", comparer.compareObjects(wrapperTypes, tlWrapperTypes));
}

public void testUpdateQueryLengthInAssignmentAndExpression() {
resetRooms();
long numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
"UPDATE Room SET length = length + 1").executeUpdate());
assertTrue("All rooms should be updated", numberOfChanges == ROOMS_COUNT);

long numberOfRoomsWithLengthChanged = getAllRooms()
.filter(room -> room.getLength() == 2)
.count();
assertTrue("All rooms should have increased length", numberOfRoomsWithLengthChanged == ROOMS_COUNT);
}

public void testSelectQueryLengthInAssignmentAndExpression() {
resetRooms();
List<Room> roomsWithIdOne = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
"SELECT this FROM Room WHERE id + length = length + 1", Room.class).getResultList());
assertTrue("Number of rooms with ID = 1", roomsWithIdOne.size() == 1);
}

public void testUpdateImplicitVariableInArithmeticExpression() {
resetRooms();
int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
Expand All @@ -296,24 +272,6 @@ public void testUpdateImplicitVariableInArithmeticExpression() {
assertTrue("Room ID = 1 has width of ", roomWidth == 5);
}

public void testDeleteQueryLengthInExpressionOnLeft() {
resetRooms();
assertTrue("Number of remaining rooms", getAllRooms().count() == ROOMS_COUNT);
int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
"DELETE FROM Room WHERE length = id - 1").executeUpdate());
long allRoomsCount = getAllRooms().count();
assertTrue("Number of rooms with ID = 1 deleted", numberOfChanges == 1);
assertTrue("Number of remaining rooms", allRoomsCount == ROOMS_COUNT - 1);
}

public void testDeleteQueryLengthInExpressionOnRight() {
resetRooms();
int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
"DELETE FROM Room WHERE id = length + 1").executeUpdate());
assertTrue("Number of rooms with ID = 1 deleted", numberOfChanges == 1);
assertTrue("Number of remaining rooms", getAllRooms().count() == ROOMS_COUNT - 1);
}

public void tesUpdateQueryWithThisVariable() {
resetRooms();
long numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
Expand Down
2 changes: 0 additions & 2 deletions ...lipse.persistence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/WordParser.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/*
* Copyright (c) 2006, 2022 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2022 IBM Corporation. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand Down Expand Up @@ -257,7 +256,6 @@ public int length() {
*/
public void moveBackward(CharSequence word) {
cursor -= word.length();
wordEndPosition();
}

/**
Expand Down
16 changes: 0 additions & 16 deletions ...src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractEncapsulatedExpression.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/*
* Copyright (c) 2006, 2023 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand Down Expand Up @@ -307,19 +306,4 @@ else if (hasSpaceAfterIdentifier) {
* like content assist
*/
protected abstract void toParsedTextEncapsulatedExpression(StringBuilder writer, boolean actual);

/**
* Should be reverted if Jakarta Data mode is enabled and the expression doesn't contain parenthesis.
* For example, the query {@code delete from Box where length = 1} contains function name {@code length}.
* The function {@code length} expects parameters but there are not parameters in the query.
* Then {@code length} is not a function but a state field with an implicit identification variable this, ie.
* the query is equivalent to {@code delete from Box where this.length = 1}.
*
* @return Should be reverted and parsed again as a different expression or should be accepted as parsed expression
*/
@Override
protected boolean isInvalid() {
return getRoot().isJakartaData() && !hasLeftParenthesis();
}

}
40 changes: 0 additions & 40 deletions ...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractExpression.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/*
* Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand All @@ -16,7 +15,6 @@
//
package org.eclipse.persistence.jpa.jpql.parser;


import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.AccessController;
Expand Down Expand Up @@ -518,22 +516,6 @@ public final JPQLExpression getRoot() {
return (parent == null) ? (JPQLExpression) this : parent.getRoot();
}

/**
* Returns closest nested expression that encapsulates this expression,
* or the root expression if not inside a nested expression.
*
* @return Parent expression
*/
public final ParentExpression getParentExpression() {
if (this instanceof ParentExpression parentExpression) {
return parentExpression;
} else if (parent == null) {
return null;
} else {
return parent.getParentExpression();
}
}

/**
* Returns the encapsulated text of this {@link AbstractExpression}, which can be used in various
* ways, it can be a keyword, a literal, etc.
Expand Down Expand Up @@ -787,9 +769,6 @@ tolerant && isParsingComplete(wordParser, word, expression)) {
if (factory != null) {
child = factory.buildExpression(this, wordParser, word, queryBNF, expression, tolerant);

// if an invalid expression came from the factory, ignore it and try fallback
child = revertExpressionIfInvalid(child, wordParser, word);

if (child != null) {

// The new expression is a child of the previous expression,
Expand Down Expand Up @@ -1009,14 +988,6 @@ else if (currentInfo != null) {
);
}

static AbstractExpression revertExpressionIfInvalid(AbstractExpression expression, WordParser wordParser, String word) {
if (expression != null && expression.isInvalid()) {
wordParser.moveBackward(word);
return null;
}
return expression;
}

/**
* Right away parses the text by retrieving the {@link ExpressionFactory} for the first word that
* is extracted from {@link WordParser} at the current location.
Expand Down Expand Up @@ -1162,17 +1133,6 @@ public String toParsedText() {
*/
protected abstract void toParsedText(StringBuilder writer, boolean actual);

/**
* Whether this expression is not valid and should be discarded. If it returns true,
* the parser will be reverted to the state before this expression was parsed
* and it will attempt to parse a different expression.
*
* @return True if this expression is invalid and should be discarded, otherwise false. By default returns false, should be overriden if expression should be reverted.
*/
protected boolean isInvalid() {
return false;
}

@Override
public final String toString() {
// toString() should only be called during debugging, thus the cached parsed text
Expand Down
3 changes: 1 addition & 2 deletions ...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractFromClause.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/*
* Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand Down Expand Up @@ -344,7 +343,7 @@ else if (hierarchicalQueryClause == null) {
identificationVariableDeclaration.hasRangeVariableDeclaration() && identificationVariableDeclaration.getRangeVariableDeclaration() instanceof RangeVariableDeclaration rangeVariableDeclaration &&
rangeVariableDeclaration.hasIdentificationVariable() && rangeVariableDeclaration.getIdentificationVariable() instanceof IdentificationVariable identificationVariable &&
Expression.THIS.equals(identificationVariable.getText())) {
this.getParentExpression().setGenerateThisPrefix(true);
this.getRoot().setGenerateThisPrefix(true);
}
}

Expand Down
100 changes: 45 additions & 55 deletions ...c/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractLiteralExpressionFactory.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/*
* Copyright (c) 2006, 2021 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0,
Expand Down Expand Up @@ -65,64 +64,67 @@ protected AbstractExpression buildExpression(AbstractExpression parent,
WordParser wordParser,
String word,
JPQLQueryBNF queryBNF,
final AbstractExpression expression,
AbstractExpression expression,
boolean tolerant) {

switch (wordParser.getWordType()) {

case NUMERIC_LITERAL: {
NumericLiteral numericLiteral = new NumericLiteral(parent, word);
numericLiteral.parse(wordParser, tolerant);
return numericLiteral;
expression = new NumericLiteral(parent, word);
expression.parse(wordParser, tolerant);
return expression;
}

case STRING_LITERAL: {
StringLiteral stringLiteral = new StringLiteral(parent, word);
stringLiteral.parse(wordParser, tolerant);
return stringLiteral;
expression = new StringLiteral(parent, word);
expression.parse(wordParser, tolerant);
return expression;
}

case INPUT_PARAMETER: {
InputParameter inputParameter = new InputParameter(parent, word);
inputParameter.parse(wordParser, tolerant);
return inputParameter;
expression = new InputParameter(parent, word);
expression.parse(wordParser, tolerant);
return expression;
}
}

default: {
// Path expression
if (word.indexOf(AbstractExpression.DOT) > -1) {
char character = word.charAt(0);
AbstractPathExpression pathExpression;

if ((expression != null) && (character == AbstractExpression.DOT)) {
if (isCollection()) {
pathExpression = new CollectionValuedPathExpression(parent, expression, word);
} else {
pathExpression = new StateFieldPathExpression(parent, expression, word);
}
} else {
if (isCollection()) {
pathExpression = new CollectionValuedPathExpression(parent, word);
} else {
pathExpression = new StateFieldPathExpression(parent, word);
}
}

pathExpression.parse(wordParser, tolerant);
return pathExpression;
// Path expression
if (word.indexOf(AbstractExpression.DOT) > -1) {
char character = word.charAt(0);

if ((expression != null) && (character == AbstractExpression.DOT)) {
if (isCollection()) {
expression = new CollectionValuedPathExpression(parent, expression, word);
}
else {
expression = new StateFieldPathExpression(parent, expression, word);
}
}
else {
if (isCollection()) {
expression = new CollectionValuedPathExpression(parent, word);
}
else {
expression = new StateFieldPathExpression(parent, word);
}
}

// Checks for invalid JPQL queries
expression.parse(wordParser, tolerant);
return expression;
}

if (tolerant && getExpressionRegistry().isIdentifier(word)) {
// Before creating the expression, check to make sure it's not a function: 'identifier('
AbstractExpression identifierExpression;
identifierExpression = getIdentifierExpression(parent, wordParser, word, queryBNF, expression, tolerant);
if (identifierExpression != null) {
return new BadExpression(parent, identifierExpression);
}
}
// Checks for invalid JPQL queries
ExpressionRegistry registry = getExpressionRegistry();

if (tolerant && registry.isIdentifier(word)) {
ExpressionFactory factory = registry.expressionFactoryForIdentifier(word);
// TODO: Before creating the expression, check to make sure it's not a function: 'identifier('
if (factory != null) {
expression = factory.buildExpression(parent, wordParser, word, queryBNF, expression, tolerant);

if (expression != null) {
return new BadExpression(parent, expression);
}
}
}

Expand All @@ -136,16 +138,4 @@ protected AbstractExpression buildExpression(AbstractExpression parent,
protected boolean isCollection() {
return false;
}

private AbstractExpression getIdentifierExpression(AbstractExpression parent, WordParser wordParser, String word, JPQLQueryBNF queryBNF, AbstractExpression expression, boolean tolerant) {
ExpressionFactory factory = getExpressionRegistry().expressionFactoryForIdentifier(word);
// TODO: Before creating the expression, check to make sure it's not a function: 'identifier('
if (factory != null) {
final AbstractExpression identifierExpression = factory.buildExpression(parent, wordParser, word, queryBNF, expression, tolerant);

// if an invalid expression came from the factory, e.g. a function without expected arguments, ignore it
return AbstractExpression.revertExpressionIfInvalid(identifierExpression, wordParser, word);
}
return null;
}
}
1 change: 0 additions & 1 deletion ...nce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/ExpressionFactory.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/*
* Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024 Contributors to the Eclipse Foundation. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
Expand Down
Loading

0 comments on commit 6acfbd8

Please sign in to comment.