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
> > > > > > > >> >> >>
> > > > > > > >> >>
> > > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
