You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reference app should be a best-practice reference.
During my reviews of #7 I looked a little deeper into the code that was never entirely reviewed before as it seems.
it does a query with a JPQL query given as String inside fragment implementation rather than using @Query annotation in repository omitting any implementation. The latter is actually the real benefit of spring-data-jpa as it will find the query when the application starts up and will create a prepared statement for it. This will improve performance and also ensures that invalid queries lead to fast failure (developers get exception when app is started rather than at runtime).
it does more or less the same query with 4 different technologies. While this was great for evaluaiton (see issue Evaluate spring-data on quarkus devonfw-forge/devonfw-microservices#37) it does not make sense from a business point of view (see Change quarkus reference app from animals to products #9). Besides best practice is to use native queries only in rare cases for performance-tuning where JPQL is not sufficient. Therefore my suggestion would be to reduce all this to QueryDSL what is our recommended technology for dyanmic queries and alongside have something like findByName (or findByTitle) based on static @Query method.
As a result also the REST API is not suitable for a reference.
I just noticed that the path is still missing the version (v1) that needs to be added.
REST API should follow business and restuful conventions so Path segments like criteriaApi are technical implementation details and shall never be exposed in REST API.
Delete method should be void and shall not load the entity what is a waste of performance.
Save method should be void and shall not return updated Dto: With our new approach based on quarkus we can not make save return the new Dto as the result will be incorrect. You can manually test it and will see that you will get a wrong modificationCounter (@Version). To do so you first need to implement Add abstract Entity base-class #13 as currently we are lacking the @Version property. The problem is that hibernate only updates the @Version property after TX gets committed, but we create the DTO from the entity via MapStruct before that. In devon4j with spring we have solved this complex issue in our bean-mapper solution for dozer and orika but with OCX we agreed on MapStruct and that we simply try to avoid this problem by design of the REST API. So to actually get the update after save you will have to call another REST method to get the entity by its ID again on the client.
With devon4j we so far suggested this UcFind* and UcManage* approach. As I already stated in Add guide for quarkus logic layer devonfw/devon4j#432 (review) this leads to developers stop thinking and putting everything else into UcManage*. So I am thinking to create always dedicated use-cases and split up UcManage* into UcSave* and UcDelete* instead. If we want to save boilerplate code, I am fine with omitting the interfaces and just have the implementation class. All public methods are the API, all other methods are not API. No interface required.
The text was updated successfully, but these errors were encountered:
The reference app should be a best-practice reference.
During my reviews of #7 I looked a little deeper into the code that was never entirely reviewed before as it seems.
The repository has several flaws:
https://github.com/devonfw-sample/devon4quarkus-reference/tree/master/src/main/java/com/devonfw/demoquarkus/domain/repo
@Query
annotation in repository omitting any implementation. The latter is actually the real benefit of spring-data-jpa as it will find the query when the application starts up and will create a prepared statement for it. This will improve performance and also ensures that invalid queries lead to fast failure (developers get exception when app is started rather than at runtime).findByName
(orfindByTitle
) based on static@Query
method.As a result also the REST API is not suitable for a reference.
path
is still missing the version (v1
) that needs to be added.criteriaApi
are technical implementation details and shall never be exposed in REST API.Also I collected review-feedback for logic layer:
void
and shall not load the entity what is a waste of performance.void
and shall not return updated Dto: With our new approach based on quarkus we can not makesave
return the newDto
as the result will be incorrect. You can manually test it and will see that you will get a wrongmodificationCounter
(@Version
). To do so you first need to implement Add abstract Entity base-class #13 as currently we are lacking the@Version
property. The problem is that hibernate only updates the@Version
property after TX gets committed, but we create the DTO from the entity via MapStruct before that. Indevon4j
withspring
we have solved this complex issue in our bean-mapper solution for dozer and orika but with OCX we agreed on MapStruct and that we simply try to avoid this problem by design of the REST API. So to actually get the update after save you will have to call another REST method to get the entity by its ID again on the client.devon4j
we so far suggested thisUcFind*
andUcManage*
approach. As I already stated in Add guide for quarkus logic layer devonfw/devon4j#432 (review) this leads to developers stop thinking and putting everything else intoUcManage*
. So I am thinking to create always dedicated use-cases and split upUcManage*
intoUcSave*
andUcDelete*
instead. If we want to save boilerplate code, I am fine with omitting the interfaces and just have the implementation class. All public methods are the API, all other methods are not API. No interface required.The text was updated successfully, but these errors were encountered: