Generator-jhipster: EntityMapper should be invoked from EntityResource only even if EntityService is present

[lordlothar99]

Overview of the issue

[Enhancement] EntityMapper should be invoked from EntityResource only even if EntityService is present

Motivation for or Use Case

In my point of view, EntityService shouldn't bother with DTO <-> Entity conversion, that's only EntityResource responsibility. EntityService should only see entities, and not make any reference to rest package

JHipster Version(s)

2.27.0

JHipster configuration, a .yo-rc.json file generated in the root folder

Example :

{
  "generator-jhipster": {
    "applicationType": "monolith",
    "baseName": "sampleDefaultFromScratch",
    "packageName": "com.mycompany.myapp",
    "packageFolder": "com/mycompany/myapp",
    "authenticationType": "session",
    "hibernateCache": "ehcache",
    "clusteredHttpSession": "no",
    "websocket": "no",
    "databaseType": "sql",
    "devDatabaseType": "h2Disk",
    "prodDatabaseType": "mysql",
    "searchEngine": "no",
    "useSass": false,
    "buildTool": "maven",
    "enableTranslation": true,
    "nativeLanguage": "en",
    "languages": ["en", "fr"],
    "enableSocialSignIn": false,
    "rememberMeKey": "f19f0827c622eb9b81f5227a869ccd44932d78eb",
    "testFrameworks": [
      "gatling"
    ]
  }
}

Entity configuration(s) entityName.json files generated in the .jhipster directory

{
    "relationships": [],
    "fields": [
        {
            "fieldName": "label",
            "fieldType": "String",
            "fieldValidateRules": [
                "required",
                "minlength"
            ],
            "fieldValidateRulesMinlength": "3"
        }
    ],
    "changelogDate": "20150805124936",
    "dto": "mapstruct",
    "pagination": "pagination",
    "service": "serviceClass"
}

Suggest a Fix

I can submit a PR for that

[deepu105]

Ok if you can make a clean PR without much code duplication then plz do

[jdubois]

I didn't code that part so I don't know it very well, but if the EntityService imports a DTO then that's an architecture issue: packages in business layer of the app should not reference packages in the Web/REST layer. So yes this should be changed.

[lordlothar99]

I'm working on the PR...

[lordlothar99]

PR #3212 created. I let you guys have a look at those change ; it seems like there are plenty, but actually generated code if very close to previous version. I tried to clean both generated and generator codes.

[gzsombor]

It's really hard to follow, your proposed changes, but the original idea behind the DTOs/mapper/service implementation is that to decapsulate the API from the actual persistence implementation and from the actual REST endpoints. So the caller only needs to know the DTO, and he can freely call the services, he doesnt need to care about persistence contexts and transaction handling. Yes, sometimes one service needs to call other services, directly with passing domain entities, but these kind of 'naked' calls are better be avoided - or you have to deal with transactions and lazy loadings of entities. From my experiences, the rest controller should only contain: input validation, authentication checks, caching, and rarely some background threading for delayed processing.

[deepu105]

@gzsombor are you trying to say that entity <-> DTO conversins should be happening in the service layer? When I implemented the service layer I too had similar opinion like you above but comment from @jdubois above got me thinking and yes I was not taking separation of concerns into account and I now kind of feel that he is right since service layer sits behind the web it shouldn't be dependent on the DTO which is part of web layer. I didnt think from that perspective when I was coding this. I think transactions should be fine since we have @Transactional on the service class.

I have reviewed and tested #3212 and it works well and I feel that its a little better and cleaner than my original implementation.

So @gzsombor if you are ok we can merge this else we need to see how to proceed

[gzsombor]

@deepu105 : yes, this is my opinion. These DTOs are declaration of the communication protocol between the various other modules, and the UI also. So I can understand, if someone wants to put it into the web layer, but I like to think about as a way to separate independent functionalities inside one application. In our projects, if we have a xyz functionality, then we try to separate the code into 3 modules : xyz-api, xyz-impl and xyz-web. xyz-api contains the DTOs, and the public interfaces which is needed by the clients of this functionality. xyz-impl depends on xyz-api and provides the implementation. xyz-web depends xyz-api, but not on xyz-impl - it only sees the DTOs and the public interfaces, and relies on spring to wire a working implementation for it. So theoretically, when/if we move to a more distributed,microservice like architecture, we only need to create a xyz-impl-remoting (which will implement the xyz-api with remote calls, and some heavy caching). Yes, it's only a theory, if we ever do it, I guess, we need to modify the APIs as well, to efficiently do the bulk operations, etc - but this is a nice theory :wink:

This is more of a rhetorical - and opinionated argument - the more practical is about the problem with the persistence contexts and @Transactional annotations:
Think about returning list of users. At first, it's just List<User> which is gets converted to List<UserDTO> in the REST controller. But after, you notice, that you need not only the name of the user, but all the granted authorities, which is rightly, lazy loaded child entities of the User entity. But as the persistence context is closed after the service returns the list of users, when you call user.getAuthorities() in the REST controller, you will get lazy initialization exceptions. Now, you can choose from the following solutions:

• Put the @Transactional annotation on the REST controller - yikes
• Modify the service class, to eagerly load the authorities :
  
  
  
  • Either with calling user.getAuthorities().size() - which looks really silly, and it's just a matter of time, when will someone think that this is not needed, and remove it altogether. (Even IDEs could remove this kind of dead code :wink: )
  
  
  • Or with modifying the originating query to use join fetchs - which could work, and from the query optimization standpoint, it could improve the performance as well - but this solution not scales, as you put more and more related entities to the result, you will need to maintain a quickly growing complex query - and if you need some entities only conditionally (for example the authorities only visible to the 'admin' users, etc), efficiently, you will have hard time not losing your mind :wink:
  
  
• Or put the mapping to DTOs inside the service class - which I think is the best, most flexible solution.

Yes, in these simple CRUD methods, this is not a big difference, you could put getSomething().size() calls here-and-there to fix the occasional lazy loading exceptions, but I think, if the developers tries to follow this pattern, they will run into these problems, and it's better to provide a more scalable solution.

[jdubois]

I totally agree with @gzsombor on his 3 points.

There is also a 4th solution, which is provided out-of-the-box by Spring Boot, but which I have not used (I have used this pattern _a lot_ in the past, but not the Spring Boot implementation): using the Open Session in View pattern.

• Pros: very convenient, people don't even have to know about lazy loading
• Cons: there's a performance hit (your transactions stay open much too long, so you don't scale well), and people will abuse lazy-loading and might have inefficient code

So I don't like this solution either, but it's worth pointing out.

Then concerning the 3rd point: maybe we should move the DTOs to the service layer, so we have a good layer separation? No more Web imports into the service layer.

[deepu105]

You do have a valid point. As you said this could be a very opinionated
thing so im leaving a blank vote here as Im ok both ways.
Lets see what @jdubois has to say
On 19 Mar 2016 18:04, "Zsombor" [email protected] wrote:

@deepu105 https://github.com/deepu105 : yes, this is my opinion. These
DTOs are declaration of the communication protocol between the various
other modules, and the UI also. So I can understand, if someone wants to
put it into the web layer, but I like to think about as a way to separate
independent functionalities inside one application. In our projects, if we
have a xyz functionality, then we try to separate the code into 3 modules :
xyz-api, xyz-impl and xyz-web. xyz-api contains the DTOs, and the public
interfaces which is needed by the clients of this functionality. xyz-impl
depends on xyz-api and provides the implementation. xyz-web depends
xyz-api, but not on xyz-impl - it only sees the DTOs and the public
interfaces, and relies on spring to wire a working implementation for it.
So theoretically, when/if we move to a more distributed,microservice like
architecture, we only need to create a xyz-impl-remoting (which will
implement the xyz-ap i with remote calls, and some heavy caching). Yes,
it's only a theory, if we ever do it, I guess, we need to modify the APIs
as well, to efficiently do the bulk operations, etc - but this is a nice
theory [image: :wink:]

This is more of a rhetorical - and opinionated argument - the more
practical is about the problem with the persistence contexts and
@Transactional https://github.com/Transactional annotations:
Think about returning list of users. At first, it's just List which is
gets converted to List in the REST controller. But after, you notice, that
you need not only the name of the user, but all the granted authorities,
which is rightly, lazy loaded child entities of the User entity. But as the
persistence context is closed after the service returns the list of users,
when you call user.getAuthorities() in the REST controller, you will get
lazy initialization exceptions. Now, you can choose from the following
solutions:

• Put the @Transactional https://github.com/Transactional annotation
  on the REST controller - yikes
• Modify the service class, to eagerly load the authorities :
  
  
  
  • Either with calling user.getAuthorities().size() - which looks
    
    really silly, and it's just a matter of time, when will someone think that
    
    this is not needed, and remove it altogether. (Even IDEs could remove this
    
    kind of dead code [image: :wink:] )
  
  
  • Or with modifying the originating query to use join fetchs -
    
    which could work, and from the query optimization standpoint, it could
    
    improve the performance as well - but this solution not scales, as you put
    
    more and more related entities to the result, you will need to maintain a
    
    quickly growing complex query - and if you need some entities only
    
    conditionally (for example the authorities only visible to the 'admin'
    
    users, etc), efficiently, you will have hard time not losing your mind [image:
    
    :wink:]
  
  
• Or put the mapping to DTOs inside the service class - which I think
  is the best, most flexible solution.

Yes, in these simple CRUD methods, this is not a big difference, you could
put getSomething().size() calls here-and-there to fix the occasional lazy
loading exceptions, but I think, if the developers tries to follow this
pattern, they will run into these problems, and it's better to provide a
more scalable solution.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-198678606

[deepu105]

Actually that might be a good idea we can move dtos to either service layer
or in a common pacakge
On 19 Mar 2016 18:54, "Deepu K Sasidharan" [email protected] wrote:

You do have a valid point. As you said this could be a very opinionated
thing so im leaving a blank vote here as Im ok both ways.
Lets see what @jdubois has to say
On 19 Mar 2016 18:04, "Zsombor" [email protected] wrote:

@deepu105 https://github.com/deepu105 : yes, this is my opinion. These
DTOs are declaration of the communication protocol between the various
other modules, and the UI also. So I can understand, if someone wants to
put it into the web layer, but I like to think about as a way to separate
independent functionalities inside one application. In our projects, if we
have a xyz functionality, then we try to separate the code into 3 modules :
xyz-api, xyz-impl and xyz-web. xyz-api contains the DTOs, and the public
interfaces which is needed by the clients of this functionality. xyz-impl
depends on xyz-api and provides the implementation. xyz-web depends
xyz-api, but not on xyz-impl - it only sees the DTOs and the public
interfaces, and relies on spring to wire a working implementation for it.
So theoretically, when/if we move to a more distributed,microservice like
architecture, we only need to create a xyz-impl-remoting (which will
implement the xyz-ap i with remote calls, and some heavy caching). Yes,
it's only a theory, if we ever do it, I guess, we need to modify the APIs
as well, to efficiently do the bulk operations, etc - but this is a nice
theory [image: :wink:]

This is more of a rhetorical - and opinionated argument - the more
practical is about the problem with the persistence contexts and
@Transactional https://github.com/Transactional annotations:
Think about returning list of users. At first, it's just List which is
gets converted to List in the REST controller. But after, you notice, that
you need not only the name of the user, but all the granted authorities,
which is rightly, lazy loaded child entities of the User entity. But as the
persistence context is closed after the service returns the list of users,
when you call user.getAuthorities() in the REST controller, you will get
lazy initialization exceptions. Now, you can choose from the following
solutions:

• Put the @Transactional https://github.com/Transactional
  annotation on the REST controller - yikes
• Modify the service class, to eagerly load the authorities :
  
  
  
  • Either with calling user.getAuthorities().size() - which looks
    
    really silly, and it's just a matter of time, when will someone think that
    
    this is not needed, and remove it altogether. (Even IDEs could remove this
    
    kind of dead code [image: :wink:] )
  
  
  • Or with modifying the originating query to use join fetchs -
    
    which could work, and from the query optimization standpoint, it could
    
    improve the performance as well - but this solution not scales, as you put
    
    more and more related entities to the result, you will need to maintain a
    
    quickly growing complex query - and if you need some entities only
    
    conditionally (for example the authorities only visible to the 'admin'
    
    users, etc), efficiently, you will have hard time not losing your mind [image:
    
    :wink:]
  
  
• Or put the mapping to DTOs inside the service class - which I think
  is the best, most flexible solution.

Yes, in these simple CRUD methods, this is not a big difference, you
could put getSomething().size() calls here-and-there to fix the occasional
lazy loading exceptions, but I think, if the developers tries to follow
this pattern, they will run into these problems, and it's better to provide
a more scalable solution.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-198678606

[lordlothar99]

I totally agree on all principles you guys talked about (layers separation, dependencies between modules, responsibility of each module..), but I'm afraid I disagree with you guys on their implementation ! in my opinion, moving DTO to service layer is indeed a violation of single responsibility of service module... as you said, DTO are dedicated to data-transfer with a specific technology, and therefore contain technology-dedicated code (annotations, types, ..). So if DTO are moved to svc-api, it implies that svc layer becomes linked to a specific transfer technology, in addition to dependency to persistence technology...

I used to work on an application which had 3 communication technologies : old EJB, a not-so-old SOAP layer (with normalized DTO for several applications, so quite different from the application own metamodel), and a yet-to-come REST layer. We talked a lot about architecture, and how avoiding code duplication, separation of api and impl, and transactions management. We implemented the principles you were talking about, and we had following (maven) modules :

• ejb
• soap
• rest
• svc-api (agnostic from any communication layer)
• svc-impl
  now that I remember, we also had batches using directly svc-api + svc-impl..
  transactions were started either in ejb, soap or rest layers, so svc didn't have to care about that.
  It would have been a mess to but DTO <-> entity conversion in svc module, cause we had a lot of different way to manipulate the same data, depending on communication protocol.

I think your are right, module "xyz-web" has indeed responsibility for validation, authentication and ... DTO conversion too !! :smile: .
I agree on not using (anymore) open-in-view patterns, and I use to handle lazy-loading hardnesses with automated tests based on communication layer.

I prefer to keep DTO in web module (/package), and mappers too. Anyway, it's always nice to talk about architecture patterns, and know other people opinion !
I let you guys decide which architecture is better ! (we could also add a switch in the generator so people could put dto in web layer, or svc depending on their own choice... but it would need a deep refactor of current ejs templates I think)

[deepu105]

I dont think we should add a switch for this, we should decide on a path
and stick to that.
worst case we can vote and close the case :)

Thanks & Regards,
Deepu

On Sat, Mar 19, 2016 at 8:49 PM, François Lecomte [email protected]
wrote:

I totally agree on all principles you guys talked about (layers
separation, dependencies between modules, responsibility of each module..),
but I'm afraid I disagree with you guys on their implementation ! in my
opinion, moving DTO to service layer is indeed a violation of single
responsibility of service module... as you said, DTO are dedicated to
data-transfer with a specific technology, and therefore contain
technology-dedicated code (annotations, types, ..). So if DTO are moved to
svc-api, it implies that svc layer becomes linked to a specific transfer
technology, in addition to dependency to persistence technology...

I used to work on an application which had 3 communication technologies :
old EJB, a not-so-old SOAP layer (with normalized DTO for several
applications, so quite different from the application own metamodel), and a
yet-to-come REST layer. We talked a lot about architecture, and how
avoiding code duplication, separation of api and impl, and transactions
management. We implemented the principles you were talking about, and we
had following (maven) modules :

• ejb
• soap
• rest
• svc-api (agnostic from any communication layer)
• svc-impl now that I remember, we also had batches using directly
  svc-api + svc-impl.. transactions were started either in ejb, soap or rest
  layers, so svc didn't have to care about that. It would have been a mess to
  but DTO <-> entity conversion in svc module, cause we had a lot of
  different way to manipulate the same data, depending on communication
  protocol.

I think your are right, module "xyz-web" has indeed responsibility for
validation, authentication and ... DTO conversion too !! [image: :smile:]
.
I agree on not using (anymore) open-in-view patterns, and I use to handle
lazy-loading hardnesses with automated tests based on communication layer.

I prefer to keep DTO in web module (/package), and mappers too. Anyway,
it's always nice to talk about architecture patterns, and know other people
opinion !
I let you guys decide which architecture is better ! (we could also add a
switch in the generator so people could put dto in web layer, or svc
depending on their own choice... but it would need a deep refactor of
current ejs templates I think)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-198695884

[gmarziou]

I prefer to have the DTOs as part of the service API, this works perfectly in JHipster's context.
If a user is not happy to have the DTOs depending on JSON, he just can remove JSON annotations and code JSON serialization in another layer.
This discussion would not apply to microservices ;)

[bosofelipe]

Mapstructs works fine at jhipster?

[lordlothar99]

Yes it is!

Le jeu. 24 mars 2016 02:11, Felipe Thiago Boso [email protected] a
écrit :

Mapstructs works fine at jhipster?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-200604164

[deepu105]

@jhipster/developers @lordlothar99 we need to progress on this as well, we need to take a decision since there is heavily opinionated arguments on both ends I suggest we do a voting to take a decision

Below are the options from the discussion

• Option 1. Leave it as it is today
  
  
  
  • pros: user need not handle lazy load issues, and need not worry about transaction handling as highlighted by @gzsombor
  
  
  • cons: no clear separation of layers, there is a web dependency(DTO) injected into service layer, doesnt look so clean from a architecture perspective as highlighted by @lordlothar99 and is less portable
  
  
• Option 2. Use the implementation suggested in this PR (merge PR)
  
  
  
  • pros: There is clear separation of layers, service contract doesnt have to worry about DTO, more portable
  
  
  • cons: There is no clean way to handle lazy load issues, transaction handling becomes a service layer activity and no control on it from web layer
  
  
• Option 3. Keep as it is today but move DTOs from the web layer to its own package outside web and service so that its common
  
  
  
  • pros: There is no web dependency in services, user need not handle lazy load issues, and need not worry about transaction handling in DTOs
  
  
  • cons: doesnt look so clean from a architecture perspective.
  
  

(edit from comment by @jdubois below)

• Option 4. DTOs and mappers are in the service layer, since service layer is an aggregation of domain objects, so it should not expose domain objects, but DTOs.
  
  
  
  • pros: out of your service layer you only see DTOs.
    
    you can use DTOs with other presentation layers, and not just the web/rest layer. For example, a Thymeleaf template.
    
    layers are respected
    
    transactions work
  
  
  • cons: Not as portable as option 2, but still better than option 3
  
  

So if you agree with any of the above options +1 that or if you have some thing better in mind suggest that.

[deepu105]

+1 for option 3 from me

[lordlothar99]

+1 option 2

Le mer. 30 mars 2016 08:37, Deepu K Sasidharan [email protected] a
écrit :

+1 for option 3 from me

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-203275614

[dlresende]

+1 option 2

[miliare]

+1 option 2

[amah]

+1 option 2

[gzsombor]

+1 option 3 - I think this is the most clear solution

[guifesquet]

+1 for option 2. All projects are developped this way,its better to follow common design

[notflorian]

+1 for option 2 : I'm all about separation of concerns and layers

[gzsombor]

All who vote for option 2 :

• please provide a solution for the lazy loading issues,
• and propose a solution, how to separate the internal API - (based on the service interfaces and DTO) from the internal entity model ? Otherwise, you would leak your entity model to distant modules in your app. Without it, the whole app will be a spagetti, without any layering :(

I find funny, the claim, that all projects are developed in the "option 2" - way, in fact, in the last ~4 years, all of the projects, where I've involved, followed that practice, that the entities are mostly hidden in services, and they not leaked to other modules :wink:

[jdubois]

I agree with @gzsombor : even if option 2 looks logical, the need for transactions (for lazy loading) just make it impossible technically.
What I would do (let' call it "option 4) is:

• DTOs and mappers are in the service layer

pros:

• out of your service layer you only see DTOs
• you can use DTOs with other presentation layers, and not just the web/rest layer. For example, a Thymeleaf template.
• layers are respected
• transactions work

I also find quite logical, as your service layer is an aggregation of domain objects, so it should not expose domain objects, but DTOs.

[lordlothar99]

about option 2 transaction concerns : in my opinion several options are valid:

• use @Transactional in rest controllers ; I know you don't like this option much... but they are already there ! so I would say, let's leave it like that...
• the other option is to move @Transactional to service service layer (when there is one) ; I'm not fond of that, not because of lazy loading issues, but because it would be heterogeneous in the application : sometimes on Service, sometime on Controller -> no best practice for the developpers..

Anyway, if @Transactional is on Service, and DTO mapping in Controller, developpers would have to add eager loading in their queries : http://docs.spring.io/spring-data/jpa/docs/current/reference/html/#jpa.entity-graph. That's not so much pain

let's say vote is over in ... 24h ? :smile:

[jdubois]

There can't be a @Transactional in the Web layer, where did you see this? This would be a huge mistake as far as I know:

• The Web layer should not be transactional, because in case of rollback then what you put in your model is just wrong
• There should be a separate Spring application context for the Web layer, and so the @Transactional is just not going to work - at least that's how Spring MVC used to behave, and if this has changed that's quite a big difference with Spring Boot

[lordlothar99]

I found @Transactional annotation in 2 templates, included in EntityResource :

• https://github.com/jhipster/generator-jhipster/blob/master/generators/entity/templates/src/main/java/package/common/get_all_template.ejs
• https://github.com/jhipster/generator-jhipster/blob/master/generators/entity/templates/src/main/java/package/common/search_template.ejs
  Maybe we should add some more tests on transactions behavior...

[jdubois]

Thanks @lordlothar99 - I don't think I coded this originally (this has been refactored, so who knows...), and indeed there are 2 @Transactional

• It's kind of OK with regards to my first point (concerning rollbacks), as they are marked "readOnly" (so you don't write, and hence you don't rollback....). There are some edge cases: if a transaction is started in "write", when you call a "readOnly" transaction, it stays writable... but it's very specific and it's not our issue here.
• For my 2nd point (the child applicationContext that's been used by Spring MVC), I did some tests: removing the annotation doesn't do anything (as everything is loaded from the repository). Putting it in "write" and adding an entity doesn't work (meaning the collection is not managed, and so we are not in a transaction). So as far as I know it shouldn't work, and those 2 basic tests indeed show it doesn't work...

More testing should be done here, but it looks like this is an error. And I hope it is :-)

[deepu105]

I think the transactional annotations are used when using repository
directly in resources without a service layer. I need to take a close look
to confirm.
On 1 Apr 2016 05:51, "Julien Dubois" [email protected] wrote:

Thanks @lordlothar99 https://github.com/lordlothar99 - I don't think I
coded this originally (this has been refactored, so who knows...), and
indeed there are 2 @Transactional

• It's kind of OK with regards to my first point (concerning
  rollbacks), as they are marked "readOnly" (so you don't write, and hence
  you don't rollback....). There are some edge cases: if a transaction is
  started in "write", when you call a "readOnly" transaction, it stays
  writable... but it's very specific and it's not our issue here.
• For my 2nd point (the child applicationContext that's been used by
  Spring MVC), I did some tests: removing the annotation doesn't do anything
  (as everything is loaded from the repository). Putting it in "write" and
  adding an entity doesn't work (meaning the collection is not managed, and
  so we are not in a transaction). So as far as I know it shouldn't work, and
  those 2 basic tests indeed show it doesn't work...

More testing should be done here, but it looks like this is an error. And
I hope it is :-)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-204146348

[deepu105]

Hmmm ok so @Transactional on the resource should be skipped when the calls to repo are via a service

[deepu105]

guys we are still stuck up on this :cry: we need to move forward

[jdubois]

• First we need to remove the @Transactional on the Web layer, and make sure there is no issue there -> this is the most important thing as there might be a issue here (I think there are just extra useless annotations, but this needs to be validated)
• Then I'd go for my "option 4", unless you have some other opinions -> @deepu105 @gzsombor what do you think?

[jdubois]

I removed the @Transactional annotations, my tests didn't show any difference, so indeed I don't think they were doing anything.
Now, is everyone OK for "option 4"?

[evelknievel]

I would like to vote for option 2. Mapping should be done in the resources which are NOT transactional. I don't see why that would be a problem?
I do see a problem in case we want to use the same service with lets say 2 different api's on top of it. The DTO's may be different (front-end specific) but you would like to use the same service. If the mapping is performed in the service, this is not possible!
Issues with lazy loading or transactions should not be solved by changing the architecture (in a bad way).

I would like to ask for reconsidering the architecture.

[windhood]

vote for option 3 especially under jhipster context. As an architecture choice this is more logic and is harder to make junior developers or new team members to make mistake (especially the nasty lazy initialization problem)

[evelknievel]

@windhood

If you want to develop your application that way, just generate it without service layer! The service layer has no meaning at all if you don't seperate the layers in a clean way.

[rroex]

Vote for option 2. Just like @evelknievel said. DTO's are API dependent. So mapping should be done in the resources.

[gzsombor]

I know it's a long thread - both in the number of posts, and in time - so hard to keep in mind everything, but please don't vote for option 2, without suggesting a solution for the lazy loading issues.
Which can be easily reproduced, just generate a new application, and from the UserService.getUserWithAuthorities method, if you remove the user.getAuthorities().size() line, and notice that the /api/account call wont work any more. This is happens, because the AccountResource is coded in such way, that the transformation from the User entity to the UserDTO happens outside of a transaction, which is really wrong.

[deepu105]

To summarize this long and old thread

So there are 2 camps here

