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

1192 Query by Example (6/7) #1195

Conversation

DiegoKrupitza
Copy link
Contributor

In this PR I implemented 6 out of the 7 methods from QueryByExampleExecutor. Due to my missing knowledge of MyBatis the implementation MyBatisDataAccessStrategy of DataAccessStrategy currently throws UnsupportedOperationException. I hope there is someone who could implement this who has a better insight into MyBatis.

One method (see below) is not implemented, since I can't see what exactly I should implement here. I also hope someone could implement this so we have full "Query By Example" support.

The following methods are now implemented:

  • Optional<S> findOne(Example<S> example);
  • Iterable<S> findAll(Example<S> example);
  • Iterable<S> findAll(Example<S> example, Sort sort);
  • Page<S> findAll(Example<S> example, Pageable pageable);
  • long count(Example<S> example);
  • boolean exists(Example<S> example);

Missing implementation:

  • findBy(Example<S> example, Function<FetchableFluentQuery<S>, R> queryFunction);

PS: I hope the formatting is now correct (I reconfigured it)

(Partially) Closes #1192

  • You have read the Spring Data contribution guidelines.
  • 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).

christophstrobl and others added 30 commits January 14, 2022 10:57
This is achieved by passing the full availabe type information of the conversion target to the conversion service.

This broke a test which wasn't functional in the first place which becomes obvious when adding the proper assertion.

Closes spring-projects#1046
Original pull request spring-projects#1144
java.sql.Types constants are int values and therefore make it tedious to read and debug the code.
SQLType values are mostly enum constants which are much nicer to use.

Original pull request spring-projects#1142
Extract docker and artifactory credentials into property file.

See spring-projects#1151
Rename "null handling" to "null precedence".
This is somewhat inconsistent with commons null handling, but more descriptive.

Minor formatting.

Original pull request spring-projects#1156
See spring-projects#821
Methods which use the derive query functionality now can be annotated with `@Lock` to used a given `LockMode`. Right now there are two different modes `PESSIMISTIC_READ` and `PESSIMISTIC_WRITE`. Based on the dialect the right select is generated. For example for HSQLDB `Select ... FOR UPDATE`.

See spring-projects#1041
Original pull request spring-projects#1158
Refactored the unit tests to include a negative case and to separate the different scenarios tested.

Removed the default LockMode from the Lock annotation.
I have the feeling that most users will assume an exclusive Lock when none is specified, but also don't want to request stronger locks than required.

Original pull request spring-projects#1158
See spring-projects#1041
Adding references to issues on test annotations.
Made test methods package private.

See spring-projects#1164
The reference documentation now contains how to use `@Lock` on derived queries and what to expect from it.

Original pull request spring-projects#1166
Original pull request spring-projects#1166
mp911de and others added 11 commits March 1, 2022 15:37
We now associate a boolean value with both operators as those operators are rendered using equals comparison in the actual SQL text.

Orginal pull request spring-projects#1188
Reduce test method visibility.

Orginal pull request spring-projects#1188
Added the missing functionality that is found in the documentation of spring-data-jdbc, but was not present in the code as functionality. Users can not choose between various QueryLookupStrategies.

Closes spring-projects#1043
Original pull request spring-projects#1187
Run tests only for a single database since it is actually not testing anything database related.

Formatting.

Original pull request spring-projects#1187
See spring-projects#1043
This commit introduces the find by example method `findOne(Example<S> example)` to spring-data-jdbc. MyBatis implementation is missing since I do not have the knowledge for this.

Related tickets spring-projects#1192
This commit introduces the find by example method `findAll(...)` to spring-data-jdbc. MyBatis implementation is missing since I do not have the knowledge for this.

Related tickets spring-projects#1192
This commit introduces the exists by example method `exists(...)` to spring-data-jdbc. MyBatis implementation is missing since I do not have the knowledge for this.

Related tickets spring-projects#1192
This commit introduces the count by example method `count(...)` to spring-data-jdbc. MyBatis implementation is missing since I do not have the knowledge for this.

