From 48120151fd4fa33c472e96417dcc287d9cf9cc38 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 3 May 2017 16:24:42 +0200 Subject: [PATCH] DATACASS-367 - Drop Serializable requirement on Id types. Id types for entities are no longer required to implement Serializable. This requirement originates from ancient ORM patterns and had never a technical requirements for Cassandra. Id types are required to map either directly or convert to a Cassandra-supported data type. --- .../convert/MappingCassandraConverter.java | 3 +- .../data/cassandra/repository/MapId.java | 3 +- .../support/IdInterfaceException.java | 29 ++++++++- .../support/IdInterfaceExceptions.java | 38 ++++++++--- .../support/IdInterfaceValidator.java | 65 ++++++++++--------- .../repository/support/MapIdFactory.java | 34 +++++----- .../support/MapIdProxyDelegate.java | 8 +-- .../support/MapIdFactoryUnitTests.java | 35 +++------- 8 files changed, 119 insertions(+), 96 deletions(-) diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java index 03072004e..4205f5f52 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java @@ -19,7 +19,6 @@ import static org.springframework.data.cassandra.repository.support.BasicMapId.* import lombok.AllArgsConstructor; -import java.io.Serializable; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -540,7 +539,7 @@ public class MappingCassandraConverter extends AbstractCassandraConverter entity.getPersistentProperties() // .filter(CassandraPersistentProperty::isPrimaryKeyColumn) // .forEach(property -> { - id.with(property.getName(), (Serializable) getWriteValue(property, accessor).orElse(null)); + id.with(property.getName(), getWriteValue(property, accessor).orElse(null)); }); return id; diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/MapId.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/MapId.java index 36f481ab7..9d7855186 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/MapId.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/MapId.java @@ -15,7 +15,6 @@ */ package org.springframework.data.cassandra.repository; -import java.io.Serializable; import java.util.Map; /** @@ -25,7 +24,7 @@ import java.util.Map; * @author Matthew T. Adams * @author Mark Paluch */ -public interface MapId extends Serializable, Map { +public interface MapId extends Map { /** * Builder method that adds the value for the named property, then returns {@code this}. diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceException.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceException.java index b0abeb8d2..3aaa765d3 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceException.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceException.java @@ -1,13 +1,36 @@ +/* + * Copyright 2017 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.cassandra.repository.support; import java.lang.reflect.Method; -public class IdInterfaceException extends RuntimeException { +import org.springframework.data.mapping.model.MappingException; + +/** + * Exception thrown on incorrect mapping of an Id interface. + * + * @author Matthew T. Adams + * @author Mark Paluch + */ +public class IdInterfaceException extends MappingException { private static final long serialVersionUID = -1635695314254522703L; - String idInterfaceName; - String method; + private final String idInterfaceName; + private final String method; public IdInterfaceException(Class idInterface, Method method, String message) { this(idInterface.getClass().getName(), method == null ? null : method.toString(), message); diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceExceptions.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceExceptions.java index 13b32a40e..d9ea5829a 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceExceptions.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceExceptions.java @@ -16,23 +16,43 @@ package org.springframework.data.cassandra.repository.support; import java.util.Collection; +import java.util.Collections; import java.util.LinkedList; import java.util.stream.Collectors; +import org.springframework.data.mapping.model.MappingException; +import org.springframework.util.Assert; + /** * Aggregator of multiple violations for convenience when verifying id interfaces. This allows the framework to * communicate all errors at once, rather than one at a time. * * @author Matthew T. Adams + * @author Mark Paluch */ @SuppressWarnings("serial") -public class IdInterfaceExceptions extends RuntimeException { +public class IdInterfaceExceptions extends MappingException { - Collection exceptions = new LinkedList<>(); - String idInterfaceName; + private final Collection exceptions; + private final String className; - public IdInterfaceExceptions(Class idInterface) { - this.idInterfaceName = idInterface.getClass().getName(); + /** + * Create a new {@link IdInterfaceExceptions} for the given {@code idInterfaceClass} and exceptions. + * + * @param idInterfaceClass must not be {@literal null}. + * @param exceptions must not be {@literal null}. + * @since 2.0 + */ + public IdInterfaceExceptions(Class idInterfaceClass, Collection exceptions) { + + super(String.format("Mapping Exceptions for %s", idInterfaceClass.getName())); + + Assert.notNull(idInterfaceClass, "CassandraPersistentEntity must not be null"); + + this.exceptions = Collections.unmodifiableCollection(new LinkedList<>(exceptions)); + this.className = idInterfaceClass.getName(); + + this.exceptions.forEach(this::addSuppressed); } public void add(IdInterfaceException e) { @@ -42,7 +62,7 @@ public class IdInterfaceExceptions extends RuntimeException { /** * Returns a list of the {@link IdInterfaceException}s aggregated within. */ - public Collection getExceptions() { + public Collection getExceptions() { return exceptions; } @@ -62,14 +82,14 @@ public class IdInterfaceExceptions extends RuntimeException { @Override public String getMessage() { - StringBuilder builder = new StringBuilder(idInterfaceName).append(":\n"); - for (IdInterfaceException e : exceptions) { + StringBuilder builder = new StringBuilder(className).append(":\n"); + for (MappingException e : exceptions) { builder.append(e.getMessage()).append("\n"); } return builder.toString(); } public String getIdInterfaceName() { - return idInterfaceName; + return className; } } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceValidator.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceValidator.java index 74ffef744..726ac1632 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceValidator.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/IdInterfaceValidator.java @@ -15,11 +15,17 @@ */ package org.springframework.data.cassandra.repository.support; -import java.io.Serializable; import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.List; import org.springframework.data.cassandra.repository.MapId; +import org.springframework.data.mapping.model.MappingException; +/** + * @author Matthew T. Adams + * @author Mark Paluch + */ public class IdInterfaceValidator { /** @@ -44,10 +50,8 @@ public class IdInterfaceValidator { * Id interfaces *
    *
  • must be an {@code interface}, not a class,
    • - *
  • may extend {@link Serializable},
    • - *
  • may extend {@link MapId},
    • - *
  • must have getter methods that only return a {@link Serializable} type,
    • - *
  • must have setter methods that only take a single {@link Serializable} type,
    • + *
  • may extend {@link MapId} or any other interface,
    • + *
  • must have setter methods that only take a single argument,
    • *
  • must have setter methods that only return {@code void} or the id interface's type,
    • *
  • must not define any getter methods with the literal name "get", and
    • *
  • must not define any setter methods with the literal names "set" or "with".
    • @@ -60,56 +64,57 @@ public class IdInterfaceValidator { */ public static void validate(Class id) { - IdInterfaceExceptions x = new IdInterfaceExceptions(id); + List exceptions = new ArrayList<>(); if (!id.isInterface()) { - x.add(new IdInterfaceException(id, null, "id type must be an interface")); - } - - Class[] interfaces = id.getInterfaces(); - if (interfaces.length > 2 || ((interfaces.length == 1 - && !(interfaces[0].equals(Serializable.class) || interfaces[0].equals(MapId.class))))) { - x.add(new IdInterfaceException(id, null, "id type may only extend Serializable and/or MapId")); + exceptions.add(new IdInterfaceException(id, null, "Id type must be an interface")); } for (Method m : id.getDeclaredMethods()) { + Class[] args = m.getParameterTypes(); String name = m.getName(); - Class ret = m.getReturnType(); + Class returnType = m.getReturnType(); + switch (args.length) { case 0: // then getter + if (name.startsWith("get") && name.length() == 3) { - x.add(new IdInterfaceException(id, m, "getter methods must have a property name following 'get' prefix")); + exceptions + .add(new IdInterfaceException(id, m, "Getter method must have a property name following 'get' prefix")); } - if (!Serializable.class.isAssignableFrom(ret)) { - x.add(new IdInterfaceException(id, m, "getter methods must return Serializable types")); + + if (Void.TYPE.isAssignableFrom(returnType) || Void.class.isAssignableFrom(returnType)) { + exceptions.add(new IdInterfaceException(id, m, "Getter method must return a value")); } + break; case 1: // then setter + if (name.startsWith("set") && name.length() == 3) { - x.add(new IdInterfaceException(id, m, "setter methods must have a property name following 'set' prefix")); + exceptions + .add(new IdInterfaceException(id, m, "Setter method must have a property name following 'set' prefix")); } + if (name.startsWith("with") && name.length() == 4) { - x.add(new IdInterfaceException(id, m, "setter methods must have a property name following 'with' prefix")); + exceptions.add( + new IdInterfaceException(id, m, "Setter method must have a property name following 'with' prefix")); } - if (!void.class.equals(ret) && !id.equals(ret)) { - x.add(new IdInterfaceException(id, m, - "setter methods not returning void may only return the same type as their id interface")); - } - Class arg = args[0]; - if (!Serializable.class.isAssignableFrom(arg)) { - x.add(new IdInterfaceException(id, m, "setter methods must take exactly one Serializable type")); + + if (!Void.TYPE.isAssignableFrom(returnType) && !Void.class.equals(returnType) && !id.equals(returnType)) { + exceptions.add(new IdInterfaceException(id, m, + "Setter method not returning void may only return the same type as their id interface")); } break; default: - x.add(new IdInterfaceException(id, m, - "id interface methods may only take zero parameters for a getter or one parameter for a setter; found " + exceptions.add(new IdInterfaceException(id, m, + "Id interface methods may only take zero parameters for a getter or one parameter for a setter; found " + args.length)); } } - if (x.getCount() > 0) { - throw x; + if (!exceptions.isEmpty()) { + throw new IdInterfaceExceptions(id, exceptions); } } } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdFactory.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdFactory.java index a9d95bd7d..8714ce06c 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdFactory.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdFactory.java @@ -15,11 +15,14 @@ */ package org.springframework.data.cassandra.repository.support; -import java.io.Serializable; import java.lang.reflect.Proxy; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; import org.springframework.data.cassandra.repository.MapId; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; /** * Factory class for producing implementations of given id interfaces. For restrictions on id interfaces definitions, @@ -35,10 +38,10 @@ public class MapIdFactory { /** * Produces an implementation of the given id interface type using the type's class loader. For restrictions on id * interfaces definitions, see {@link IdInterfaceValidator#validate(Class)}. Returns an implementation of the given - * interface that also implements {@link MapId} and {@link Serializable}, so it can be cast as such if necessary. + * interface that also implements {@link MapId}, so it can be cast as such if necessary. * * @param idInterface The type of the id interface. - * @return An implementation of the given interface that also implements {@link MapId} and {@link Serializable}. + * @return An implementation of the given interface that also implements {@link MapId}. * @see IdInterfaceValidator#validate(Class) */ public static T id(Class idInterface) { @@ -50,10 +53,10 @@ public class MapIdFactory { /** * Produces an implementation of the given class loader. For restrictions on id interfaces definitions, see * {@link IdInterfaceValidator#validate(Class)}. Returns an implementation of the given interface that also implements - * {@link MapId} and {@link Serializable}, so it can be cast as such if necessary. + * {@link MapId}, so it can be cast as such if necessary. * * @param idInterface The type of the id interface. - * @return An implementation of the given interface that also implements {@link MapId} and {@link Serializable}. + * @return An implementation of the given interface that also implements {@link MapId}. * @see IdInterfaceValidator#validate(Class) */ public static T id(Class idInterface, ClassLoader loader) { @@ -63,18 +66,13 @@ public class MapIdFactory { IdInterfaceValidator.validate(idInterface); - Class[] interfaces = idInterface.getInterfaces(); - switch (interfaces.length) { - case 0: - interfaces = new Class[] { idInterface, MapId.class, Serializable.class }; - break; - case 1: - Class other = interfaces[0].equals(Serializable.class) ? MapId.class : Serializable.class; - interfaces = new Class[] { idInterface, interfaces[0], other }; - break; - default: - interfaces = new Class[] { idInterface, interfaces[0], interfaces[1] }; - } - return (T) Proxy.newProxyInstance(loader, interfaces, new MapIdProxyDelegate(idInterface)); + Class[] idInterfaces = ClassUtils.getAllInterfacesForClass(idInterface); + Set> proxyInterfaces = new HashSet<>(idInterfaces.length + 1, 1); + + proxyInterfaces.add(MapId.class); + proxyInterfaces.addAll(Arrays.asList(idInterfaces)); + + return (T) Proxy.newProxyInstance(loader, proxyInterfaces.toArray(new Class[proxyInterfaces.size()]), + new MapIdProxyDelegate(idInterface)); } } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdProxyDelegate.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdProxyDelegate.java index 97ab97c30..e60785dc7 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdProxyDelegate.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MapIdProxyDelegate.java @@ -15,7 +15,6 @@ */ package org.springframework.data.cassandra.repository.support; -import java.io.Serializable; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.util.Arrays; @@ -110,11 +109,8 @@ class MapIdProxyDelegate implements InvocationHandler { delegate.put(name, null); return; } - if (!(value instanceof Serializable)) { - throw new IllegalArgumentException( - String.format("Given object [%s] must implement %s", value, Serializable.class.getName())); - } - delegate.put(name, (Serializable) value); + + delegate.put(name, value); } } diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MapIdFactoryUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MapIdFactoryUnitTests.java index 9428519fc..78320c626 100755 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MapIdFactoryUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MapIdFactoryUnitTests.java @@ -30,6 +30,7 @@ import org.springframework.data.cassandra.repository.MapId; * Unit tests for {@link MapIdFactory}. * * @author Matthew T. Adams + * @author Mark Paluch */ public class MapIdFactoryUnitTests { @@ -209,8 +210,6 @@ public class MapIdFactoryUnitTests { interface Foo {} - interface IdExtendingNotMapId extends Foo {} - interface LiteralGet { String get(); } @@ -223,10 +222,6 @@ public class MapIdFactoryUnitTests { void string(); } - interface GetReturningNonSerializable { - Object getFoo(); - } - interface MethodWithMoreThanOneArgument { void foo(Object a, Object b); } @@ -251,30 +246,18 @@ public class MapIdFactoryUnitTests { String withString(String s); } - interface SetterMethodTakingNonSerializable { - void string(Object o); - } - - interface SetMethodTakingNonSerializable { - void string(Object o); - } - - interface WithMethodTakingNonSerializable { - void string(Object o); - } - @Test public void testUnhappies() { - Class[] interfaces = new Class[] { IdClass.class, IdExtendingNotMapId.class, LiteralGet.class, - GetterReturningVoid.class, GetReturningVoid.class, GetReturningNonSerializable.class, - MethodWithMoreThanOneArgument.class, LiteralSet.class, LiteralWith.class, + + Class[] interfaces = new Class[] { IdClass.class, LiteralGet.class, GetterReturningVoid.class, + GetReturningVoid.class, MethodWithMoreThanOneArgument.class, LiteralSet.class, LiteralWith.class, SetterMethodNotReturningVoidOrThis.class, SetMethodNotReturningVoidOrThis.class, - WithMethodNotReturningVoidOrThis.class, SetterMethodTakingNonSerializable.class, - SetMethodTakingNonSerializable.class, WithMethodTakingNonSerializable.class }; - for (Class i : interfaces) { + WithMethodNotReturningVoidOrThis.class }; + + for (Class idInterface : interfaces) { try { - validate(i); - fail("should've caught IdInterfaceException validating interface " + i); + validate(idInterface); + fail("should've caught IdInterfaceException validating interface " + idInterface); } catch (IdInterfaceExceptions e) { assertThat(e.getCount()).isEqualTo(1); }