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