Skip to content

QueryDSL support. #1330

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
Merged
merged 1 commit into from
Mar 31, 2022
Merged

QueryDSL support. #1330

merged 1 commit into from
Mar 31, 2022

Conversation

mikereiche
Copy link
Collaborator

Closes #1288.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@mikereiche mikereiche requested a review from daschl February 9, 2022 16:25
@mikereiche mikereiche mentioned this pull request Feb 11, 2022
Closed
@mikereiche mikereiche force-pushed the datacouch_1288_querydsl_support branch from 17bd375 to 5a8d65c Compare February 18, 2022 01:02
@mikereiche mikereiche force-pushed the datacouch_1288_querydsl_support branch 2 times, most recently from d4a3910 to faeb781 Compare March 2, 2022 22:43
daschl
daschl suggested changes Mar 4, 2022
View reviewed changes
...ngframework/data/couchbase/repository/query/CouchbaseRepositoryQuerydslIntegrationTests.java
@BeforeAll
static public void beforeAll() {
callSuperBeforeAll(new Object() {});
ApplicationContext ac = new AnnotationConfigApplicationContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

is that class also formatted with the spring data formatter? looks odd

...ngframework/data/couchbase/repository/query/CouchbaseRepositoryQuerydslIntegrationTests.java
}

// this gives hqCountry == "" and hqCountry is missing
// @Test
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this test commented out?

...ava/org/springframework/data/couchbase/repository/support/SpringDataCouchbaseSerializer.java
}

protected DBRef asReference(Object constant, Path<?> path) {
return null; // converter.toDBRef(constant, getPropertyForPotentialDbRef(path));
Copy link
Contributor

Choose a reason for hiding this comment

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

should this throw an unsupported op or something instead of returning null? (not sure how the comment relates)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The commented out is from the mongo implementation. Our converter does not have a toDBRef (or similar) method. I don't think it is ever used.
I changed this to throw an RuntimeException.

...a/org/springframework/data/couchbase/repository/support/SpringDataCouchbaseQuerySupport.java
* CouchbaseDocumentSerializer#toSort(List)
*/
protected Sort createSort(List<OrderSpecifier<?>> orderSpecifiers) {
return null; // TODO serializer.toSort(orderSpecifiers);
Copy link
Contributor

Choose a reason for hiding this comment

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

lots of todos in here

@mikereiche mikereiche self-assigned this Mar 11, 2022
@mikereiche mikereiche force-pushed the datacouch_1288_querydsl_support branch from faeb781 to a37722a Compare March 11, 2022 02:34
@mikereiche mikereiche requested a review from mp911de March 11, 2022 17:39
mp911de
mp911de reviewed Mar 15, 2022
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.

Not sure how well the MongoDB serializer fits into Couchbase. I thought that translating Querydsl predicates into N1QL would be the way to go. Left a few dependency and code style comments.

pom.xml
<dependency>
<groupId>com.querydsl</groupId>
<artifactId>querydsl-apt</artifactId>
<version>${querydsl}</version>
Copy link
Member

Choose a reason for hiding this comment

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

Should be <scope>provided</scope> to avoid mandatory dependencies on the annotation processor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added scope provided

pom.xml
</executions>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

Buildhelper should actually not be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

pom.xml
@@ -160,7 +167,6 @@
<groupId>javax.annotation</groupId>
<artifactId>javax.annotation-api</artifactId>
<version>${javax-annotation-api}</version>
<scope>test</scope>
Copy link
Member

Choose a reason for hiding this comment

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

Should actually remain in test scope. Probably a consequence of how the generated sources are mounted into the project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

restored scope test.

src/main/java/com/querydsl/couchbase/AbstractCouchbaseQuery.save Outdated
// (powered by FernFlower decompiler)
//

package com.querydsl.couchbase;
Copy link
Member

Choose a reason for hiding this comment

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

Missing license headers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. This file doesn't belong in the commit.

src/main/java/com/querydsl/couchbase/AbstractCouchbaseQuery.save Outdated
import org.jetbrains.annotations.Nullable;
import org.springframework.data.couchbase.core.mapping.CouchbaseDocument;

public abstract class AbstractCouchbaseQuery<K, Q extends AbstractCouchbaseQuery<K, Q>> implements SimpleQuery<Q>, Fetchable<K> {
Copy link
Member

Choose a reason for hiding this comment

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

The formatting of this file is off.

@mikereiche mikereiche force-pushed the datacouch_1288_querydsl_support branch from a37722a to c41d710 Compare March 16, 2022 18:36
@mikereiche mikereiche force-pushed the datacouch_1288_querydsl_support branch from c41d710 to 3c9b7f5 Compare March 16, 2022 19:16
@mikereiche mikereiche merged commit c288d05 into 4.4.x Mar 31, 2022
mikereiche added a commit that referenced this pull request Mar 31, 2022
@mikereiche
Querydsl support. (#1330)
d29956f 
Closes #1288.
@mikereiche mikereiche deleted the datacouch_1288_querydsl_support branch May 3, 2022 16:17
@mikereiche mikereiche added this to the 5.0 M4 (2022.0.0) milestone May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de left review comments

@daschl daschl daschl requested changes

Assignees

@mikereiche mikereiche

Labels
None yet
Projects
None yet
Milestone
5.0 M4 (2022.0.0)
Development

Successfully merging this pull request may close these issues.

Support QueryDSL
3 participants
@mikereiche @daschl @mp911de