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

Fix for JPQL queries where identifier clashes with function #2218

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 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 @@ -55,6 +56,9 @@ public class Pokemon {

private String name;

// to test queries where entity field clashes with a function name in QueryTest
private int length = 0;

@ManyToOne
@JoinColumn(name = "TRAINER_ID")
private Trainer trainer;
Expand Down Expand Up @@ -129,6 +133,14 @@ public void setTrainer(Trainer trainer) {
this.trainer = trainer;
}

public int getLength() {
return length;
}

public void setLength(int length) {
this.length = length;
}

@Override
public boolean equals(Object obj) {
if (obj == null || obj.getClass() != this.getClass()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 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 All @@ -11,10 +12,15 @@
*/
package org.eclipse.persistence.testing.tests.jpa.persistence32;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
Copy link
Member

Choose a reason for hiding this comment

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

There is no single use of hamcrest in the repo and the project itself is happy without it, so do not add it, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Hamcrest assertions. The hamcrest dependency is transitively brought to the project by junit. Maybe it would be good to exclude hamcrest from the project. I wouldn't use it if I saw it's excluded.

Copy link
Member

Choose a reason for hiding this comment

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

inability to avoid hamcrest dependency is known junit 3/4 issue - see ie junit-team/junit4#1145 and/or junit-team/junit4#1741 if you're interested in details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

I did it in #2226

Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

It looks not so easy in EclipseLink project. There are test failures in #2226
mvn clean verify -pl :org.eclipse.persistence.jpa.jpql throws exception in org.eclipse.persistence.jpa.tests.jpql.AllHermesTests . I think it's releated with @RunWith, org.junit.runner.RunWith.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like hamcrest is used somewhere. Maybe by JUnit itself. Here seems to be the same problem: #261. At that time, it looks like it was necessary to add hamcrest on the classpath in Ant configuration. But the problem was the same - something needed Hamcrest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, JUnit 4 uses hamcrest in several places: https://github.com/search?q=repo%3Ajunit-team%2Fjunit4+hamcrest+language%3AJava&type=code&l=Java. So it's not possible to completely remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A solution could be to make hamcrest a runtime dependency. JUnit would work but it couldn't be used in compiled code.


import java.util.List;

import jakarta.persistence.NoResultException;
import jakarta.persistence.NonUniqueResultException;
import java.util.stream.Stream;
import junit.framework.Test;
import org.eclipse.persistence.testing.models.jpa.persistence32.Pokemon;
import org.junit.Assert;
Expand All @@ -34,6 +40,8 @@ public class QueryTest extends AbstractPokemon {
new Pokemon(4, "Caterpie", List.of(TYPES[7]))
};

static final long POKEMONS_COUNT = POKEMONS.length -1; // we ignore the first one with index 0

public static Test suite() {
return suite(
"QueryTest",
Expand All @@ -42,7 +50,12 @@ public static Test suite() {
new QueryTest("testGetSingleResultWithMultipleResults"),
new QueryTest("testGetSingleResultOrNullWithEmptyResult"),
new QueryTest("testGetSingleResultOrNullWithSingleResult"),
new QueryTest("testGetSingleResultOrNullWithMultipleResults")
new QueryTest("testGetSingleResultOrNullWithMultipleResults"),
new QueryTest("testUpdateQueryLengthInAssignmentAndExpression"),
new QueryTest("tesUpdateQueryWithThisVariable"),
new QueryTest("testSelectQueryLengthInAssignmentAndExpression"),
new QueryTest("testDeleteQueryLengthInExpressionOnLeft"),
new QueryTest("testDeleteQueryLengthInExpressionOnRight")
);
}

Expand All @@ -54,11 +67,12 @@ public QueryTest(String name) {
setPuName(getPersistenceUnitName());
}

// Initialize data
// Initialize data for each test
@Override
protected void suiteSetUp() {
super.suiteSetUp();
public void setUp() {
super.setUp();
emf.runInTransaction(em -> {
em.createQuery("DELETE FROM Pokemon").executeUpdate();
for (int i = 1; i < POKEMONS.length; i++) {
em.persist(POKEMONS[i]);
}
Expand Down Expand Up @@ -108,4 +122,49 @@ public void testGetSingleResultOrNullWithMultipleResults() {
"SELECT p FROM Pokemon p ", Pokemon.class).getSingleResultOrNull()));
}

public void testUpdateQueryLengthInAssignmentAndExpression() {
testUpdateAllPokemons("UPDATE Pokemon SET length = length + 1");
}

public void tesUpdateQueryWithThisVariable() {
testUpdateAllPokemons("UPDATE Pokemon SET length = this.length + 1");
}

private void testUpdateAllPokemons(String query) {
long numberOfChanges = emf.callInTransaction(em -> em.createQuery(query).executeUpdate());
assertThat("All pokemons should be updated", numberOfChanges, is(equalTo(POKEMONS_COUNT)));

long numberOfPokemonsWithLengthChanged = getAllPokemons()
.filter(pokemon -> pokemon.getLength() == 1)
.count();
assertThat("All pokemons should have increased length", numberOfPokemonsWithLengthChanged, is(equalTo(POKEMONS_COUNT)));
}

public void testSelectQueryLengthInAssignmentAndExpression() {
List<Pokemon> pokemonsWithIdOne = emf.callInTransaction(em -> em.createQuery(
"SELECT this FROM Pokemon WHERE id + length = length + 1", Pokemon.class).getResultList());
assertThat("Number of pokemons with ID = 1", pokemonsWithIdOne.size(), is(equalTo(1)));
}

public void testDeleteQueryLengthInExpressionOnLeft() {
assertThat("Number of remaining pokemons", getAllPokemons().count(), is(equalTo(POKEMONS_COUNT)));
int numberOfChanges = emf.callInTransaction(em -> em.createQuery(
"DELETE FROM Pokemon WHERE length = id - 1").executeUpdate());
assertThat("Number of pokemons with ID = 1 deleted", numberOfChanges, is(equalTo(1)));
assertThat("Number of remaining pokemons", getAllPokemons().count(), is(equalTo(POKEMONS_COUNT - 1)));
}

public void testDeleteQueryLengthInExpressionOnRight() {
assertThat("Number of remaining pokemons", getAllPokemons().count(), is(equalTo(POKEMONS_COUNT)));
int numberOfChanges = emf.callInTransaction(em -> em.createQuery(
"DELETE FROM Pokemon WHERE id = length + 1").executeUpdate());
assertThat("Number of pokemons with ID = 1 deleted", numberOfChanges, is(equalTo(1)));
assertThat("Number of remaining pokemons", getAllPokemons().count(), is(equalTo(POKEMONS_COUNT - 1)));
}

private Stream<Pokemon> getAllPokemons() {
return emf.callInTransaction(em -> em.createQuery(
"SELECT p FROM Pokemon p", Pokemon.class).getResultStream());
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* Copyright (c) 2006, 2024 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 @@ -1791,35 +1792,6 @@ protected final boolean isNewerThanOrEqual(JPAVersion version) {
return getJPAVersion().isNewerThanOrEqual(version);
}

/**
* Determines whether the given sequence of characters is a numeric literal or not. There are
* two types of numeric literal that is supported:
* <ul>
* <li>Decimal literal</li>
* <li>Hexadecimal literal</li>
* </ul>
*
* @param text The sequence of characters to validate
* @return <code>true</code> if the given sequence of characters is a valid numeric literal;
* <code>false</code> otherwise
*/
protected boolean isNumericLiteral(String text) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't delete this method, just moved it to the NumericLiteral class and renamed to hasValidValue, so that it can be used also in the parser, not only in the validator.

Copy link
Member

Choose a reason for hiding this comment

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

So you did an API change in a public API. Is it required to fix the bug? If so, re-do it in a backward compatible way, with appropriate deprecation step eventually, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a different way to fix the bug, so I reverted removing this method.


// The ending 'l' or 'L' for a long number has to be removed, Java will not parse it
if (text.endsWith("l") || text.endsWith("L")) {
text = text.substring(0, text.length() - 1);
}

// Simply try to parse it as a double number (integer and hexadecimal are handled as well)
try {
Double.parseDouble(text);
return true;
}
catch (Exception e2) {
return false;
}
}

/**
* Determines whether the JPA version for which the JPQL grammar was defined represents a version
* that is older than the given version.
Expand Down Expand Up @@ -4394,15 +4366,14 @@ public void visit(NullIfExpression expression) {
@Override
public void visit(NumericLiteral expression) {

String text = expression.getText();

// - Exact numeric literals support the use of Java integer literal syntax as well as SQL
// exact numeric literal syntax
// - Approximate literals support the use Java floating point literal syntax as well as SQL
// approximate numeric literal syntax
// - Appropriate suffixes can be used to indicate the specific type of a numeric literal in
// accordance with the Java Language Specification
if (!isNumericLiteral(text)) {
if (!expression.hasValidValue()) {
String text = expression.getText();
int startPosition = position(expression);
int endPosition = startPosition + text.length();
addProblem(expression, startPosition, endPosition, NumericLiteral_Invalid, text);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* 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 @@ -256,6 +257,7 @@ public int length() {
*/
public void moveBackward(CharSequence word) {
cursor -= word.length();
wordEndPosition();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to reset the word parser

}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* 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 @@ -306,4 +307,19 @@ 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 shouldBeReverted() {
return getRoot().isJakartaData() && !hasLeftParenthesis();
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* 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 @@ -516,6 +517,22 @@ 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() {
Copy link
Contributor Author

@OndroMih OndroMih Jul 23, 2024

Choose a reason for hiding this comment

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

For a query "SELECT e Employee e WHERE (SELECT COUNT(m) FROM managedEmployees m) > 0)", it will return root expression for subexpressions related to the a variable, and will return the nested SELECT COUNT(m) expression for subexpressions related to the m variable

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 @@ -1133,6 +1150,17 @@ 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 shouldBeReverted() {
return false;
}

@Override
public final String toString() {
// toString() should only be called during debugging, thus the cached parsed text
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* 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 @@ -343,7 +344,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.getRoot().setGenerateThisPrefix(true);
this.getParentExpression().setGenerateThisPrefix(true);
}
}

Expand Down
Loading
Loading