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.

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

Jump to bottom

Implement EQL query parser #3176

Closed
wants to merge 5 commits into from
Closed

Implement EQL query parser #3176

wants to merge 5 commits into from

Conversation

gregturn
Copy link
Contributor

No description provided.

@gregturn
New branch.
ff0511b
@gregturn
Implement EQL parser.
0b1b416 
Implement support for EclipseLink Query Language (EQL), handling the various extensions it offers.

See #3170
@gregturn gregturn requested a review from mp911de September 26, 2023 18:16
@gregturn gregturn added in: repository Repositories abstraction type: enhancement A general enhancement in: query-parser Everything related to parsing JPQL or SQL labels Sep 26, 2023
@gregturn gregturn linked an issue Sep 26, 2023 that may be closed by this pull request
Implement EQL query parser for EclipseLink. #3170
Closed
@gregturn gregturn added this to the 3.2 RC1 (2023.1.0) milestone Sep 26, 2023
mp911de
mp911de requested changes Sep 27, 2023
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

Did a first review pass. Impressive work, also on the verification side. I left a few comments to be addressed.

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/EqlQueryParser.java Outdated
@@ -0,0 +1,132 @@
/*
* Copyright 2022-2023 the original author or authors.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Year of inception is the current year across all newly introduced files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...g-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryEnhancer.java
* @param query must not be {@literal null}.
* @return a new {@link JpaQueryEnhancer} using EQL.
*/
public static JpaQueryEnhancer forEql(DeclaredQuery query) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Missing @since

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancerFactory.java Outdated
@@ -37,6 +37,9 @@ public final class QueryEnhancerFactory {
private static final boolean hibernatePresent = ClassUtils.isPresent("org.hibernate.query.TypedParameterValue",
QueryEnhancerFactory.class.getClassLoader());

private static final boolean eclipseLinkPresent = ClassUtils
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not use org.eclipse.persistence.jpa.JpaEntityManager? Generally, why do we not introduce a property on PersistenceProvider to check whether it is available on the class path?

Introducing redundant checks results in code that is difficult to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I crafted a solution to essentially check for the presence of the same class names used to construct the instances of PersistenceProvider. I also created an interface, PresenceDetector, which I'm not 100% certain we need. Feel free to drop that. It should be visible below.

...data-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlComplianceTests.java Outdated
* IMPORTANT: Purely verifies the parser without any transformations.
*
* @author Greg Turnquist
* @since 3.2
Copy link
Member

Choose a reason for hiding this comment

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

We do not maintain @since tags for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...a-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlQueryRendererTests.java Outdated
}

@Test
void theRest() {
Copy link
Member

Choose a reason for hiding this comment

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

theRest<n> isn't a good naming in particular because it doesn't provide any information. Any chance we can find a better naming that at least gives hints what we're up to?

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 copied that over from JPQL tests. It was a bit of a shortcut. I'll fix it.

...a-jpa/src/test/java/org/springframework/data/jpa/repository/query/EqlSpecificationTests.java
}

@Test
void theRest() {
Copy link
Member

Choose a reason for hiding this comment

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

See theRest comments in one of the previous files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same.

@gregturn
Polishing.
94d28df 
Fix dates and names of theRest methods.
@gregturn
Polishing.
6980ea1 
Fix "the rest" of the theRest method names.
@gregturn
Move testing of JPA persistence provider existence into PersistencePr…
b6561c4 
…ovider.

I created an interface to define the isPresent operation. I don't know whether or not we really need a separate interfacce. Remove it if needed. The lookup is used directly, so perhaps there is little gained by that.
mp911de pushed a commit that referenced this pull request Sep 28, 2023
@gregturn @mp911de
Implement EQL parser.
dde9e07 
Implement support for EclipseLink Query Language (EQL), handling the various extensions it offers.

Closes #3170
Original pull request: #3176
@mp911de mp911de closed this Sep 28, 2023
mp911de added a commit that referenced this pull request Sep 28, 2023
@mp911de
Polishing.
1d49fe9 
Simplify PersistenceProvider by removing PresenceDetector interface. Refine presence detection to make present field final. Add warning suppressions as we know that we duplicate code (similar code) and that ANTLR doesn't generate methods with nullable annotations.

See #3170
Original pull request: #3176
@mp911de mp911de deleted the issue/gh-3170 branch September 28, 2023 09:40
@mp911de
Copy link
Member

mp911de commented Sep 28, 2023

That's merged and polished now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de requested changes

Assignees
No one assigned
Labels
in: query-parser Everything related to parsing JPQL or SQL in: repository Repositories abstraction type: enhancement A general enhancement
Projects
None yet
Milestone
3.2 RC1 (2023.1.0)
Development

Successfully merging this pull request may close these issues.

Implement EQL query parser for EclipseLink.
2 participants
@gregturn @mp911de