spring-projects / spring-data-jpa

Simplifies the development of creating a JPA-based data access layer.
https://spring.io/projects/spring-data-jpa/
Apache License 2.0
3.01k stars 1.42k forks source link

Specification toPredicate usage is violated in SimpleJpaRepository::delete #3036

Closed aryu-ki closed 1 year ago

aryu-ki commented 1 year ago
        @Override
    public long delete(Specification<T> spec) {

        CriteriaBuilder builder = this.em.getCriteriaBuilder();
        CriteriaDelete<T> delete = builder.createCriteriaDelete(getDomainClass());

        Predicate predicate = spec.toPredicate(delete.from(getDomainClass()), null, builder);

        if (predicate != null) {
            delete.where(predicate);
        }

        return this.em.createQuery(delete).executeUpdate();
    }

This is the definition of delete method in SimpleJpaRepository. Clearly, query param sent is null, but in Specification::toPredicate we have this doc:

         /**
     * Creates a WHERE clause for a query of the referenced entity in form of a {@link Predicate} for the given
     * {@link Root} and {@link CriteriaQuery}.
     *
     * @param root must not be {@literal null}.
     * @param query must not be {@literal null}.
     * @param criteriaBuilder must not be {@literal null}.
     * @return a {@link Predicate}, may be {@literal null}.
     */
    @Nullable
    Predicate toPredicate(Root<T> root, CriteriaQuery<?> query, CriteriaBuilder criteriaBuilder);
mp911de commented 1 year ago

You're right. This is because CriteriaDelete is not a CriteriaQuery. See also #2936

aryu-ki commented 1 year ago

@mp911de You should change documentation string of toPredicate if I'm right, because it's a confusing doc. I could find the issue fast, but in my code, there were no obvious signs of what was going on. Other developers might get confused as well. Who knows how much time is it gonna cost to someone. It's obvious that documentation is misleading! At least change it to 'should' instead of 'must' or provide some additional docs on exceptional cases

ghostg00 commented 10 months ago

I also think that toPredicate's method documentation should be changed, It does cause confusion