Skip to content

DATAJPA-1652: Using || (pipes) along with named parameters in custom queries raises an exception #405

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

DATAJPA-1652: Using || (pipes) along with named parameters in custom queries raises an exception #405

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3084969
DATAJPA-1652: Using || (pipes) along with named parameters in custom …
reta Dec 28, 2019
71ba560
DATAJPA-1652: Addressing code review comments
reta Jan 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ public abstract class QueryUtils {
// Z Separator
// Cc Control
// Cf Format
// P Punctuation
private static final String IDENTIFIER = "[._[\\P{Z}&&\\P{Cc}&&\\P{Cf}&&\\P{P}]]+";
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds a the | to the list of separators for an identifiers.
But the underlying problem lies deeper: the Unicode punctuation (\p{P})category doesn't contain many of the values in the Posix punctuation category (\{Punct}). For example < and > aren't in that list either.

I quickly tried \{Punct} but breaks some other test.

In any case we need a solution addressing the general problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @schauder , I have refactored the implementation a bit to address the issue at large, wondering what do you think, all tests are passing. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

While this fixes the problem at hand while not breaking any tests it just makes it obvious that something is really wrong here: an Identifier shouldn't contain a # and also probably not a $.

It seems IDENTIFIER isn't used for matching identifier, but something like identifier-or-spel
Therefore it

a) should be named as such
b) should be constructed as such, i.e. out of two regex that matches identifiers and spels respectively.
c) all the usages should be verified that they actually should use the more general regex vs the one that actually matches identifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the $ seems to be a valid symbol within identifier name, there is even a dedicated test for it:

@Test // DATAJPA-1200
public void testHasNamedParameter() {
    ...
    checkHasNamedParameter(":$something", true, "dollar");
    ...
}

The # indeed is the SpelEx delimiter, in this context it turned out to be correct but redundant: it is explicitly included within NAMED_PARAMETER pattern. So I removed it from the more strict IDENTIFIER match.

// Punct Punctuation
private static final String IDENTIFIER = "[._$#[\\P{Z}&&\\P{Cc}&&\\P{Cf}&&\\P{Punct}]]+";
static final String COLON_NO_DOUBLE_COLON = "(?<![:\\\\]):";
static final String IDENTIFIER_GROUP = String.format("(%s)", IDENTIFIER);

Expand Down
28 changes: 28 additions & 0 deletions src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,34 @@ public void isNotDefaultProjection() {
softly.assertAll();
}

@Test // DATAJPA-1652
public void usingPipesWithNamedParameter() {

String queryString = "SELECT u FROM User u WHERE u.lastname LIKE '%'||:name||'%'";
StringQuery query = new StringQuery(queryString);

softly.assertThat(query.getQueryString()).isEqualTo(queryString);
softly.assertThat(query.hasParameterBindings()).isTrue();
softly.assertThat(query.getParameterBindings()).hasSize(1);
softly.assertThat(query.getParameterBindings().get(0).getName()).isEqualTo("name");

softly.assertAll();
}

@Test // DATAJPA-1652
public void usingGreaterThanWithNamedParameter() {

String queryString = "SELECT u FROM User u WHERE :age<u.age";
StringQuery query = new StringQuery(queryString);

softly.assertThat(query.getQueryString()).isEqualTo(queryString);
softly.assertThat(query.hasParameterBindings()).isTrue();
softly.assertThat(query.getParameterBindings()).hasSize(1);
softly.assertThat(query.getParameterBindings().get(0).getName()).isEqualTo("age");

softly.assertAll();
}

public void checkNumberOfNamedParameters(String query, int expectedSize, String label) {

DeclaredQuery declaredQuery = DeclaredQuery.of(query);
Expand Down