search

oasp / oasp4j

The Open Application Standard Platform for Java
Apache License 2.0
60 stars 303 forks source link

spring-data and more #627

Closed hohwille closed 5 years ago

hohwille commented 6 years ago

Here is a great PR with the following features:

Have fun.

Questions:

As we voted to go for pagination support of spring-data, I deprecated all the TOs in oasp4j-jpa and moved all the other code that is still valid to oasp4j-jpa-basic. Now we have oasp4j-jpa-dao and oasp4j-jpa-spring-data that both depend on oasp4j-jpa-basic and give you the option to choose between spring-data or our own DAO support. If you want to have compatibility support for SearchCriteriaTo with the old pagination support, you can still depend on oasp4j-jpa module where this stuff is still contained but deprecated. For your DOA Implementations you can use LegacyDaoQuerySupport that still contains the old paging support as static methods. However, on the long run, we want to get rid of oasp4j-jpa module itself.

hohwille commented 6 years ago

https://stackoverflow.com/questions/42133023/is-there-a-way-to-register-a-repository-base-class-with-a-spring-boot-auto-confi

maybeec commented 6 years ago

Further, spring(-data) has its own support for paging. Should we also align something here?

IMHO, we should go with spring data paging support. Spring data already provides connectors to nearly ever database of interest. However, we should keep the existing mechanism but not make it compatible with spring data but keep it seperately. Thus, we can keep the protocol for more exotic projects or even for the case, where spring data may get deprectated.

hohwille commented 6 years ago

IMHO, we should go with spring data paging support.

I fully agree. That means you vote for solution 1 or 3 (https://github.com/hohwille/oasp4j/wiki/paging). We need to raise a vote/poll in the community for this as this is IMHO a really important decision. However, IMHO not something that would need to block this PR. We could actually easily merge this and other PRs first and then redo PR #630... On the one side you are saying that PRs get too large on the other hand your review implies PRs to be "perfect" before merging. These two do not perfectly go together. Of course I do not want to merge things that bring "bad code" into our mainlines and I am happy for all the feedback even if it forces me to put even more effort into all my PRs. But still I think we need to do things a bit more light-weight and efficient as currently many open PRs somehow closely depend or at least influence on each other. Even in cases where we have "crap" on the mainline we argue in long rounds before we merge even though already the first state of the PR was a great improvement.

maybeec commented 6 years ago

On the one side you are saying that PRs get too large on the other hand your review implies PRs to be "perfect" before merging. These two do not perfectly go together. Of course I do not want to merge things that bring "bad code" into our mainlines and I am happy for all the feedback even if it forces me to put even more effort into all my PRs. But still I think we need to do things a bit more light-weight and efficient as currently many open PRs somehow closely depend or at least influence on each other. Even in cases where we have "crap" on the mainline we argue in long rounds before we merge even though already the first state of the PR was a great improvement.

Totally agree. The issue here is that for large refactorings or rework, PRs are simply not the right tool. I also had issues with that at cobigen repository and thus switching to the "all commit to one branch" mode for a short time. In such a scenario as we have currently, PRs start to diverge all the time and you have to create merge commits all over the place, which already indicates the root cause. IMHO, we should merge all strongly related PRs to the development branch in a controlled and intregrated way and continue work based on that. Whether working in a CI approach (all commit to one branch) or non-CI approach (PRs), depends on the workload.

hohwille commented 6 years ago

Example of Repository from JUnit-Test to show how to use it in the future: https://github.com/hohwille/oasp4j/blob/9325b6d662f567d6be27ffd745777d170673704b/starters/starter-spring-data-jpa/src/test/java/io/oasp/example/component/dataaccess/api/FooRepository.java

maybeec commented 6 years ago

I don't think that the example of integrating QueryDSL and Spring Data as proposed here is a good example. It even hides database operations completely. I don't think it will be of help for developers. In the contrary, I think it would be easier to use a language like SQL or queryDSL in its original API to define queries.

Eventually, I think your implementation is a good compromise. For me this PR is fine.

hohwille commented 6 years ago

I need 2.6.1 being released (#656 and #650) and merged onto develop. Then I will merge the changes into my PR and resolve the conflicts. Hope with all our improvements in the future we will gain more speed and thereby also reduce complex PR dependencies.

hohwille commented 6 years ago

I don't think that the example of integrating QueryDSL and Spring Data as proposed ...

@maybeec thanks for your feedback. I have the same impression and it is good to see you came to the same point. However, one thing I learned from the example you linked is that you can create predicates without the need to reference the entitymanager and can then let the QueryDslPredicateExecutor perform the actual query. At least that confirmed my idea how that interface was meant to be used. Still I also agree that this approach is very limited compared to what the full QueryDSL API offers and what we now offer with our custom integration.

vapadwal commented 5 years ago

I tried to resolve the above conflicts locally, so that I could test it further, but after resolving it there still some modules like jpa-basics and jpa-dao that are removed in develop and that are still there in this PR