spring-data and more #627
spring-data and more #627
Conversation
| * @since 3.0.0 | ||
| */ | ||
| // QueryDslJpaRepository<E, ID> forces you to use QueryDSL APT generation what is not desired | ||
| // therefore no support for QueryDslPredicateExecutor (we offer more flexible QueryDSL support anyhow) |
There was a problem hiding this comment.
We should put this remark into javadocs. Otherwise, we might get features requests for this as the docs were not accessible easily.
| if (size <= maxSizeOfInClause) { | ||
| return expression.in(inValues); | ||
| } | ||
| LOG.warn("Creating workaround for IN-clause with {} items - this can cause performance problems.", size); |
There was a problem hiding this comment.
I am not sure, whether we should do this automatically.
- I think this should be opt-in. Not opt-out.
- The default
getInstance()constructor even does not restrict anything as it will set maxSize to INT_MAX. - The
DatabaseWorkaroundclass comes with a strange name, which does not correlate to the function of the class. It is just a value holder, not a workaround implementation. - How can I adapt the value of DatabaseWorkaround maxSize? I think it is not possible besides calling the constructor in bootstrapping phase of my application. However, this is not documented anywhere and I think it is a bad design. We should go back and provide config properties for
maxInClauseSizeand/orallowArbitraryLongInClauses. However, as you can set the first property to MAX_INT, it is even up to discussion if you want to have the second property, which just makes sense if we have a good default value in mind besides MAX_INT.
There was a problem hiding this comment.
But this feature is already opt-in as it is disabled by default:
https://github.com/hohwille/oasp4j/blob/2fe1d69736fc62ed5e91ef5e6845587e8f7e654d/modules/jpa/src/main/java/io/oasp/module/jpa/dataaccess/api/DatabaseWorkaround.java#L29
In order to enable the feature you can subclass DatabaseWorkaround and instantiate it during bootstrap (e.g. via spring).
So all I am seeing valid here is that DatabaseWorkaround is an odd name. We should then agree on what would be a better name for it. A neutral name such as DatabaseConfig?
I would have used application.properties and use a custom spring property but that would IMHO not work well with the static access (and might make the solution unusable without spring-boot).
There was a problem hiding this comment.
But this feature is already opt-in as it is disabled by default
ok, got it. I thought the "workaround" is to support a larger amount of IN clauses than a preconfigured maximum. So the "workaround" here is to restrict IN clauses manually to assure database misbehavior? This was not really clear for me by reading the javadocs.
So all I am seeing valid here is that DatabaseWorkaround is an odd name. We should then agree on what would be a better name for it. A neutral name such as DatabaseConfig?
What about SqlInClauseRestriction? This is the only value, which is hold by this class.
I would have used application.properties and use a custom spring property but that would IMHO not work well with the static access (and might make the solution unusable without spring-boot).
I do not get this entirely. You mean, as you don't have access to the config bean if also supporting non-spring frameworks with this? There are two a little bit ugly solutions:
- Why not supporting both by configuring the class as SpringBean as well as general singleton? This is kind of a hack, but you could set the workaround beans instance on bootstrap when the bean is created and read the spring config at this time.
- You could even just access the properties file in a static instance by the general properties interface of the jdk.
Both solutions do not provide any configuration at run-time, but at bootstrap time. However, I like them more to have the configuration in the general config in contrast to inherit some class to set one property. If we decide for this solution, we should also state in a comment in the properties, that to change this property, you have to restart the application.
There was a problem hiding this comment.
I see your point and agree. Will need some more effort and rework but will be worth it. Then also the class connected with the spring config can be package visible and then also the name is less important. Further you only have to set a property in application.properties to activate the feature.
For the problem itself: The IN clause bind variable design is entirely flawed. An IN clause should actually take one parameter which is a list of values such as primary keys of type Long. However, it is not like this. Instead for each value in the list a separate bind variable is created. This already leads to bad performance and spoofing of statement and execution plan caches. DBs also limit the number of values in an IN clause. Actually that is for reason and therefore we do not support any workaround for it by default. In most cases this is an indicator that you are doing something odd in your queries. Still there are project situations where you have to workaround this limitations. If that is the case, you have to write a lot of code. That is what is offered here only for those that actually need it. But if you ever run into this ugly situation you will love it. Otherwise just ignore it.
| @@ -0,0 +1,512 @@ | |||
| package io.oasp.module.jpa.dataaccess.api; | |||
There was a problem hiding this comment.
I think we should extract the search pattern query support to a seperate util to also document in the class javadocs, that this is not for general use, but the newXClause functions come with a special use case.
Otherwise, you will end up in users writing their queries just which this provided functions, which would end up in a code mess, which cannot be understood easily as you cannot trace the query construction anymore.
There was a problem hiding this comment.
Sorry, but I do not really get this point. Should the QueryDslHelper class go to a different package, a different module (and package) or should it be split into multiple classes? Can you give a more concrete suggestion how to improve?
There was a problem hiding this comment.
The QueryDslHelper contains more than just the newXClause methods. It also contains the pagination creation methods. The latter once might be access by programmers frequently. The newXClause methods however should not as they follow a very specific use case. So I would simply differentiate between both functionalities by seperating them in two different util classes.
There was a problem hiding this comment.
Should then the separate class holding the newXClause methods extend the other class? Otherwise writing repositories gets unnecessary more complicated as you have to know where to find which method. I will give it a try this way. However, then one can still use the extended class in the same way as the current class QueryDslHelper as it is now anywhere. I know that separating aspects is a good idea in general. But this will also raise naming issues and might even lead to more complexity and confusion as the developers using this have to understand what to use when and how the relation between these separated classes is actually designed.
Currently QueryDslHelper is quite obvious: You get a lot of utility/helper methods that help you with QueryDsl. How should we name the extracted class?
There was a problem hiding this comment.
However, then one can still use the extended class in the same way as the current class QueryDslHelper as it is now anywhere.
yes, maybe we are talking about a matter of taste. I just wanted to point out, that there is a certain probability of misuse of the newXClause. But as long as we want keeping inheritance as the way to probagate common API, there is no way out. I would have extracted this to a static util class as this functionality serves a very specific need, i.e. supporting advanced query syntax entered by a user. So why not point our users, who really need it to the util class?
Would even make the API a little bit clearer as the general context is defined by the util class. By using static imports, you even do not produce more unreadable in your dao methods.
| query.where(Alias.$(orderPosition.getOrder().getId()).eq(orderId)); | ||
| return query.list(alias); | ||
| EntityPathBase<OrderPositionEntity> alias = $(orderPosition); | ||
| JPAQuery<OrderPositionEntity> query = new JPAQuery<OrderPositionEntity>(getEntityManager()).from(alias); |
There was a problem hiding this comment.
duplicate type parameter declaration as also in other Dao's multiple times.
Use diamond operator.
There was a problem hiding this comment.
Yes, seems that my eclipse is misconfigured (in some cases it is also a java7 type inference issue but surely not here). I will fix this.
There was a problem hiding this comment.
Actually the entire restaurant sample is going to be deleted very soon. We should not make any fuss about this code. I just invest what is necessary to not break the build. See PR #629 - if that has been merged, we can start PR to delete restaurant.
There was a problem hiding this comment.
Actually you are even wrong here. In this case the Diamond Operator can not be used. This would lead to a compiler error. Yes, Java is sometimes sick.
modules/jpa-spring-data/pom.xml
Outdated
| <groupId>org.hibernate</groupId> | ||
| <artifactId>hibernate-validator</artifactId> | ||
| </dependency> | ||
| --> |
…verything together
|
|
||
| private final int maxSizeOfInClause; | ||
| @Value("${database.query.in-clause.max-values:2147483647}") | ||
| private int maxSizeOfInClause; | ||
|
|
||
| /** Die maximale Anzahl von Werten, welche Oracle in einer IN-Klausel verarbeiten kann. */ | ||
| public static final int MAX_SIZE_OF_IN_CLAUSE_IN_ORACLE = 1000; |
There was a problem hiding this comment.
I think we do not need this field anymore as we cannot set it programmatically on runtime now.
Maybe worth to add it commented to a basic application.properties as well as the property itself also commented.
There was a problem hiding this comment.
I do not see why we do not need this field anymore. Would you suggest rather to inject the entire spring boot application.properties as a Map or Environment instead and get it manually from there? IMHO using the @Value is more compact, more expressive and simpler. Also it is the suggested way to do it according to the spring documentation.
I do agree that we need some kind of documentation for this feature. Once this PR is agreed and merged, I would also provide documentation for the entire feature in the forge wiki. We could also add a comment to the application.properties of our archetype but as we have so many PRs open at the moment I would prefer to get things done and merged first. For nice-to-haves we should create issues and/or follow up PRs. Currently I feel like being in a kind of cage to get fast progress and improvements into OASP4J. Also the PR #629 for our archetype was a nightmare and turned again into a monster PR with tons of different aspects. However, we also fail to get PRs reviewed and merged quickly so in such cases it would even take way longer to create small PRs as I would need >10 PRs then and wait weeks up to moths for each to get merged while I am blocked for the follow up PR as it depends on the previous PR to be done.
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. |
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. |
# Conflicts: # modules/jpa/src/main/java/io/oasp/module/jpa/dataaccess/base/AbstractGenericDao.java
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. |
|
Example of Repository from JUnit-Test to show how to use it in the future: |
|
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. |
@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 |
|
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 |
Here is a great PR with the following features:
Have fun.
Questions:
oasp4j-basic.See https://github.com/hohwille/oasp4j/wiki/paging
As we voted to go for pagination support of spring-data, I deprecated all the TOs in
oasp4j-jpaand moved all the other code that is still valid tooasp4j-jpa-basic. Now we haveoasp4j-jpa-daoandoasp4j-jpa-spring-datathat both depend onoasp4j-jpa-basicand give you the option to choose between spring-data or our own DAO support. If you want to have compatibility support forSearchCriteriaTowith the old pagination support, you can still depend onoasp4j-jpamodule where this stuff is still contained but deprecated. For your DOA Implementations you can useLegacyDaoQuerySupportthat still contains the old paging support as static methods. However, on the long run, we want to get rid ofoasp4j-jpamodule itself.