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

detectAlias() in QueryUtils will pick up other tokens that have "from" in them and pick the next alias from there. #2508

Closed
hedshogg opened this issue Apr 26, 2022 · 9 comments
Assignees
Labels
type: bug A general bug

Comments

@hedshogg
Copy link

In Spring Data JPA 2.6.4 something changed in the alias detection that causes it to sometimes incorrectly determine the alias in a query that uses a cast().

For example:

from User u where (cast(:effectiveFrom as date) is null) OR :effectiveFrom >= u.createdAt

incorrectly identifies the alias to be date when it should be u.

Oddly it doesn't pick up on some other similar uses of cast which it has no trouble with, e.g.

@Test
void detectAliasWithCastCorrectly() {

	assertThat(detectAlias("from User u where (cast(:effective as date) is null) OR :effective >= u.createdAt")).isEqualTo("u");
	assertThat(detectAlias("from User u where (cast(:effectiveDate as date) is null) OR :effectiveDate >= u.createdAt")).isEqualTo("u");
	assertThat(detectAlias("from User u where (cast(:effectiveFrom as date) is null) OR :effectiveFrom >= u.createdAt")).isEqualTo("u");

}

where only the last assertion fails. This is similar in some ways to 2232 although in my case it's with a JPA query rather than a native query.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 26, 2022
@markkrause-wk
Copy link

markkrause-wk commented Apr 26, 2022

It appears the change to the org.springframework.data.jpa.repository.query.QueryUtils#detectAlias method is the cause.
When previously it would return the first match, it now returns the last match.

Previous:

	public static String detectAlias(String query) {
		Matcher matcher = ALIAS_MATCH.matcher(query);
		return matcher.find() ? matcher.group(2) : null;
	}

Current:

	public static String detectAlias(String query) {
		String alias = null;
		Matcher matcher = ALIAS_MATCH.matcher(query);
		while (matcher.find()) {
			alias = matcher.group(2);
		}
		return alias;
	}

@gregturn
Copy link
Contributor

gregturn commented May 4, 2022

Actually, the problem isn't about the CAST at all. It's that your third test case has "from" buried in the middle of :effectiveFrom. Change the "From" part to ANYTHING else, and the test case will pass.

That's because QueryUtils considers that a token.

@gregturn gregturn self-assigned this May 4, 2022
@gregturn gregturn added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 4, 2022
@gregturn gregturn added this to the 3.0 M4 (2022.0.0) milestone May 4, 2022
@gregturn gregturn changed the title detectAlias() in QueryUtils picks up type from cast operation as alias detectAlias() in QueryUtils will pick up other tokens that have "from" in them and pick the next alias from there. May 4, 2022
gregturn added a commit that referenced this issue May 4, 2022
Looking for "from" token during alias detection must be triggered by columns and other constructs that may also have "from" (e.g. :effectiveFrom binding variable).

See #2508.
gregturn added a commit that referenced this issue May 4, 2022
Looking for "from" token during alias detection must NOT be triggered by columns and other constructs that may also have "from" (e.g. :effectiveFrom binding variable).

See #2508.
@gregturn
Copy link
Contributor

gregturn commented May 4, 2022

NOTE: Part of the issue in solving this is that many of our unit tests have "from User u..." (starting with from). When in truth, no JPQL/SQL statement starts with "from". Hence, making a pattern match that is ridiculously hard.

Unit tests should at least have a space before from.

gregturn added a commit that referenced this issue May 4, 2022
Looking for "from" token during alias detection must NOT be triggered by columns and other constructs that may also have "from" (e.g. :effectiveFrom binding variable).

See #2508.
gregturn pushed a commit that referenced this issue May 5, 2022
Add a zero-width word boundary to the regex that identifies the from clause. This is used in alias detection.

See #2508, #2260.
gregturn pushed a commit that referenced this issue May 5, 2022
Add a zero-width word boundary to the regex that identifies the from clause. This is used in alias detection.

See #2508, #2260.
gregturn pushed a commit that referenced this issue May 5, 2022
Add a zero-width word boundary to the regex that identifies the from clause. This is used in alias detection.

See #2508, #2260.
gregturn pushed a commit that referenced this issue May 5, 2022
Add a zero-width word boundary to the regex that identifies the from clause. This is used in alias detection.

See #2508, #2260.
@gregturn
Copy link
Contributor

gregturn commented May 5, 2022

Resolved in main.

@gregturn gregturn closed this as completed May 5, 2022
@gregturn
Copy link
Contributor

gregturn commented May 5, 2022

Backported to 2.7.x.

@gregturn
Copy link
Contributor

gregturn commented May 5, 2022

Backported to 2.6.x.

@gregturn
Copy link
Contributor

gregturn commented May 5, 2022

Backported to 2.5.x.

@markkrause-wk
Copy link

@gregturn thanks for looking into this.
However, I think there is still an issue (although it may be different root case). We were unable to update to Spring Data JPA 2.6.4 because of what I highlighted above. Finding the last match breaks queries which contain sub-selects.

gregturn added a commit that referenced this issue Mar 22, 2023
In the past, a token with "from" buried in it would trip up QueryUtils. These tests further demonstrate that this is no longer an issue with the HQL parser.

Related: #2508.
@gregturn
Copy link
Contributor

@markkrause-wk You may like to know, that the new HQL parser handles this with ease (latest branch of Spring Data JPA). All three scenarios now pass. I'm adding this to our test suite. (2f6955e)

If you have another query to further check this out, please let us know.

klajdipaja pushed a commit to klajdipaja/spring-data-jpa that referenced this issue Mar 24, 2023
In the past, a token with "from" buried in it would trip up QueryUtils. These tests further demonstrate that this is no longer an issue with the HQL parser.

Related: spring-projects#2508.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants