Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve detection of entity state #3604

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,13 @@
* @author Mark Paluch
* @author Jens Schauder
* @author Greg Turnquist
* @author Yanming Zhou
*/
public class JpaMetamodelEntityInformation<T, ID> extends JpaEntityInformationSupport<T, ID> {

private final IdMetadata<T> idMetadata;
private final Optional<SingularAttribute<? super T, ?>> versionAttribute;
private final Metamodel metamodel;
private final PersistenceProvider persistenceProvider;
private final @Nullable String entityName;
private final PersistenceUnitUtil persistenceUnitUtil;

Expand All @@ -77,7 +78,7 @@ public JpaMetamodelEntityInformation(Class<T> domainClass, Metamodel metamodel,
super(domainClass);

Assert.notNull(metamodel, "Metamodel must not be null");
this.metamodel = metamodel;
this.persistenceProvider = PersistenceProvider.fromMetamodel(metamodel);

ManagedType<T> type = metamodel.managedType(domainClass);

Expand All @@ -91,7 +92,7 @@ public JpaMetamodelEntityInformation(Class<T> domainClass, Metamodel metamodel,
throw new IllegalArgumentException("The given domain class does not contain an id attribute");
}

this.idMetadata = new IdMetadata<>(identifiableType, PersistenceProvider.fromMetamodel(metamodel));
this.idMetadata = new IdMetadata<>(identifiableType, persistenceProvider);
this.versionAttribute = findVersionAttribute(identifiableType, metamodel);

Assert.notNull(persistenceUnitUtil, "PersistenceUnitUtil must not be null");
Expand Down Expand Up @@ -148,8 +149,6 @@ public String getEntityName() {
public ID getId(T entity) {

// check if this is a proxy. If so use Proxy mechanics to access the id.
PersistenceProvider persistenceProvider = PersistenceProvider.fromMetamodel(metamodel);

if (persistenceProvider.shouldUseAccessorFor(entity)) {
return (ID) persistenceProvider.getIdentifierFrom(entity);
}
Expand Down Expand Up @@ -219,13 +218,37 @@ public Object getCompositeIdAttributeValue(Object id, String idAttribute) {
@Override
public boolean isNew(T entity) {

if (versionAttribute.isEmpty()
|| versionAttribute.map(Attribute::getJavaType).map(Class::isPrimitive).orElse(false)) {
if (versionAttribute.isEmpty()) {
return super.isNew(entity);
}

BeanWrapper wrapper = new DirectFieldAccessFallbackBeanWrapper(entity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work when entity is a proxy?

I'd like an integration test, that ensures it has a proxy an based on that tests the isNew method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why it's related to proxy, could you expand?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entity might be a Hibernate proxy. And I basically don't trust any code to properly work with those. Just the normal paranoia after working with JPA to much.

Copy link
Contributor Author

@quaff quaff Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not expert of proxy, as far as I know, proxy only take care of accessing lazy attributes and associations, version is not in that case, and this PR only reuse the existing code wrapper.getPropertyValue(it.getName()) to get value of version to refine the logic.


if (versionAttribute.map(Attribute::getJavaType).map(Class::isPrimitive).orElse(false)) {
return versionAttribute.map(it -> {
Object version = wrapper.getPropertyValue(it.getName());
if (version instanceof Number value) {
if (persistenceProvider == PersistenceProvider.HIBERNATE) {
// Hibernate use 0 or user provided positive initial value as seed of integer version
if (value.longValue() < 0) {
// see org.hibernate.engine.internal.Versioning#isNullInitialVersion()
return true;
}
// TODO Compare version to initial value (field value of transient entity)
// It's unknown if equals because entity maybe transient or just persisted
// But it's absolute not new if not equals
} else if (persistenceProvider == PersistenceProvider.ECLIPSELINK) {
// EclipseLink always use 1 as seed of integer version
if (value.longValue() < 1) {
return true;
}
// It's unknown because the value may be initial value or persisted entity version
}
}
return super.isNew(entity);
}).orElseThrow(); // should never throw
}

return versionAttribute.map(it -> wrapper.getPropertyValue(it.getName()) == null).orElse(true);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright 2013-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.springframework.data.jpa.domain.sample;

import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.Version;

/**
* @author Yanming Zhou
*/
@Entity
public class SampleWithPrimitiveVersion {

@Id private Long id;

@Version private long version = -1;

public void setId(Long id) {
this.id = id;
}

public long getVersion() {
return version;
}

public void setVersion(long version) {
this.version = version;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.data.jpa.domain.AbstractPersistable;
import org.springframework.data.jpa.domain.sample.SampleWithPrimitiveVersion;
import org.springframework.data.repository.core.EntityInformation;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;

Expand All @@ -33,6 +35,7 @@
* @author Oliver Gierke
* @author Jens Schauder
* @author Greg Turnquist
* @author Yanming Zhou
*/
@ExtendWith(SpringExtension.class)
@ContextConfiguration({ "classpath:infrastructure.xml", "classpath:eclipselink.xml" })
Expand Down Expand Up @@ -79,4 +82,21 @@ void correctlyDeterminesIdValueForNestedIdClassesWithNonPrimitiveNonManagedType(
@Override
@Disabled
void prefersPrivateGetterOverFieldAccess() {}

@Override
@Disabled
// superseded by #nonPositiveVersionedEntityIsNew()
void considersEntityAsNotNewWhenHavingIdSetAndUsingPrimitiveTypeForVersionProperty() {}

@Test
void nonPositiveVersionedEntityIsNew() {
EntityInformation<SampleWithPrimitiveVersion, Long> information = new JpaMetamodelEntityInformation<>(SampleWithPrimitiveVersion.class,
em.getMetamodel(), em.getEntityManagerFactory().getPersistenceUnitUtil());

SampleWithPrimitiveVersion entity = new SampleWithPrimitiveVersion();
entity.setId(23L); // assigned
assertThat(information.isNew(entity)).isTrue();
entity.setVersion(0);
assertThat(information.isNew(entity)).isTrue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,19 @@

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.data.jpa.domain.sample.SampleWithPrimitiveVersion;
import org.springframework.data.jpa.util.DisabledOnHibernate61;
import org.springframework.data.repository.core.EntityInformation;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;

import static org.assertj.core.api.Assertions.assertThat;

/**
* Hibernate execution for {@link JpaMetamodelEntityInformationIntegrationTests}.
*
* @author Greg Turnquist
* @author Yanming Zhou
*/
@ExtendWith(SpringExtension.class)
@ContextConfiguration("classpath:infrastructure.xml")
Expand Down Expand Up @@ -55,4 +60,14 @@ void prefersPrivateGetterOverFieldAccess() {
void findsIdClassOnMappedSuperclass() {
super.findsIdClassOnMappedSuperclass();
}

@Test
void negativeVersionedEntityIsNew() {
EntityInformation<SampleWithPrimitiveVersion, Long> information = new JpaMetamodelEntityInformation<>(SampleWithPrimitiveVersion.class,
em.getMetamodel(), em.getEntityManagerFactory().getPersistenceUnitUtil());

SampleWithPrimitiveVersion entity = new SampleWithPrimitiveVersion();
entity.setId(23L); // assigned
assertThat(information.isNew(entity)).isTrue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
<class>org.springframework.data.jpa.domain.sample.SampleEntityPK</class>
<class>org.springframework.data.jpa.domain.sample.SampleWithIdClass</class>
<class>org.springframework.data.jpa.domain.sample.SampleWithPrimitiveId</class>
<class>org.springframework.data.jpa.domain.sample.SampleWithPrimitiveVersion</class>
<class>org.springframework.data.jpa.domain.sample.SampleWithTimestampVersion</class>
<class>org.springframework.data.jpa.domain.sample.Site</class>
<class>org.springframework.data.jpa.domain.sample.SpecialUser</class>
Expand Down