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 FluentQuery support for Querydsl and Query by Example #2326

Closed
wants to merge 2 commits into from
Closed

Implement FluentQuery support for Querydsl and Query by Example #2326

wants to merge 2 commits into from

Conversation

gregturn
Copy link
Contributor

@gregturn gregturn commented Oct 6, 2021

No description provided.

@gregturn gregturn requested a review from mp911de October 6, 2021 16:39
@gregturn
Copy link
Contributor Author

gregturn commented Oct 7, 2021

I should drop the DtoInstantistingConverter since it isn’t used (yet).

mp911de
mp911de requested changes Oct 7, 2021
View reviewed changes
src/main/java/org/springframework/data/jpa/repository/query/DtoInstantiatingConverter.java Outdated
import org.springframework.data.mapping.model.ParameterValueProvider;
import org.springframework.util.Assert;

public class DtoInstantiatingConverter implements Converter<Object, Object> {
Copy link
Member

Choose a reason for hiding this comment

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

For the time being I suggest removing this class as the DTO support needs some additional work.

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.

src/main/java/org/springframework/data/jpa/repository/query/DtoInstantiatingConverter.java Outdated

this.targetType = dtoType;
this.context = context;
System.out.println(this.context.getManagedTypes());
Copy link
Member

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.

Gone

src/main/java/org/springframework/data/jpa/repository/query/FetchableFluentQueryByExample.java Outdated
* @author Mark Paluch
* @since 2.6
*/
public class FetchableFluentQueryByExample<S, R> extends FluentQuerySupport<R> implements FetchableFluentQuery<R> {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a new type, we don't need any additional authors. Additionally, I'd make this class package-private.

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 will drop the additional authors. However, SimpleJpaRepository, the primary user of this class, is in a separate package, hence. package-private won't work.

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've move this to the same package and adjusted visibility.

src/main/java/org/springframework/data/jpa/repository/query/FetchableFluentQueryByExample.java Outdated
@Override
public <NR> FetchableFluentQuery<NR> as(Class<NR> resultType) {

return new FetchableFluentQueryByExample<S, NR>(this.example, resultType, this.sort, this.properties, this.finder,
Copy link
Member

Choose a reason for hiding this comment

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

We might want to reject resultType that are classes for a clear and early indication that DTO's aren't supported yet.

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.

src/main/java/org/springframework/data/jpa/repository/query/FetchableFluentQueryByExample.java Outdated
public Stream<R> stream() {

if (this.resultType != this.example.getProbeType()) {
System.out.println("You have a projection!");
Copy link
Member

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.

Removed.

src/main/java/org/springframework/data/jpa/repository/query/FluentQuerySupport.java Outdated
* @author Michael J. Simons
* @since 2.6
*/
abstract class FluentQuerySupport<R> {
Copy link
Member

Choose a reason for hiding this comment

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

Author is only you.

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

src/main/java/org/springframework/data/jpa/repository/query/FluentQuerySupport.java Outdated

protected Class<R> resultType;
protected Sort sort;
@Nullable protected Set<String> properties;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We put @Nullable after the visibility modifier. It would make sense to keep all of these properties final.

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

...main/java/org/springframework/data/jpa/repository/query/FetchableFluentQueryByPredicate.java Outdated

private Page<R> readPage(Pageable pageable) {

JPQLQuery<S> pagedQuery = this.pagedFinder.apply(this.sort, pageable);
Copy link
Member

Choose a reason for hiding this comment

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

properties(from project) aren't evaluated yet within the projection. Not sure what it requires to construct an entity graph from these properties.

src/main/java/org/springframework/data/jpa/repository/support/QuerydslJpaRepository.java
@@ -164,6 +167,13 @@ public QuerydslJpaRepository(JpaEntityInformation<T, ID> entityInformation, Enti
return PageableExecutionUtils.getPage(query.fetch(), pageable, countQuery::fetchCount);
}

@Override
public <S extends T, R> R findBy(Predicate predicate,
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java Outdated
*/
private static class ExampleSpecification<T> implements Specification<T> {
public static class ExampleSpecification<T> implements Specification<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this class private? We should not expose 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.

Done

@mp911de mp911de self-assigned this Oct 7, 2021
@mp911de
Copy link
Member

mp911de commented Oct 7, 2021

Can we move the fluent query classes into the support package? That way we can keep them package-private.

@gregturn
Copy link
Contributor Author

gregturn commented Oct 7, 2021

Packages moved, visibility adjusted.

@gregturn
Implement FluentQuery for Querydsl and Query by Example.
64beaab 
Add support for both QueryByExampleExecutor and QuerydslPredicateExecutor. This is used in SimpleJpaRepository and QuerydslJpaPredicateExecutor, resulting in various test cases proving support by both examples and Querydsl predicates.

NOTE: Class-based DTOs are NOT supported yet.

Closes #2294.

Related: #2327.
@gregturn
Copy link
Contributor Author

gregturn commented Oct 7, 2021

Branch has been polished and force pushed.

Opened #2327 to track class-based DTOs.

mp911de pushed a commit that referenced this pull request Oct 8, 2021
@gregturn @mp911de
Implement FluentQuery for Querydsl and Query by Example.
ea430f3 
Add support for both QueryByExampleExecutor and QuerydslPredicateExecutor. This is used in SimpleJpaRepository and QuerydslJpaPredicateExecutor, resulting in various test cases proving support by both examples and Querydsl predicates.

NOTE: Class-based DTOs are NOT supported yet.

See #2294
Original pull request: #2326.
mp911de added a commit that referenced this pull request Oct 8, 2021
@mp911de
Polishing.
a31c39d 
Fix version reference to Spring Data Commons. Avoid stream creation when fetching all/one/first element. Move off deprecated API. Add override comments. Add null guargs.

See #2294
Original pull request: #2326.
@mp911de mp911de added the type: enhancement A general enhancement label Oct 8, 2021
@mp911de mp911de changed the title Introduce Fluent Query support Implement FluentQuery support for Querydsl and Query by Example Oct 8, 2021
@mp911de mp911de added this to the 2.6 RC1 (2021.1.0) milestone Oct 8, 2021
@mp911de mp911de linked an issue Oct 8, 2021 that may be closed by this pull request
Implement FluentQuery support for Querydsl and Query by Example #2294
Closed
@mp911de
Copy link
Member

mp911de commented Oct 8, 2021

That's merged and polished now.

@mp911de mp911de closed this Oct 8, 2021
@mp911de mp911de deleted the issue/gh-2294 branch October 8, 2021 12:09
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

@mp911de mp911de

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.6 RC1 (2021.1.0)
Development

Successfully merging this pull request may close these issues.

Implement FluentQuery support for Querydsl and Query by Example
2 participants
@gregturn @mp911de