4
\$\begingroup\$

I would like to know if the solution I came up with for managing relationships between entities is sensible. Guys in my team feel it is transparent, but maybe I'm missing a pitfall somewhere?

Consider the following situation:

@Entity
public class User {

    @OneToMany(mappedBy = "user", cascade = ALL, orphanRemoval = true)
    private List<Permission> permissions = new Arraylist();

    // getters and setters...

}


@Entity
public class Permission {

    @ManyToOne(fetch = LAZY, cascade = {PERSIST, MERGE, REFRESH})
    @JoinColumn(name = "user_id", nullable = false)
    private User user;

    // getters and setters...

}


@Service
public class UserService {

    // Just an interface extending JpaRepository<User, Long>
    private final UserRepository repository;
    private final PermissionService permissionService;

    @Inject
    public UserService(UserRepository repository, PermissionService permissionService) {
        this.repository = repository;
        this.permissionService = permissionService;
    }

    // Represents an update scenario
    public User save(User user) {

        // Find managed copy of the user
        User managed = repository.findOne(user.getId());

        // Ensure the user entity's permissions are consistent
        if(isNotEmpty(user.getPermissions()) {

            List<Permission> detached = user.getPermissions();
            List<Permission> attached = new ArrayList();

            for(Permission permission : detached) {
                attached.add(permissionService.get(permission.getId());
            }

            managed.setPermissions(attached);
        }

        user = managed;

        return repository.save(user);

    }

    // other service methods...

}


@Service
public class PermissionService {

    private final PermissionRepository repository;
    private final UserService userService;

    @Inject
    public PermissionService(PermissionRepository repository, UserService userService) {
        this.repository = repository;
        this.userService = userService;
    }

    public User save(Permission permission) {

        // Similar code to UserService.save(User) employing UserService.get(Long id) to find the required user

    }

    // other service methods...

}

As you can see there is a circular dependency between the services. It is necessary for updating a user, for example, because the user to be saved must have permission entities which are managed by the entity manager. One could argue that setter injection could "solve" this, but it is not a real solution as the service should be constructed completely.

Here is the idea: using property change listeners to manage entity attached/detached state in a transparent manner.

Step 1: Add property support to the Entity:

@Entity
public class User {

    // same as before plus:

    @Transient
    private PropertyChangeSupport support = new PropertyChangeSupport(this);

    public synchronized void addPropertyChangeListener(PropertyChangeListener listener) {
        this.support.addPropertyChangeListener(listener);
    }

    public void setPermissions(List<UserPermission> permissions) {

        List<UserPermission> old = new ArrayList<>(this.permissions);
        this.permissions.clear();

        for(UserPermission permission : permissions) {
            addPermission(permission);
        }

        support.firePropertyChange("permissions", old, permissions);

    }

    public void addPermission(Permission permission) {
        this.permissions.add(permission);

        if(permission.getUser() != this) {
            permission.setUser(this);
        }
    }

}

In UserService:

@Service
public class UserService {

    // Just an interface extending JpaRepository<User, Long>
    private final UserRepository repository;
    private final ApplicationContext context;

    @Inject
    public UserService(UserRepository repository, ApplicationContext context) {
        this.repository = repository;
        this.context = context;
    }

    // Represents an update scenario
    public User save(User user) {

        // Find managed copy of the user
        User managed = repository.findOne(user.getId());
        // Add a property listener for Permissions
        managed.addPropertyChangeListener(context.getBean(PermissionChangeListener.class));

        managed.setPermissions(user.getPermissions());

        user = managed;

        return repository.save(user);

    }

    // other service methods...

}

And the new PermissionChangeListener class which will be responsible with managing whether the entity is managed by the entity manager:

// An incomplete example which simply checks if there are any unmanaged Permissions in the source User, if there are then retrieve those and attach them to the user
@Component
public class PermissionChangeListener implements PropertyChangeListener {

    private static final Logger logger = LoggerFactory.getLogger(PermissionChangeListener.class);

    @PersistenceContext
    private final EntityManager manager;
    private final PermissionService service;

    @Inject
    public PermissionChangeListener(EntityManager manager, PermissionService service) {
        this.manager = manager;
        this.service = service;
    }

    @Override
    public void propertyChange(PropertyChangeEvent event) {

        if("permissions".equals(event.getPropertyName())) {
            setPermissions(event);
        }

    }

    private void setPermissions(PropertyChangeEvent event) {

        List<Permission> values = (List<Permission>) event.getNewValue();

        User user = (User) event.getSource();

        if(user != null && CollectionUtils.isNotEmpty(values) && !isManaged(values)) {

            // Set the permissions to empty to clear permissions in ORM
            user.setPermissions(new ArrayList<>(0));

            List<Permission> managed = new ArrayList<>(values.size());

            for(int i = 0; i < values.size(); i++) {

                Permission value = values.get(i);

                logger.debug("[{}]:{}", (manager.contains(value)) ? "attached" : "detached", value);
                if(!isManaged(value)) {
                    value = service.get(value.getId());
                }

                managed.add(value);

            }

            user.setPermissions(managed);

        }

    }

    private Boolean isManaged(Permission permission) {
        return permission != null && manager.contains(permission);
    }

    private Boolean isManaged(List<Permission> permissions) {

        for(Permission permission : permissions) {

            if(!isManaged(permission)) {
                return false;
            }
        }

        return true;
    }

}

As you can see we seem to have a few wins:

  1. The service code becomes much simpler;
  2. The management of ORM functionality is collected in a single place;
  3. Different relationships are encapsulated into their own ChangeListeners;
  4. We solve the circular dependecy Constructor Injection on the service level by following the advise here: Circular Dependency in constructors and Dependency Injection;
  5. We avoid setter injection which is completely unecessary here as the service should not be reconfigured at runtime as discussed here: Why I Changed My Mind About Field Injection?;
  6. PermissionChangeListener can be further improved to deal with single addition of permissions (User.addPermission(Permission)) and other property changes (removing permissions, etc...). It could also create Permissions on-the-fly if the permission does not exist in the database.
  7. The ChangeListener can be further improved with Typed Change Events.

I would like to know if the idea is coherent and makes sense (from my tests it seems to be working), if I should be synchronizing the User's setters as I'm now updating properties in the listener and if there would be a problem with multiple entity updates (they should all be happening in transactions I guess).

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

The problem is not in your Services. The problem is the circular dependency you have modeled from Permissions to Users:

if (permission.getUser() != this) {
    permission.setUser(this);
}

This circular dependency permeates into your Services, which is completely understandable. But to resolve it you'll need to go one level of abstraction deeper.
Overall what you seem to have here is a Many-to-Many relationship gone awry.

If you want allow a User to have multiple Permissions and allow tracing Permissions back to users you'll have to work with a mapping table. Unfortunately from reading your code I can't quite discern whether I'm correct, accordingly this is ... guesswork.


Nitpicks:

  • You should return primitives instead of objects where possible, because it makes it abundantly clear that a method cannot return null (relevant to isManaged)
  • You are using copious amounts of whitespace. I personally dislike code having overly much whitespace and I'd remove about two thirds of the empty lines you have there.
  • Use an enhanced for-loop in PermissionChangeListener#setPermissions. you're not using the index for anything but retrieving the element. It's simpler to use higher-level constructs.
  • You're not using isManaged(List<Permission>) anywhere in the code you've shown.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.