-
Notifications
You must be signed in to change notification settings - Fork 20
Open
Description
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
- it does not properly support paging
- it does a query with a JPQL query given as String inside fragment implementation rather than using
@Queryannotation 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(orfindByTitle) based on static@Querymethod.
As a result also the REST API is not suitable for a reference.
- I just noticed that the
pathis still missing the version (v1) that needs to be added. - REST API should follow business and restuful conventions so Path segments like
criteriaApiare technical implementation details and shall never be exposed in REST API. - I already stated or reviewed somewhere that we should avoid plural forms and also added it to our guide: https://github.com/devonfw/devon4j/blob/master/documentation/guide-rest.asciidoc#urls - Github is a good example of such anti-pattern: Example of logic layer #7 but https://github.com/devonfw-sample/devon4quarkus-reference/pull gives you 404 as it has to be https://github.com/devonfw-sample/devon4quarkus-reference/pulls - with issues however, it is always plural for both collection and resource (https://github.com/devonfw-sample/devon4quarkus-reference/issues and Add abstract Entity base-class #13). This is crap and flawed. Please always use singluar and follow KISS.
Also I collected review-feedback for logic layer:
- Delete method should be
voidand shall not load the entity what is a waste of performance. - Save method should be
voidand shall not return updated Dto: With our new approach based on quarkus we can not makesavereturn the newDtoas 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@Versionproperty. The problem is that hibernate only updates the@Versionproperty after TX gets committed, but we create the DTO from the entity via MapStruct before that. Indevon4jwithspringwe 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
devon4jwe 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.
Metadata
Metadata
Assignees
Labels
No labels