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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions