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 <rmannibu...@gmail.com> wrote:
> Yes you need to merge it but the responsability is yours (user) IMO.
> Le 16 juin 2014 09:56, "Karl Kildén" <karl.kil...@gmail.com> 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 <rmannibu...@gmail.com> 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" <
> andraschko.tho...@gmail.com>
> > 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 <karl.kil...@gmail.com>:
> > > >
> > > > > 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 <thomas....@gmail.com> 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 <
> > karl.kil...@gmail.com>
> > > > > > 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 <thomas....@gmail.com>
> 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 <
> > > > karl.kil...@gmail.com>
> > > > > > > > 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 <thomas....@gmail.com>
> > > 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 <
> > > > > > karl.kil...@gmail.com>
> > > > > > > > > > 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 <
> > > > > rmannibu...@gmail.com>
> > > > > > > > > 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 <
> > > > thomas....@gmail.com
> > > > > >:
> > > > > > > > > > > > > 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
> > > > > > > > > > > > > <rmannibu...@gmail.com>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 <
> > > > > thomas....@gmail.com
> > > > > > >:
> > > > > > > > > > > > >> > 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
> > > > > > > > > > > > >> > <rmannibu...@gmail.com>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 <
> > > > > > > thomas....@gmail.com
> > > > > > > > >:
> > > > > > > > > > > > >> >> > 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
> > > > <
> > > > > > > > > > > > >> >> rmannibu...@gmail.com>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
> > > > > > > > > > > > >> >> >>
> > > > > > > > > > > > >> >>
> > > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