1: People who want option 2 discussed above(keep DTO in the web layer and doesn't want any DTO stuff in the service layer) This has 8 votes
2: People who want either option 3 or 4 (Move DTO to the service layer or a common layer outside of the web layer) This has 5 votes (most from the core JHipster team including @jdubois @gzsombor @gmarziou and myself)

Since people who voted option 2 have not provided any clear solution for the lazy load issues arising there we are again at a standstill.

@jdubois I'll leave this to you to take a final decision.

[deepu105]

@lordlothar99 thanks for the patience as I know this has taken a lot of time, hope you understand. @jhipster/hipsters please register here if you have any opinion on this as this could be a major change in server layers

[lordlothar99]

no pb @deepu105 ! sure this has been long talking... but yet no option is really clean IMO. anyway, project has to forward ! let's close this issue..

[evelknievel]

I think collections should not be mapped. You mostly don't need them in frontend. Only in case of a many to many realationship this could be a problem. But I think it can be solved by ignoring the collection in the mapper and do the mapping by hand in the resource.

We changed the architecture in our project to option 2 and we don't have any problems at all.

[lordlothar99]

@evelknievel same for me

[cbornet]

Voting for option 4.
I think that the objects returned by the service layer should not be managed by hibernate anymore. Then these objects can be mapped to "real" DTOs but as an extra mapping, generally not needed.

[jdubois]

Yes we need to move forward:

• The only thing that is sure is that nobody wants option 1, and as that's what we currently have, nobody is happy with our current solution!!
• Option 2 looks indeed the easiest at first, but:
  
  
  
  • it doesn't provide any solution for lazy loading, and that's one of the core issues we're trying to solve here.
  
  
  • From an architecture point of view: if you want DTOs (which is not our default, remember!), that's because you're building a complex system and want your design to last for some time. In that case, the business/service layer is here to protect your domain objects, and it's normal that they are "hidden" behind it. You could have many view layers on top of it: several REST APIs, Thymeleaf templates, etc... All those would only use the same business layer + DTOs, which makes your application more robust.
  
  
• Option 3 & 4 are basically the same: I find option 4 simpler, as there is less code, and as using DTOs is already quite complex, I'd stick with that.

-> this is why I want to go with option 4.

Now from a voting perspective: all top contributors @deepu105 @cbornet @gzsombor @gmarziou and myself are for option 4, and those are the people who know our design the best. And for big decision they are the only ones with voting powers (remember being a top contributor is open and free to everyone, you only need to contribute!!).

-> so for me that's option 4 all the way!!!! Let's do it this way.

[deepu105]

@lordlothar99 hope you are ok with our decision?
@jdubois do you want to do this or shall I do it?

[jdubois]

@deepu105 I'll be on holidays this week, so if you want you can do it now.... Otherwise I'll have time in August

[deepu105]

Ok ill give it a try, shou;dnt be hard as we are just moving the DTO from web layer to the service layer.

So ill create the below package structure and move all DTO/mappers there is it ok?

|_ service
  |_ dto
  |_ mapper

[jdubois]

@deepu105 yes basically they go "down" one level. And that solves the package tangling issues we had, as well as the lazy-loading issues (with @Transactional being at the service level).

[deepu105]

yes, so ill proceed with the above structure

[deepu105]

I will do it as a PR so that you guys can review

[deepu105]

This might be breaking change for people who try to upgrade so may be we can club this with angular2 release? as its not an urgent item anyway

[jdubois]

@deepu105 yes that's going to be a breaking change, so that's for JHipster 4, with AngularJS 2 -> can you create a branch for this?

[deepu105]

yes ill do

[jdubois]

@deepu105 I would like to work on this, have you already started? Can I help you?

[deepu105]

No I havent started coz our angular2 is gonna take more time so I thought
we can do this later

Thanks & Regards,
Deepu

On Tue, Aug 2, 2016 at 9:10 PM, Julien Dubois [email protected]
wrote:

@deepu105 https://github.com/deepu105 I would like to work on this,
have you already started? Can I help you?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-236944945,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlFw-7G5hdcXSRGEACSDm6bVo5_TM8ks5qb2TwgaJpZM4Hxg4v
.

[jdubois]

I'm going to work on this. There shouldn't be any consequences on the client side, so it should be OK with AngularJS 2.

But there is one more topic we haven't talked about: in the current "rest/dto" layer, we have objects like "KeyAndPasswordDTO". Those are not "true" DTOs, I would call them "View Models".

So in the end we're going to have:

|_ service
   |_ dto
   |_ mapper
|_ web
   |_ rest
       |_ vm

I'm using the term "View Model" and calling it "vm" as this is how it's called in Asp.NET, as well as in AngularJS.

Sounds good to everyone?

[sendilkumarn]

👍

[evelknievel]

Another layer? So we have to map 2 times? Or just an exception for one entity? It is getting really messy.

[notflorian]

@evelknievel if I understood well, @jdubois want to rename the DTO into View Model as they are not real DTO, so no, it is not another new layer.

[evelknievel]

That makes it even more stange. Then we are going to map to view models in the service layer... Absolutely not done imo. If you call it DTO it makes a little more sense to map in service layer.

[deepu105]

LGTM

Thanks & Regards,
Deepu

On Wed, Aug 3, 2016 at 7:18 PM, Sendil Kumar N [email protected]
wrote:

👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-237236768,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlFxIfaVct4NKBmJsmLxIGbnWA8CAZks5qcJwqgaJpZM4Hxg4v
.

[jdubois]

I'm discovering our current code for UserDTO and ManagedUserDTO is so wrong here... We use classes from the Web layer in the service layer, we have transactional Spring controllers - I am not happy at all with our code!! And I clearly have my share of responsibility here :-(

So to come back to the DTOs vs "view models":

• DTOs are for transferring data, they are mapped on domain objects so we don't expose our internal data model to the outside world, and to solve the lazy initialization issues.
• View Models are just what is being exposed by controllers, when we need something different or much simpler than an existing domain object: currently we would have the LoggerDTO, the RouterDTO and the KeyAndPasswordDTO which would become LoggerVM, RouterVM and KeyAndPasswordVM -> none of those are mapped on domain objects, so this is consistent with the previous point

This raises 2 questions:

• Could we have a domain object -> DTO -> VM combo? In theory yes, but in practice I don't think this will be very common. Which means we won't generate VMs with the entity sub-generator, at least not in this first version.
• Will we use MapStruct to create the VM? At the moment we don't use it for LoggerDTO, the RouterDTO and the KeyAndPasswordDTO, so it looks like MapStruct isn't needed here. In fact, VMs should be quite different from the domain object or the DTO - if they are just a simple mapping, then there is no need for a VM. So in the end it's less interesting to use MapStruct for VMs.

[evelknievel]

I see what you mean. Looking at the code I agree with you this is a special case where we need some sort of view model. I regret we have 3 sorts of entities. It's getting so confusing.

[jdubois]

@evelknievel this is why, by default, I'd rather only have the domain object all the time, from the database to the view. This was one of my strong opinions at the beginning of the project, and still the default option.

Then, there are specific use cases:

• You might want to have DTOs as they provide a more stable architecture in the long time.
• You might require DTOs because of Hibernate lazy-loading issues
• You might want View Models as you are displaying something very specific, maybe something that is not even an entity

Those use case occur when you code more complex business code, so this is kind of outside the scope of JHipster (as we are only a scaffolding tool), but still we must provide a solution, and some guidance to solve those issues.

Last but not least, there is indeed the possibility to have an entity + DTO + VM. This would only be for a very specific and complex use case, but still that's possible to use if you want to.

[jdubois]

I just commited a first version (only the core generator, not the entity sub-generator) to a specific branch at https://github.com/jhipster/generator-jhipster/commit/a87b3b305962d8cdb6793adffb5f99bace834146

Reviews are welcome!

[jdubois]

And I just added the entity sub-generator migration.
There are probably edge cases that are not working, but the basic stuff is here, so if you want to have a look at what this is going to do, please send feedback!!!!.

[jdubois]

Closing this as I merged the branch into the master.

I have tested many edge cases, and everything looks fine, but this is a very important change, so we might have some issues here.

[deepu105]

If you merged to master then the next master release would become a
breaking one right? How are we gonna handle that

Thanks & regards,
Deepu
On 5 Aug 2016 19:52, "Julien Dubois" [email protected] wrote:

Closing this as I merged the branch into the master.

I have tested many edge cases, and everything looks fine, but this is a
very important change, so we might have some issues here.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-237863680,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF1XSnutn4OfhYXocLlfkdHQtZutqks5qc0c2gaJpZM4Hxg4v
.

[jdubois]

@deepu105 yes, sorry I should have explained why I did this.

• This is only a refactoring of the DTOs, which are still in Beta. So, we can change this without doing a major release, that's what Beta is here for. And it will stay in Beta for some time, as I'm still not sure of everything here.
• The change isn't that big, as the REST APIs stay exactly the same (there is absolutely no change on the client side). It's just some classes that move from one package to another, or which have a new name. So yes it can be breaking, but it shouldn't be too bad.

[cbornet]

As for the folder/package name, I think views would be more explicit than vm. People seeing this package for the first time might not understand what it contains (virtual machines ??)

[jdubois]

Here is why I called it vm:

• that's what we use in AngularJS, so I like this to be consistent
• I also want to have the "model" in it: those are not views, those are view models

Still, I know it's not clear for everyone: that's why I added package-info.java classes, as well as added JavaDoc on top of the classes.

[deepu105]

Ok then +1

Thanks & regards,
Deepu
On 6 Aug 2016 02:41, "Julien Dubois" [email protected] wrote:

Here is why I called it vm:

• that's what we use in AngularJS, so I like this to be consistent
• I also want to have the "model" in it: those are not views, those
  are view models

Still, I know it's not clear for everyone: that's why I added
package-info.java classes, as well as added JavaDoc on top of the classes.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/3175#issuecomment-237967116,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlFwmCvu-5WZaVKxAWXYk1lCHWKlUxks5qc6bogaJpZM4Hxg4v
.

[agoncal]

Arriving after the battle on a closed issue ;o) I just discovered this thread after posting a question on SoF.

I would have voted on Option 2. Being too late, let me just explain why. We used JHipster + DTO since day one. That allows us to really optimize our REST calls by sending over HTTP the bare minium. Now, we need to add a messaging layer... and we are stuck because our service layer returns very small DTOs, and we need more information (i.e. the entities).

As for the transactional issue, we don't have it because we created extra services that deal with lazy loading (I suppose it's not always easy to automatically generate).

So our plans is to re-generate our JHipster app with no DTOs, refactor our services, and then create two sorts of DTOs:

• DTOs specific to REST
• DTOs for messaging

So we are in the pattern "DTO are dedicated to data-transfer with a specific technology".

Plus, JHipster already "kinds" has this possibility. When you generate an application with DTOs and no services, the REST layer does the mapping :

return new ResponseEntity<>(sponsorAgreementMapper.toDto(page.getContent()), headers, HttpStatus.OK);