Skip to content

JpaCrudService: no semantic enforcement between save() and update() #113

@javier-godoy

Description

@javier-godoy

Problem

JpaCrudService.save() and update() both delegate to the same CrudRepository.save() without checking the entity's ID state, so neither enforces its own semantic contract:

  • save(entity) is documented as creating a new entity (CreationService contract: "saves a new entity to the database") and runs CreationValidator. However, if an entity with an existing ID is passed, it silently performs an update while CreationValidator was already executed — bypassing the intended validator semantics.
  • update(entity) is documented as updating an existing entity (UpdateService contract: "updates the given entity") and runs UpdateValidator. However, if an entity with a null ID is passed, it silently performs an insert while UpdateValidator was already executed — again bypassing the intended validator semantics.

Expected Behavior

  • save() should throw IllegalArgumentException if the entity already has an ID set.
  • update() should throw IllegalArgumentException if the entity has a null ID.

Precedent

The DAO layer already enforces the update() contract. ConversionJpaDaoSupport.update() (line 80) does:

Objects.requireNonNull(persistentEntity.getId(), "id");

JpaCrudService should apply the same enforcement at the service layer for both methods.

Suggested Fix

@Override
public K save(T entity) {
    K id = getId(entity);
    if (id != null) {
        throw new IllegalArgumentException("Cannot save entity that already has an id: " + id);
    }
    validate(CreationValidator.class, entity, CreationValidationException::new);
    return getId(getCrudRepository().save(entity));
}

@Override
public void update(T entity) {
    K id = getId(entity);
    if (id == null) {
        throw new IllegalArgumentException("Cannot update entity without an id");
    }
    validate(UpdateValidator.class, entity, UpdateValidationException::new);
    getCrudRepository().save(entity);
}

No new imports are needed. The existing getId() reflection helper (line 45) is reused as-is, and the check is placed before validate() since it is a structural pre-condition rather than a business rule.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions