From 9843447a5838fa4b3a1ab5a64dc12cad4ac3fe7b Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 14 Aug 2013 09:18:26 +0200 Subject: [PATCH] DATACMNS-357 - Cleanups in primitive id detection. Changed implementation in AbstractEntityInformation quite a bit to not need the non-final fields and a protected callback method. Cleaned up test cases to really test the new behavior, not the artificially introduced test helper class. Original pull request: #37. --- .../support/AbstractEntityInformation.java | 45 +++---- .../AbstractEntityInformationUnitTests.java | 123 ++++++++++-------- .../support/DummyEntityAndIdInformation.java | 57 -------- .../ReflectionEntityInformationUnitTests.java | 26 +++- 4 files changed, 109 insertions(+), 142 deletions(-) delete mode 100644 src/test/java/org/springframework/data/repository/core/support/DummyEntityAndIdInformation.java diff --git a/src/main/java/org/springframework/data/repository/core/support/AbstractEntityInformation.java b/src/main/java/org/springframework/data/repository/core/support/AbstractEntityInformation.java index 3849e5395..e31d13d0a 100644 --- a/src/main/java/org/springframework/data/repository/core/support/AbstractEntityInformation.java +++ b/src/main/java/org/springframework/data/repository/core/support/AbstractEntityInformation.java @@ -31,13 +31,10 @@ public abstract class AbstractEntityInformation impl private final Class domainClass; - private boolean idTypePrimitiveSet; - private boolean idTypePrimitive; - /** * Creates a new {@link AbstractEntityInformation} from the given domain class. * - * @param domainClass + * @param domainClass must not be {@literal null}. */ public AbstractEntityInformation(Class domainClass) { @@ -46,36 +43,30 @@ public abstract class AbstractEntityInformation impl } /* - * (non-Javadoc) - * - * @see - * org.springframework.data.repository.support.IsNewAware#isNew(java.lang - * .Object) - */ + * (non-Javadoc) + * @see org.springframework.data.repository.core.EntityInformation#isNew(java.lang.Object) + */ public boolean isNew(T entity) { ID id = getId(entity); - return id == null || this.isIdTypePrimitive() && id instanceof Number && ((Number) id).longValue() == 0; + Class idType = getIdType(); + + if (!idType.isPrimitive()) { + return id == null; + } + + if (id instanceof Number) { + return ((Number) id).longValue() == 0L; + } + + throw new IllegalArgumentException(String.format("Unsupported primitive id type %s!", idType)); } /* - * (non-Javadoc) - * - * @see - * org.springframework.data.repository.support.EntityInformation#getJavaType - * () - */ + * (non-Javadoc) + * @see org.springframework.data.repository.core.EntityMetadata#getJavaType() + */ public Class getJavaType() { - return this.domainClass; } - - protected boolean isIdTypePrimitive() { - if (!this.idTypePrimitiveSet) { - this.idTypePrimitive = getIdType() != null && getIdType().isPrimitive(); - this.idTypePrimitiveSet = true; - } - - return this.idTypePrimitive; - } } diff --git a/src/test/java/org/springframework/data/repository/core/support/AbstractEntityInformationUnitTests.java b/src/test/java/org/springframework/data/repository/core/support/AbstractEntityInformationUnitTests.java index 54ec1c62a..32c63597a 100644 --- a/src/test/java/org/springframework/data/repository/core/support/AbstractEntityInformationUnitTests.java +++ b/src/test/java/org/springframework/data/repository/core/support/AbstractEntityInformationUnitTests.java @@ -20,9 +20,13 @@ import static org.junit.Assert.*; import java.io.Serializable; +import org.junit.Rule; import org.junit.Test; -import org.springframework.data.domain.Persistable; +import org.junit.rules.ExpectedException; +import org.springframework.data.annotation.Id; import org.springframework.data.repository.core.EntityInformation; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.util.ReflectionUtils; /** * Unit tests for {@link AbstractEntityInformation}. @@ -32,6 +36,8 @@ import org.springframework.data.repository.core.EntityInformation; */ public class AbstractEntityInformationUnitTests { + @Rule public ExpectedException exception = ExpectedException.none(); + @Test(expected = IllegalArgumentException.class) public void rejectsNullDomainClass() throws Exception { @@ -46,78 +52,85 @@ public class AbstractEntityInformationUnitTests { assertThat(metadata.isNew(new Object()), is(false)); } - // See DATACMNS-357 + /** + * @see DATACMNS-357 + */ @Test - public void considersEntityNewIfGetIdReturnsNullAndIdTypeIsPrimitiveNumber() throws Exception { + public void detectsNewStateForPrimitiveIds() { - EntityInformation metadata1 = - new DummyEntityAndIdInformation(LongIdEntity.class, long.class); - assertThat(metadata1.isNew(null), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(null)), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(-1L)), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(0L)), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(1L)), is(false)); + FooEn fooEn = new FooEn(PrimitiveIdEntity.class); - EntityInformation metadata2 = - new DummyEntityAndIdInformation(IntIdEntity.class, int.class); - assertThat(metadata2.isNew(null), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(null)), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(-1)), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(0)), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(1)), is(false)); + PrimitiveIdEntity entity = new PrimitiveIdEntity(); + assertThat(fooEn.isNew(entity), is(true)); + + entity.id = 5L; + assertThat(fooEn.isNew(entity), is(false)); } - // See DATACMNS-357 + /** + * @see DATACMNS-357 + */ @Test - public void considersEntityNewIfGetIdReturnsNullAndIdTypeIsPrimitiveWrapperNumber() throws Exception { + public void detectsNewStateForPrimitiveWrapperIds() { - EntityInformation metadata1 = - new DummyEntityAndIdInformation(LongIdEntity.class, Long.class); - assertThat(metadata1.isNew(null), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(null)), is(true)); - assertThat(metadata1.isNew(new LongIdEntity(-1L)), is(false)); - assertThat(metadata1.isNew(new LongIdEntity(0L)), is(false)); - assertThat(metadata1.isNew(new LongIdEntity(1L)), is(false)); + FooEn fooEn = new FooEn( + PrimitiveWrapperIdEntity.class); - EntityInformation metadata2 = - new DummyEntityAndIdInformation(IntIdEntity.class, Integer.class); - assertThat(metadata2.isNew(null), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(null)), is(true)); - assertThat(metadata2.isNew(new IntIdEntity(-1)), is(false)); - assertThat(metadata2.isNew(new IntIdEntity(0)), is(false)); - assertThat(metadata2.isNew(new IntIdEntity(1)), is(false)); + PrimitiveWrapperIdEntity entity = new PrimitiveWrapperIdEntity(); + assertThat(fooEn.isNew(entity), is(true)); + + entity.id = 5L; + assertThat(fooEn.isNew(entity), is(false)); } - private static abstract class NumberIdEntity implements Persistable { + /** + * @see DATACMNS-357 + */ + @Test + public void rejectsUnsupportedPrimitiveIdType() { - private final T id; + FooEn information = new FooEn( + UnsupportedPrimitiveIdEntity.class); - public NumberIdEntity(T id) { - this.id = id; + exception.expect(IllegalArgumentException.class); + exception.expectMessage(boolean.class.getName()); + information.isNew(new UnsupportedPrimitiveIdEntity()); + } + + static class PrimitiveIdEntity { + + @Id long id; + } + + static class PrimitiveWrapperIdEntity { + + @Id Long id; + } + + static class UnsupportedPrimitiveIdEntity { + + @Id boolean id; + } + + static class FooEn extends AbstractEntityInformation { + + private final Class type; + + private FooEn(Class type) { + super(type); + this.type = type; } @Override - public T getId() { - return this.id; + @SuppressWarnings("unchecked") + public ID getId(T entity) { + return (ID) ReflectionTestUtils.getField(entity, "id"); } @Override - public boolean isNew() { - throw new UnsupportedOperationException(); - } - } - - private static final class LongIdEntity extends NumberIdEntity { - - public LongIdEntity(Long id) { - super(id); - } - } - - private static final class IntIdEntity extends NumberIdEntity { - - public IntIdEntity(Integer id) { - super(id); + @SuppressWarnings("unchecked") + public Class getIdType() { + return (Class) ReflectionUtils.findField(type, "id").getType(); } } } diff --git a/src/test/java/org/springframework/data/repository/core/support/DummyEntityAndIdInformation.java b/src/test/java/org/springframework/data/repository/core/support/DummyEntityAndIdInformation.java deleted file mode 100644 index 1400134ef..000000000 --- a/src/test/java/org/springframework/data/repository/core/support/DummyEntityAndIdInformation.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Copyright 2011-2013 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 - * - * http://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.repository.core.support; - -import org.springframework.data.domain.Persistable; - -import java.io.Serializable; - -/** - * Dummy implementation of {@link AbstractEntityInformation} that also provides ID information. - * - * @author Nick Williams - */ -public class DummyEntityAndIdInformation, ID extends Serializable> - extends AbstractEntityInformation { - - private final Class idClass; - - /** - * Creates a new {@link DummyEntityInformation} for the given domain class. - * - * @param domainClass - */ - public DummyEntityAndIdInformation(Class domainClass, Class idClass) { - super(domainClass); - this.idClass = idClass; - } - - /* - * (non-Javadoc) - * @see org.springframework.data.repository.core.EntityInformation#getId(java.lang.Object) - */ - public ID getId(T entity) { - return entity == null ? null : entity.getId(); - } - - /* - * (non-Javadoc) - * @see org.springframework.data.repository.core.EntityInformation#getIdType() - */ - public Class getIdType() { - return this.idClass; - } -} diff --git a/src/test/java/org/springframework/data/repository/core/support/ReflectionEntityInformationUnitTests.java b/src/test/java/org/springframework/data/repository/core/support/ReflectionEntityInformationUnitTests.java index 5566fbda1..7b3fb557c 100644 --- a/src/test/java/org/springframework/data/repository/core/support/ReflectionEntityInformationUnitTests.java +++ b/src/test/java/org/springframework/data/repository/core/support/ReflectionEntityInformationUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012 the original author or authors. + * Copyright 2012-2013 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may @@ -48,18 +48,38 @@ public class ReflectionEntityInformationUnitTests { getEntityInformation(Unannotated.class); } + /** + * @see DATACMNS-357 + */ + @Test + public void detectsNewStateForEntitiesWithPrimitiveIds() { + + PrimitiveId primitiveId = new PrimitiveId(); + + EntityInformation information = new ReflectionEntityInformation( + PrimitiveId.class); + assertThat(information.isNew(primitiveId), is(true)); + + primitiveId.id = 5L; + assertThat(information.isNew(primitiveId), is(false)); + } + private static EntityInformation getEntityInformation(Class type) { return new ReflectionEntityInformation(type, Id.class); } static class Sample { - @Id - String id; + @Id String id; } static class Unannotated { String id; } + + static class PrimitiveId { + + @Id long id; + } }