Support for PagingRepositories [DATAJDBC-101]

[spring-projects-issues]

Jens Schauder opened DATAJDBC-101 and commented

---

Issue Links:

• DATAJDBC-29 Add pagination abstraction

Referenced from: pull request #183

7 votes, 9 watchers

[spring-projects-issues]

Jens Schauder commented

Spring Batch contains various implementations of PagingQueryProvider which we probably could copy and adapt for our purposes

[spring-projects-issues]

Milan Milanov commented

Hi there, Jens Schauder, Mark Paluch. I saw that https://github.com/spring-projects/spring-data-jdbc/pull/125 was merged some time ago, but the paging/sorting repository is still not available. Since I'm interested in having this feature I thought I can try and implement it myself. Some questions:

• has any work been done already that i'm unaware of
• (if not) do you think it's feasible for a newcomer to implement it
• (if so) can you give me some pointers as to what needs to be done in general, like the sketch, and i can try to take it from there

Kind regards,

Milan

[spring-projects-issues]

Jens Schauder commented

Hi Milan Milanov, 

• No there hasn't been anything done on this yet.
• Yes I think it should be feasible to implement.

 

A sketch how to implement this:

1. let SimpleJdbcRepository implement PagingAndSortingRepository
2. Take a look at the findAll implementation ans inspiriation on how to implement that. This will lead to new methods in JdbcAggregateOperations and DataAccessStrategy and their implementations.
3. The interesting part is in the SqlGenerator where you'll have to create the appropriate SQL based on DATAJDBC-357

 

[spring-projects-issues]

Milan Milanov commented

Hi Jens Schauder,

i took a look at the project and the task yesterday. Kudos for the easier to reason about abstractions. I'm now looking at at the SelectBuilder.SelectWhere selectBuilder(Collection<String> keyColumns) method in the SqlGenerator.I rewrote the end of the method as such, just for testing:

SelectBuilder.SelectAndFrom selectBuilder = StatementBuilder.select(columnExpressions);

Sort s = null;
Pageable p = null;

if (!joinTables.isEmpty()) {
    SelectBuilder.SelectJoin baseSelect = selectBuilder.from(table);
    for (Join join : joinTables) {
        baseSelect = baseSelect.leftOuterJoin(join.joinTable).on(join.joinColumn).equals(join.parentId);
    }
    return ((SelectBuilder.SelectFromAndJoinCondition) baseSelect).limitOffset(p.getPageSize(), p.getOffset()).orderBy(<orders>);
} else {
    return selectBuilder.from(table).orderBy(<orders>).limitOffset(p.getPageSize(), p.getOffset());
}

So, now my questions are:

• Would you be okay if i pass the Sort/Pageable params all the way in JdbcAggregateOperations->DataAccessStrategy->SqlGenerator, or do you prefer some in-house spring data JdbcRepository specific classes
• Does the code look fine? Or should i try to separate it in some other methods and not touch the existing selectBuilder (although i'm doing some castings which would otherwise be unsafe outside of the method body).
• Do you know of an existing way to construct Column/OrderByField from the Sort.Order. If not, should i add the conversion in some util or could i add a static factory method in one of the two classes and make it accept a Sort.Order instance

[spring-projects-issues]

Jens Schauder commented

• Sort/Pageable arguments are fine. Those are already Spring Data abstractions.
• In principle the code looks fine. There will probably some polishing happen afterwards, but don't wory to much about it.
• The conversion from Sort/Pageable to constructs applicable to SelectBuilder should probably happen in a static conversion method. Probably in the SqlGenerator. If it gets to complex/length/hard to test consider creating separate classes. Normal clean code rules apply

[spring-projects-issues]

Milan Milanov commented

Hello again. I did some actual work on this issue and have the following new questions:

• Which formatter do you use (as the one in my IntelliJ Idea is somewhat different)
• In the MyBatisDataAccessStrategy, how should i name the two new statements (find sorted and paged) be named? Do .findAllSorted/Paged make sense? Or .findAll-sorted/paged?
• Should I pass the sort/pageable parameters in the additional values of the MyBatisContext? If so, how should i name them (the keys)?
• In a DataAccessStrategy implementation, am i allowed to call the public methods from the same instance? Or does the Spring proxying magic interfere and i should just inline the implementation (of the count method, which is one line).
• Finally, and most importantly, in the SqlGenerator there is a render method that calls SqlRenderer.create. This uses a SimpleRenderContext, when in my case i need the DialectRenderContext from the RenderContextFactory. However, it requires a Dialect which i'm not sure how exactly to acquire in the sql generator

[spring-projects-issues]

Jens Schauder commented

• Formatter: Wie use the eclipse formatter with the configuration which can be found here: https://github.com/spring-projects/spring-data-build/tree/master/etc/ide
• MyBatis names: findAllSorted and findAllPaged please. The dash names are for dynamically created query names.
• Context names: sort and pageable sound good. And yes the need to be added as additional values ot the MyBatisContext
• Delegation to within DataAccessStrategy: No they shouldn't call themselves. We want two methods one doing the main select and returning a List or similar and one doing the count. The results can then be combined into a Page by the JdbcAggregateTemplate. The reason for this is that the count OR the main query might get implemented via MyBatis or a home grown DataAccessStrategy, which won't work with method calls inside a DAS.
• The Dialect should be an optional dependency of a SqlGenerator, so that if it is present it gets used and if not a SimpleRenderContext gets used. This will probably percolate through some code possibly ending up in AbstractJdbcConfiguration

I hope that helps. Feel free to ask if you have more questions

[spring-projects-issues]

Milan Milanov commented

Yup, one more round, hopefully one of the last ones:

• Should i add myself as "@author" in the files that i modify? I saw that being done in some PRs, but i don't really feel like an author of any of these files.
• Regarding the Dialect setup - does it make sense in your opinion to have an additional Optional<Dialect> parameter in the AbstractJdbcConfiguration#dataAccessStrategyBean}, then pass it down to the {{new SqlGeneratorSource(context, dialect) and there based on the presence or absence of a dialect instantiate the proper SqlRenderer - dialect.map(d -> SqlRenderer.create(newRenderContextFactory(d).createRenderContext())).orElse(SqlRenderer.create())? Then i can pass down the renderer to the SqlGenerator instances and they can use that instead of instantiating new renderers on each call.
• Should i add integration tests for the two new JdbcAggregateTemplate methods? Anything special i should know here? I saw quite a few tests skipped under mssql?

[spring-projects-issues]

Jens Schauder commented

• Yes, please add yourself as @author in the files you edit.
• Dialect setup: sounds absolutely reasonable.
• Yes please add integration tests. The mssql tests are skipped because we don't fully support it yet.

[spring-projects-issues]

Milan Milanov commented

Hi Jens Schauder, some time passed but i'm back onto this issue now. One more question - in the SqlGeneratorUnitTests, if i want to try the generation of the paging, should i create some test AbstractDialect or should i reuse some of the existing ones? I'd prefer to have a test one, but tbh i'd just implement it as a copy of the PostgresDialect, just without the getArraySupport method override

[spring-projects-issues]

Jens Schauder commented

The variant that I'd prefer is to create an AnsiDialect that implements standard SQL and then use that for testing.

 

You should base your work on https://github.com/spring-projects/spring-data-jdbc/pull/182 since it actually introduces dialects in Spring Data JDBC

[spring-projects-issues]

Jens Schauder commented

Also I want this to get in the next GA release, so please don't take to much time, otherwise I'd have to implement it myself

[spring-projects-issues]

Milan Milanov commented

Okay, i'll take a look at the PR and try to base my changes onto it. I'll write here in a few days to let you know how it goes so you can decide how to proceed

[spring-projects-issues]

Milan Milanov commented

Opened #183. Feedback welcome