But then I need to use the entityManager in the mapper or am I missing something?
On 16 June 2014 11:17, Romain Manni-Bucau <[email protected]> wrote: > Yes you need to merge it but the responsability is yours (user) IMO. > Le 16 juin 2014 09:56, "Karl Kildén" <[email protected]> a écrit : > > > Hrmm maybe I mixed things up now. > > > > If you have a relationship to another pre existing entity can it be > > detached when you save? All I am really looking for is the groupId to be > > updated but maybe JPA can't determine this in a good way. And updating > the > > entity itself is solved by including it as an argument to the mapper, all > > though I am a wondering if that solution is optimal. > > > > Romain and Thomas, your comments on the best way to handle relationships > in > > the Mapper? If the entity has not changed then you can just use > > user.getGroup() but as I described previously this is wrong when the > group > > has changed. > > > > > > On 16 June 2014 10:34, Romain Manni-Bucau <[email protected]> wrote: > > > > > Cause mapping can be done through several transactions (think jaxrs) so > > it > > > would be wrong. PersistenceUtil has some good things to gey id or null > if > > > new but IIRC some impl are broken. > > > Le 16 juin 2014 09:31, "Thomas Andraschko" < > [email protected]> > > a > > > écrit : > > > > > > > Why don't we use entityManager#contains instead of checking the ID? > > > > > > > > > > > > 2014-06-16 10:22 GMT+02:00 Karl Kildén <[email protected]>: > > > > > > > > > Hi, > > > > > > > > > > On could argue that the real problem is that the algorithm that > > decides > > > > if > > > > > it should be a save or persist. It uses some portable way of > deciding > > > > this > > > > > that requires the entity to be managed. > > > > > That algorithm could be improved in each project. > > > > > > > > > > > > > > > @Override > > > > > @RequiresTransaction > > > > > public E save(E entity) > > > > > { > > > > > if (context.isNew(entity)) > > > > > { > > > > > entityManager().persist(entity); > > > > > return entity; > > > > > } > > > > > return entityManager().merge(entity); > > > > > } > > > > > > > > > > > > > > > > > > > > I would say that overriding this method would solve > > > EntityExistsException > > > > > in a cleaner way. For this project I have no natural keys only > > > generated > > > > > long so it would be a cheap and easy way to fix it... This is fully > > > > > sufficient for me: > > > > > > > > > > > > > > > @Override > > > > > @RequiresTransaction > > > > > public E save(E entity) > > > > > { > > > > > BaseEntity e = (BaseEntity) entity; > > > > > if (e.getId == 0) > > > > > { > > > > > entityManager().persist(entity); > > > > > return entity; > > > > > } > > > > > return entityManager().merge(entity); > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > If not overridden then what happens if the group has changed in my > > > > example, > > > > > you are supposed to use entityManager and find it? > > > > > > > > > > Maybe the API should provide something like the utility method I > > > proposed > > > > > then... I will explain it a little better. All you need to do is > > inject > > > > the > > > > > groupMapper or whatever mapper you need. To get the group if it > > changed > > > > you > > > > > simply call fetch with a new Group instance, the DTO with the new > > > > > information and the groupMapper. Why send in new group instance? > Well > > > one > > > > > could also send in Group.class and use a reflection to create a new > > > group > > > > > if needed. But new Group() vs Group.class I actually prefer the > first > > > > > because it avoids reflection. Because the new Group() argument also > > > > allows > > > > > for getClass() the entityManager has all the info required except > the > > > id > > > > > but that is no problem since we also send in the mapper with the > > handy > > > > > #getPrimaryKey > > > > > method. > > > > > > > > > > Group g = fetch (new Group(), user.getGroup(), groupMapper); > > > > > > > > > > public <Entity, Dto> Entity fetch(Entity entity, Dto dto, > > > > > SimpleQueryInOutMapperBase<Entity, Dto> entityMapper){ > > > > > Object pk = entityMapper.getPrimaryKey(dto); > > > > > Entity foundEntity = (Entity) > > > > entityManager.find(entity.getClass(), > > > > > pk); > > > > > > > > > > if (foundEntity == null) { > > > > > foundEntity = entity; > > > > > } > > > > > return entityMapper.toEntity(foundEntity, dto); > > > > > } > > > > > > > > > > > > > > > I have not tested this method at all, but something like it would > > work > > > > well > > > > > together with the default strategy for determine save or merge... > But > > > my > > > > > main wish now is to override the save logic actually. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 16 June 2014 09:48, Thomas Hug <[email protected]> wrote: > > > > > > > > > > > Thanks Karl for the clarification! > > > > > > If you assign a new group, I'd first make sure you have this one > > > > > persisted > > > > > > as well (or we do too much in the mapper IMO). Then save the > > userDto > > > > with > > > > > > something like > > > > > > > > > > > > User toEntity(User user, UserDto dto) { > > > > > > ... > > > > > > user.setGroup(groupMapper.toEntity(dto.getGroup()); // or > check > > > > > before > > > > > > if ID changed > > > > > > } > > > > > > > > > > > > I guess that would even work if the group is not persisted if you > > > adapt > > > > > > cascading. > > > > > > > > > > > > Makes sense? > > > > > > > > > > > > On Fri, Jun 13, 2014 at 5:56 PM, Karl Kildén < > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > Not sure I get myself ;) > > > > > > > > > > > > > > Lets walk through how I see it: > > > > > > > > > > > > > > 1. User "foo" is created and is assigned to the preexisting > group > > > > > > "Admin". > > > > > > > > > > > > > > It goes like this in the flow: user = new UserDTO() -> > > > > > > > *user*.setGroupDTO(selectedGroup) > > > > > > > -> save > > > > > > > > > > > > > > Some logic must make sure that we don't get > EntityExistsException > > > > > trying > > > > > > to > > > > > > > save that group. > > > > > > > > > > > > > > > > > > > > > 2. Many sessions later user "foo" is edited. Flow: > > > > > > > *user*.setGroupDTO(newGroup) > > > > > > > -> save > > > > > > > > > > > > > > The variable *user *is a random object that happens to be the > > > latest > > > > > > > version of that user that also has a new group set. > > > > > > > > > > > > > > So *PK, user.getGroup()* > > > > > > > *should lazyload the group - right? *This will result in the > > > previous > > > > > > group > > > > > > > being loaded unless I am missing something. While it is > > technically > > > > > > correct > > > > > > > since the new group relationship has not been persisted yet it > is > > > > > > actually > > > > > > > impossible to ever update group with this flow > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 13 June 2014 17:09, Thomas Hug <[email protected]> > wrote: > > > > > > > > > > > > > > > Hi Karl > > > > > > > > Sorry not sure if I get you right. Why would user.getGroup() > be > > > > > stale? > > > > > > As > > > > > > > > in the update case user has just been fetched by the PK, > > > > > > user.getGroup() > > > > > > > > should lazyload the group - right? > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jun 13, 2014 at 2:51 PM, Karl Kildén < > > > > [email protected]> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi and hrmmm, > > > > > > > > > > > > > > > > > > What if user group was changed? > > > > > > groupMapper.toEntity(user.getGroup(), > > > > > > > > > userDto.getGroup()); This would then operate on stale data > > > unless > > > > > you > > > > > > > > fetch > > > > > > > > > and send in correct group. And managing new groups is easy > > for > > > > me I > > > > > > > > think, > > > > > > > > > I would call the method using groupMapper.toEntity(new > > Group(), > > > > > > > > > userDto.getGroup()); > > > > > > > > > > > > > > > > > > > > > > > > > > > I would much prefer all three methods to be non protected. > > > Then I > > > > > > could > > > > > > > > > create my helper something along the lines of this: > > > > > > > > > > > > > > > > > > I did not test this very well but unless I thought wrong > > > > completely > > > > > > it > > > > > > > > > would be a one time thing to implement. But using mappers > > from > > > > > > mappers > > > > > > > > are > > > > > > > > > hard because if the relationship is declared in both ways > you > > > can > > > > > get > > > > > > > > > circular references. Anyways here's my helper I > theorycrafted > > > > > > together: > > > > > > > > > > > > > > > > > > > > > > > > > > > Group g = fetch (new Group(), user.getGroup(), > groupMapper); > > > > > > > > > > > > > > > > > > public <Entity, Dto> Entity fetch(Entity entity, Dto > dto, > > > > > > > > > SimpleQueryInOutMapperBase<Entity, Dto> entityMapper){ > > > > > > > > > Object pk = entityMapper.getPrimaryKey(dto); > > > > > > > > > Entity foundEntity = (Entity) > > > > > > > > entityManager.find(entity.getClass(), > > > > > > > > > pk); > > > > > > > > > > > > > > > > > > if (foundEntity == null) { > > > > > > > > > foundEntity = entity; > > > > > > > > > } > > > > > > > > > return entityMapper.toEntity(foundEntity, dto); > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > One could always create their own base class overriding the > > > > > protected > > > > > > > > > methods and adding that fetch helper I guess... > > > > > > > > > > > > > > > > > > cheers > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 13 June 2014 12:54, Thomas Hug <[email protected]> > > > wrote: > > > > > > > > > > > > > > > > > > > Hey Karl > > > > > > > > > > > > > > > > > > > > Sorry missed your mail indeed - it's probably time I > > > subscribe > > > > to > > > > > > the > > > > > > > > > user > > > > > > > > > > mailing list too :-) > > > > > > > > > > > > > > > > > > > > You can still chain those mappers together, but I agree > the > > > > > casting > > > > > > > and > > > > > > > > > > wiring is clunky. As options I see: > > > > > > > > > > - we add a public E toEntity(Dto dto) back which > basically > > > does > > > > > the > > > > > > > > same > > > > > > > > > as > > > > > > > > > > mapParameter now (just hides the casting) for new > entities > > > > > > > > > > - we make the E toEntity(Entity e, Dto dto) public as > well, > > > so > > > > > it's > > > > > > > > quite > > > > > > > > > > easy to chain mapper calls for updates > > > > > > > > > > > > > > > > > > > > groupMapper.toEntity(user.getGroup(), > userDto.getGroup()); > > > > > > > > > > > > > > > > > > > > You will still have to have some conditionals to see > which > > > one > > > > to > > > > > > > call, > > > > > > > > > > also depends on your data model (required relations, lazy > > or > > > > > eager > > > > > > > > > fetch). > > > > > > > > > > How does that look? Better ideas? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jun 13, 2014 at 8:42 AM, Karl Kildén < > > > > > > [email protected]> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > I wrote a response to the users list but not sure it > came > > > > > > through. > > > > > > > It > > > > > > > > > > kinda > > > > > > > > > > > belongs in this thread so here it goes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So I ran into issues with the DTO mapper api and voiced > > my > > > > > > concerns > > > > > > > > in > > > > > > > > > > irc. > > > > > > > > > > > I saw this discussion and now I am trying the solution > > > > present > > > > > in > > > > > > > the > > > > > > > > > > > current SNAPSHOT. However I have one comment / > question: > > > > > > > > > > > > > > > > > > > > > > What if my Entity has a relationship to another Entity? > > > > > > > > > > > > > > > > > > > > > > Like this: > > > > > > > > > > > > > > > > > > > > > > public class User extends BaseAuditEntity { > > > > > > > > > > > > > > > > > > > > > > @Column > > > > > > > > > > > private String name; > > > > > > > > > > > > > > > > > > > > > > @Column > > > > > > > > > > > @ManyToOne > > > > > > > > > > > @JoinColumn(name="group_id") > > > > > > > > > > > private Group group; > > > > > > > > > > > > > > > > > > > > > > This means my userDTO may come with a GroupDTO and how > > do I > > > > map > > > > > > > that > > > > > > > > > > > relationship? Or to explain by code please fill in the > > > > question > > > > > > > marks > > > > > > > > > > below > > > > > > > > > > > ;) or if you feel my implementation is weird then I > would > > > > > gladly > > > > > > > > accept > > > > > > > > > > > comments on that too. But the way I see it I need to > > > @Inject > > > > > the > > > > > > > > > > > GroupMapper, use the EntityManager to find the Group > then > > > > call > > > > > > > > > > > groupMapper.toEntity and then I think to myself that > the > > > API > > > > is > > > > > > > worse > > > > > > > > > now > > > > > > > > > > > because before I could call groupMapper.toEntity with > > only > > > a > > > > > dto > > > > > > > > > > argument. > > > > > > > > > > > At that point you had to use the entitymanager anyways > to > > > > find > > > > > > > > > "yourself" > > > > > > > > > > > and that feels clean compared to find your entities you > > > form > > > > > > > > > > relationships > > > > > > > > > > > with. > > > > > > > > > > > > > > > > > > > > > > @Override > > > > > > > > > > > protected User toEntity(final User user, final > > UserDTO > > > > > > > userDTO) { > > > > > > > > > > > MapperUtil.toAuditEntity(user, userDTO); > > > > > > > > > > > user.setName(userDTO.getName()); > > > > > > > > > > > user.setEmail(userDTO.getEmail()); > > > > > > > > > > > user.setLocale(userDTO.getLocale()); > > > > > > > > > > > user.setGroup(*?????*); > > > > > > > > > > > return user; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > Cheers / Karl > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 17 May 2014 16:40, Romain Manni-Bucau < > > > > > [email protected]> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Yep, missed it. > > > > > > > > > > > > > > > > > > > > > > > > Works for me. Maybe the name should be closer to > other > > > > > methods, > > > > > > > > > > > > mapKey? But whatever you choose as name this solution > > > works > > > > > :). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Romain Manni-Bucau > > > > > > > > > > > > Twitter: @rmannibucau > > > > > > > > > > > > Blog: http://rmannibucau.wordpress.com/ > > > > > > > > > > > > LinkedIn: http://fr.linkedin.com/in/rmannibucau > > > > > > > > > > > > Github: https://github.com/rmannibucau > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2014-05-17 15:54 GMT+02:00 Thomas Hug < > > > > [email protected] > > > > > >: > > > > > > > > > > > > > It's the PK, not the Entity ;) In > > > > > SimpleQueryInOutMapperBase, > > > > > > > it > > > > > > > > > > could > > > > > > > > > > > be > > > > > > > > > > > > > something like > > > > > > > > > > > > > > > > > > > > > > > > > > @Inject > > > > > > > > > > > > > private QueryInvocationContext context; > > > > > > > > > > > > > > > > > > > > > > > > > > protected abstract Object getPrimaryKey(Dto dto); > > > > > > > > > > > > > > > > > > > > > > > > > > protected E findEntity(Object pk) > > > > > > > > > > > > > { > > > > > > > > > > > > > return (E) > > > > > > > > > > > > context.getEntityManager().find(context.getEntityClass(), > > > > > > > > > > > > > pk); > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > @Override > > > > > > > > > > > > > public Object mapParameter(final Object parameter) > > > > > > > > > > > > > { > > > > > > > > > > > > > Object pk = getPrimaryKey((Dto) parameter); > > > > > > > > > > > > > if (pk != null) > > > > > > > > > > > > > { > > > > > > > > > > > > > E entity = findEntity(pk); > > > > > > > > > > > > > return toEntity(entity, (Dto) parameter); > > > > > > > > > > > > > } > > > > > > > > > > > > > return toEntity(newEntity(), (Dto) parameter); > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sat, May 17, 2014 at 1:57 PM, Romain Manni-Bucau > > > > > > > > > > > > > <[email protected]>wrote: > > > > > > > > > > > > > > > > > > > > > > > > > >> would work while return is <E> and not Object ;) > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> Romain Manni-Bucau > > > > > > > > > > > > >> Twitter: @rmannibucau > > > > > > > > > > > > >> Blog: http://rmannibucau.wordpress.com/ > > > > > > > > > > > > >> LinkedIn: http://fr.linkedin.com/in/rmannibucau > > > > > > > > > > > > >> Github: https://github.com/rmannibucau > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> 2014-05-17 11:47 GMT+02:00 Thomas Hug < > > > > > [email protected] > > > > > > >: > > > > > > > > > > > > >> > Or a protected abstract Object getPrimaryKey(Dto > > > dto). > > > > > We > > > > > > > can > > > > > > > > > get > > > > > > > > > > > the > > > > > > > > > > > > EM > > > > > > > > > > > > >> > over an injected QueryInvocationContext. > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > On Thu, May 15, 2014 at 9:06 PM, Romain > > Manni-Bucau > > > > > > > > > > > > >> > <[email protected]>wrote: > > > > > > > > > > > > >> > > > > > > > > > > > > > >> >> I think a protected findEntity(id) in the > mapper > > > can > > > > be > > > > > > > > enough. > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> >> Romain Manni-Bucau > > > > > > > > > > > > >> >> Twitter: @rmannibucau > > > > > > > > > > > > >> >> Blog: http://rmannibucau.wordpress.com/ > > > > > > > > > > > > >> >> LinkedIn: > http://fr.linkedin.com/in/rmannibucau > > > > > > > > > > > > >> >> Github: https://github.com/rmannibucau > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> >> 2014-05-07 22:29 GMT+02:00 Thomas Hug < > > > > > > > [email protected] > > > > > > > > >: > > > > > > > > > > > > >> >> > Hi Romain, > > > > > > > > > > > > >> >> > See your point. But if we only get the DTO - > > with > > > > > what > > > > > > > > would > > > > > > > > > we > > > > > > > > > > > > call > > > > > > > > > > > > >> the > > > > > > > > > > > > >> >> > find? Could even be that the PK is a DTO or > > > > encoded / > > > > > > > > > encrypted > > > > > > > > > > > and > > > > > > > > > > > > >> needs > > > > > > > > > > > > >> >> > to go through the mapper first. Maybe we can > > > > provide > > > > > > some > > > > > > > > > > > > convenience > > > > > > > > > > > > >> >> > methods in the base mapper? > > > > > > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > >> >> > On Tue, May 6, 2014 at 7:41 PM, Romain > > > Manni-Bucau > > > > < > > > > > > > > > > > > >> >> [email protected]>wrote: > > > > > > > > > > > > >> >> > > > > > > > > > > > > > >> >> >> Hi guys, > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> DTO feature is awesome but doesn't work in > > > update > > > > > mode > > > > > > > > since > > > > > > > > > > > isNew > > > > > > > > > > > > >> >> >> doesn't use a managed entity. > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> When using a mapper we should call find and > > pass > > > > it > > > > > to > > > > > > > the > > > > > > > > > > > mapper > > > > > > > > > > > > (or > > > > > > > > > > > > >> >> >> create a new unmanaged entity if not found). > > So > > > > > mapper > > > > > > > > > > signature > > > > > > > > > > > > >> >> >> should be Entity toEntity(Entity, DTO) no? > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> Otherwise users need to do the find in the > > > > > > > mapper...almost > > > > > > > > > > > > eveytime. > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> wdyt? > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> >> Romain Manni-Bucau > > > > > > > > > > > > >> >> >> Twitter: @rmannibucau > > > > > > > > > > > > >> >> >> Blog: http://rmannibucau.wordpress.com/ > > > > > > > > > > > > >> >> >> LinkedIn: > > http://fr.linkedin.com/in/rmannibucau > > > > > > > > > > > > >> >> >> Github: https://github.com/rmannibucau > > > > > > > > > > > > >> >> >> > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