Related tickets spring-projects#1192
…le pageable)`.

This commit introduces the find by example pageable method `findAll(Example<S> example, Pageable pageable)` to spring-data-jdbc. MyBatis implementation is missing since I do not have the knowledge for this.

Related tickets spring-projects#1192
Missing MyBatisDataAccessStrategy implementation and `findBy(Example<S> example, Function<FluentQuery.FetchableFluentQuery<S>, R> queryFunction)` marked with `UnsupportedOperationException`.

Related tickets spring-projects#1192
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 12, 2022
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

As for MyBatis: I'd say we can start without MyBatis support.
But if we implement it we should look for query parts that we can assemble using MyBatis into the full query.

As for findBy(Example<S> example, Function<FetchableFluentQuery<S>, R> queryFunction);
There is an implementation in SD JPA for this: https://github.com/spring-projects/spring-data-jpa/blob/47e2423612b2410ec284d1cb048d5a8815a705f3/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java#L499

Maybe with this as a basis you can add the implementation for JDBC as well?

As to the PR: We shouldn't drag the Example abstraction into the JdbcAggregateTemplate. Instead we should pass a Criteria into methods that then perform as select/count/exists query based on that criteria.

There is alread a QueryMapper which gets used by derived queries, which unfortunately currently side steps the JdbcAggregateTemplate if I remember correctly.

@DiegoKrupitza
Copy link
Contributor Author

DiegoKrupitza commented Mar 23, 2022

As for MyBatis: I'd say we can start without MyBatis support

ok sounds good to me. After this PR is completed I can open an new issue for the MyBatis implementation.

As for findBy(Example<S> example, Function<FetchableFluentQuery<S>, R> queryFunction);
There is an implementation in SD JPA for this:

Ok I will take a closer look at this.

We shouldn't drag the Example abstraction into the JdbcAggregateTemplate. Instead we should pass a Criteria into methods that then perform as select/count/exists query based on that criteria.

The main reason why I did that was that people who might use JdbcAggregateTemplate directly still can benefit from the abstraction. Additionally it was easier to create a RelationalExampleMapper object within JdbcAggregateTemplate than inside SimpleJdbcRepository, since I could not see a way to get a RelationalMappingContext in SimpleJdbcRepository.

There is alread a QueryMapper which gets used by derived queries, which unfortunately currently side steps the JdbcAggregateTemplate if I remember correctly.

Yeah I am already using the QueryMapper you are mentioning in SqlGenerator. To do so I had to make certain methods public since they were package-private.

@schauder

PS: I can't promise when I can continue working on this PR since right now I have a lot to do for Uni, but I hope I can find some time here and there

@DiegoKrupitza
Copy link
Contributor Author

@schauder Why was there a force push from the main?

@schauder
Copy link
Contributor

@DiegoKrupitza We turned the old main into 2.7.x and 3.0.x became the new main.

Sorry for the confusion.

@DiegoKrupitza
Copy link
Contributor Author

@schauder I resolved all the merge conflicts and triggered a mvn clean test to check that I did not break anything.
Two tests are failing:

  • org.springframework.data.jdbc.core.convert.InsertStrategyFactoryTest: has nothing to do with my code (I assume this is also why the CI currently fails)
  • org.springframework.data.jdbc.DependencyTests.cycleFree(): this tests fails since inside the SqlGenerator (from package core.convert) I am using the QueryMapper (package repository.query) to convert the Criteria condition to a WHERE condition. QueryMapper has some quite sophisticated logic that is really needed inside the SqlGenerator. The ONLY other location that uses QueryMapper is JdbcQueryCreator (package repository.query) and this is why the cycleFree() test fails. JdbcQueryCreator uses classes from package core.converter. The circle is Cycle detected: Slice core.convert -> Slice repository.query -> Slice core.convert. Rewriting the Cirteria to SqlWhere condition just for the SQLGenerator class would not make sense since this logic is already implemented somewhere.

Can you recommend what I can do against the two failing tests?

…feat/1192_query_by_example.

Two tests are failing. One due to the circular dependency (`jdbc.DependencyTests.cycleFree()`) and one that apparently failed before (`core.convert.InsertStrategyFactoryTest`).
@DiegoKrupitza DiegoKrupitza requested a review from schauder April 9, 2022 15:27
schauder added a commit that referenced this pull request Jul 13, 2022
schauder pushed a commit that referenced this pull request Jul 13, 2022
Original pull request #1195
Closes #1192
schauder added a commit that referenced this pull request Jul 13, 2022
The use of QueryMapper caused dependency cycles.

Original pull request #1195
See #1192
schauder added a commit that referenced this pull request Jul 13, 2022
schauder added a commit that referenced this pull request Jul 13, 2022
Added `@since` comments for new methods and classes.
General formatting and code style tweaking.
Github references for new tests added.
Fixes for integration tests with various databases:
- Not all stores support submillisecond precision for Instant.
- Count for exists query doesn't work for all databases, nor does `LEAST(COUNT(1), 1)`
- MariaDB defaults timestamp columns to the current time.
- Ordering was applied twice.
- DATETIME in SqlServer has a most peculiar preceision. We switch to DATETIME2.

Original pull request #1195
See #1192
@schauder
Copy link
Contributor

Thanks, that's merged.

@schauder schauder closed this Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query by Example support
10 participants