Basic implementation of a page-based pagination.

[henning-cg]

This is a basic implementation of pagination support for REST services and DAOs. It is based on input by @hohwille in #193 and also takes into consideration the discussion in #217.

It offers TOs for requesting pagination inside of general search parameters, and convenience methods in the base DAOs to realize paginated queries.

There are no tests yet, as I mainly see this PR as a place for a first discussion on the general concepts employed.

I converted UcFindOrder#findOrderEtos to use this new pagination support, so we are able to see it in action, although for now no changes have been made to the JS clients.

[oasp-ci]

Can one of the admins verify this patch?

[hohwille]

Here is my review feedback:
In general this is good stuff that is matching my expectations for what we have discussed. So thank you very much already so far. Some things to discuss and improve:

• PaginatedEntityListTo has a generic that is bound to PersistenceEntity. I strongly recomment to remove that bound and leave it unbounded. With JPA you can also do constructor queries and other exotic stuff and we do not want to create a framework that is in the way for specific cases. If you work on this still keep a link to PersistenceEntity in the javadoc for the generic explanation. Also I would recommend to given the generic a reasonable name. Why is it "I"? I do not see that. Call it "E" or "ENTITY" or "HIT" or something.
• Replacing/rewriting SearchCriteriaTo is fine and makes sense but this way you dropped a feature what is the search timeout. It will not hurt to keep that feature for those who might want to use it (I mean this was all related to a general meta-data discussion).
• Implementation looks fine for JPAQuery. But JPA standard queries are not yet supported but had been supported before by applyCriteria. However, as you made the total size a standard feature of all queries we also would need to find a generic solution for the count query for other Queries than the query-dsl ones.

[henning-cg]

Thanks for your feedback, Joerg:

• PaginatedEntityListTo and its bounded type: I see your point and agree.
• Rewritten SearchCriteriaTo and no timeout: Yes, that is missing right now, and probably should be readded. I do think, though, that it should not be exposed to the client, and that it should be set in the service / use case, similar to the maximum allowed page size. What do you think, @hohwille, @amarinso ?
• Standard JPA queries: Well, my vision for the pagination is that the client can request a total count, but that it is the service which decides to actually return it (similar to how the service can limit the maximum page size to a reasonable number). So I don't think that we are obligated to provide the same support for standard JPA queries, although I think it would be nice to have.

[hohwille]

Rewritten SearchCriteriaTo and no timeout: Yes, that is missing right now, and probably should be readded. I do think, though, that it should not be exposed to the client, and that it should be set in the service / use case, similar to the maximum allowed page size. What do you think, @hohwille, @amarinso ?

I agree. But we can simply configure Jackson (later JSON-B) and JAXB in a way that the property is not serialized from/to client.

[hohwille]

Standard JPA queries: Well, my vision for the pagination is that the client can request a total count, but that it is the service which decides to actually return it (similar to how the service can limit the maximum page size to a reasonable number). So I don't think that we are obligated to provide the same support for standard JPA queries, although I think it would be nice to have.

Maybe the easiest way to proceed. However, if there is such a "standard metadata protocol" this will create a general expectation. And then in some cases if total is requested (true) the server will throw an UnsupportedOperationException? We can do that and still improve later.

[henning-cg]

I updated the branch with the suggestions from @hohwille.

One thing though: After removing the type limitation in PaginatedListTo, it is basically a general TO, not directly related to JPA, but it resides in oasp-jpa. When adding jackson annotations to prevent serializing the searchTimeout field, we get a strange dependency to jackson inside oasp-jpa (which funnily enough is fulfilled through oasp-test, it seems).

An obvious solution would be to create corresponding TOs in the service layer, and add mapping between the different layer's TOs, but I am hesitant to do this for the added complexity, and because the rest of the sample doesn't do it neither.

This leads to a general observation, in that sonar is complaining about the usage of search TOs in the data layer, because those TOs are defined in the logic layer of the sample application.

Has there been any discussion about this earlier? If so, could someone give me a link to it in order to understand it better? If not, I will create a separate ticket for it, so as to not derail this PR.

[hohwille]

You can do the jackson mapping configuration with code even outside the TO class. E.g. this can be done as a Mixin (see jackson docs). This is also far better as it allows to toggle the feature as the user actually likes. Maybe someone has a use-case to set that on the client - who knows.
However if the client can influence timeout or limit the server should have the last word to crop the values. Otherwise the client has a great door for DoS attacks (e.g. set limit to max value or set timeout to 1 second and blast the server with timeout exceptions and thread interrupts).

[hohwille]

The PR now looks great. The only picky and tiny thing I could find is a pointless statement:
https://github.com/henning-cg/oasp4j/blob/5d6f0f3398dda3b98d4a76caf2334d20e3a29a97/oasp4j-modules/oasp4j-jpa/src/main/java/io/oasp/module/jpa/dataaccess/base/AbstractGenericDao.java#L308

[hohwille]

his leads to a general observation, in that sonar is complaining about the usage of search TOs in the data layer, because those TOs are defined in the logic layer of the sample application.
Has there been any discussion about this earlier?

#67

My opinion is still that these TOs should simply go to common as they are not restricted to a layer. Raise your voice in #67 if you agree.

[hohwille]

I would vote for merging this PR. If there are no vetos in the next hours I will proceed.
This issue is important and brings us desired progress.

@henning-cg thanks for all your effort 👍

[henning-cg]

Updated to remove pointless statement, hehe 😄

[henning-cg]

I also agree in general that it's fine to have the TOs in common.

For the concrete case of the pagination TOs, there is one problem, though: We use them in the AbstractGenericDao, which is part of oasp-dao, so just putting them into the sample app is not possible.

A possible solution would be to move the pagination support in AbstractGenericDao into the sample app, as well, or to create an interface for the TOs, and use that in AbstractGenericDao. Both options do not convince me completely, as I think that pagination is generic enough for it to be in an oasp module.