From 4939fbf3bc3bb378b9901962d8b5041020a5abeb Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 3 Sep 2012 13:36:34 +0200 Subject: [PATCH] DATACMNS-226 - Fixed bug in ConfigurableTypeInformationMapper caching. Separated ConfigurableTypeInformationMapper from MappingContextTypeInformationMapper. The latter now lazily picks up the alias information if the cached values do not contain aliases. Polished JavaDocs and clarified meaning of null values. --- .../ConfigurableTypeInformationMapper.java | 33 ++--- .../data/convert/DefaultTypeMapper.java | 30 ++++- .../MappingContextTypeInformationMapper.java | 125 ++++++++++++++++++ .../convert/SimpleTypeInformationMapper.java | 6 +- .../data/convert/TypeAliasAccessor.java | 2 +- .../data/convert/TypeInformationMapper.java | 2 +- .../context/AbstractMappingContext.java | 4 + ...gurableTypeInformationMapperUnitTests.java | 61 +-------- ...ContextTypeInformationMapperUnitTests.java | 106 +++++++++++++++ .../data/mapping/MappingMetadataTests.java | 5 +- 10 files changed, 281 insertions(+), 93 deletions(-) create mode 100644 spring-data-commons-core/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java create mode 100644 spring-data-commons-core/src/test/java/org/springframework/data/convert/MappingContextTypeInformationMapperUnitTests.java diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/ConfigurableTypeInformationMapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/ConfigurableTypeInformationMapper.java index 03a8d46fa..befbc77de 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/convert/ConfigurableTypeInformationMapper.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/ConfigurableTypeInformationMapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2011 the original author or authors. + * Copyright 2011-12 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. @@ -36,25 +36,6 @@ public class ConfigurableTypeInformationMapper implements TypeInformationMapper private final Map, Object> typeMap; - /** - * Creates a {@link ConfigurableTypeInformationMapper} from the given {@link MappingContext}. Inspects all - * {@link PersistentEntity} instances for alias information and builds a {@link Map} of aliases to types from it. - * - * @param mappingContext - */ - public ConfigurableTypeInformationMapper(MappingContext, ?> mappingContext) { - - Assert.notNull(mappingContext); - this.typeMap = new HashMap, Object>(); - - for (PersistentEntity entity : mappingContext.getPersistentEntities()) { - Object alias = entity.getTypeAlias(); - if (alias != null) { - typeMap.put(entity.getTypeInformation(), alias); - } - } - } - /** * Creates a new {@link ConfigurableTypeMapper} for the given type map. * @@ -63,7 +44,6 @@ public class ConfigurableTypeInformationMapper implements TypeInformationMapper public ConfigurableTypeInformationMapper(Map, String> sourceTypeMap) { Assert.notNull(sourceTypeMap); - this.typeMap = new HashMap, Object>(sourceTypeMap.size()); for (Entry, String> entry : sourceTypeMap.entrySet()) { @@ -79,13 +59,12 @@ public class ConfigurableTypeInformationMapper implements TypeInformationMapper } } - /* (non-Javadoc) + /* + * (non-Javadoc) * @see org.springframework.data.convert.TypeInformationMapper#createAliasFor(org.springframework.data.util.TypeInformation) */ public Object createAliasFor(TypeInformation type) { - - Object key = typeMap.get(type); - return key == null ? null : key; + return typeMap.get(type); } /* @@ -94,6 +73,10 @@ public class ConfigurableTypeInformationMapper implements TypeInformationMapper */ public TypeInformation resolveTypeFrom(Object alias) { + if (alias == null) { + return null; + } + for (Entry, Object> entry : typeMap.entrySet()) { if (entry.getValue().equals(alias)) { return entry.getKey(); diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/DefaultTypeMapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/DefaultTypeMapper.java index d2d65c742..f400e9330 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/convert/DefaultTypeMapper.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/DefaultTypeMapper.java @@ -39,24 +39,46 @@ public class DefaultTypeMapper implements TypeMapper { private final TypeAliasAccessor accessor; private final List mappers; + /** + * Creates a new {@link DefaultTypeMapper} using the given {@link TypeAliasAccessor}. It will use a + * {@link SimpleTypeInformationMapper} to calculate type aliases. + * + * @param accessor must not be {@literal null}. + */ public DefaultTypeMapper(TypeAliasAccessor accessor) { this(accessor, Arrays.asList(SimpleTypeInformationMapper.INSTANCE)); } + /** + * Creates a new {@link DefaultTypeMapper} using the given {@link TypeAliasAccessor} and {@link TypeInformationMapper} + * s. + * + * @param accessor must not be {@literal null}. + * @param mappers must not be {@literal null}. + */ public DefaultTypeMapper(TypeAliasAccessor accessor, List mappers) { - this(accessor, null, mappers); } + /** + * Creates a new {@link DefaultTypeMapper} using the given {@link TypeAliasAccessor}, {@link MappingContext} and + * additional {@link TypeInformationMapper}s. Will register a {@link MappingContextTypeInformationMapper} before the + * given additional mappers. + * + * @param accessor must not be {@literal null}. + * @param mappingContext + * @param additionalMappers must not be {@literal null}. + */ public DefaultTypeMapper(TypeAliasAccessor accessor, MappingContext, ?> mappingContext, List additionalMappers) { Assert.notNull(accessor); + Assert.notNull(additionalMappers); List mappers = new ArrayList(additionalMappers.size() + 1); if (mappingContext != null) { - mappers.add(new ConfigurableTypeInformationMapper(mappingContext)); + mappers.add(new MappingContextTypeInformationMapper(mappingContext)); } mappers.addAll(additionalMappers); @@ -73,6 +95,10 @@ public class DefaultTypeMapper implements TypeMapper { Assert.notNull(source); Object alias = accessor.readAliasFrom(source); + if (alias == null) { + return null; + } + for (TypeInformationMapper mapper : mappers) { TypeInformation type = mapper.resolveTypeFrom(alias); diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java new file mode 100644 index 000000000..b3adb5a26 --- /dev/null +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java @@ -0,0 +1,125 @@ +/* + * Copyright 2011-2012 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.convert; + +import java.util.HashMap; +import java.util.Map; +import java.util.Map.Entry; + +import org.springframework.data.mapping.PersistentEntity; +import org.springframework.data.mapping.context.MappingContext; +import org.springframework.data.util.TypeInformation; +import org.springframework.util.Assert; + +/** + * {@link TypeInformationMapper} implementation that can be either set up using a {@link MappingContext} or manually set + * up {@link Map} of {@link String} aliases to types. If a {@link MappingContext} is used the {@link Map} will be build + * inspecting the {@link PersistentEntity} instances for type alias information. + * + * @author Oliver Gierke + */ +public class MappingContextTypeInformationMapper implements TypeInformationMapper { + + private final Map, Object> typeMap; + private final MappingContext, ?> mappingContext; + + /** + * Creates a {@link MappingContextTypeInformationMapper} from the given {@link MappingContext}. Inspects all + * {@link PersistentEntity} instances for alias information and builds a {@link Map} of aliases to types from it. + * + * @param mappingContext must not be {@literal null}. + */ + public MappingContextTypeInformationMapper(MappingContext, ?> mappingContext) { + + Assert.notNull(mappingContext); + + this.typeMap = new HashMap, Object>(); + this.mappingContext = mappingContext; + + for (PersistentEntity entity : mappingContext.getPersistentEntities()) { + safelyAddToCache(entity.getTypeInformation(), entity.getTypeAlias()); + } + } + + /* + * (non-Javadoc) + * @see org.springframework.data.convert.TypeInformationMapper#createAliasFor(org.springframework.data.util.TypeInformation) + */ + public Object createAliasFor(TypeInformation type) { + + Object key = typeMap.get(type); + + if (key != null) { + return key; + } + + PersistentEntity entity = mappingContext.getPersistentEntity(type); + + if (entity == null) { + return null; + } + + Object alias = entity.getTypeAlias(); + safelyAddToCache(type, alias); + + return alias; + } + + /** + * Adds the given alias to the cache in a {@literal null}-safe manner. + * + * @param key must not be {@literal null}. + * @param alias can be {@literal null}. + */ + private void safelyAddToCache(TypeInformation key, Object alias) { + + if (alias == null) { + return; + } + + if (typeMap.containsValue(alias)) { + throw new IllegalArgumentException(String.format( + "Detected mapping ambiguity! String %s cannot be mapped to more than one type!", alias)); + } + + typeMap.put(key, alias); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.convert.TypeInformationMapper#resolveTypeFrom(java.lang.Object) + */ + public TypeInformation resolveTypeFrom(Object alias) { + + if (alias == null) { + return null; + } + + for (Entry, Object> entry : typeMap.entrySet()) { + if (entry.getValue().equals(alias)) { + return entry.getKey(); + } + } + + for (PersistentEntity entity : mappingContext.getPersistentEntities()) { + if (alias.equals(entity.getTypeAlias())) { + return entity.getTypeInformation(); + } + } + + return null; + } +} diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/SimpleTypeInformationMapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/SimpleTypeInformationMapper.java index 261f052cd..7ee24a622 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/convert/SimpleTypeInformationMapper.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/SimpleTypeInformationMapper.java @@ -40,13 +40,13 @@ public class SimpleTypeInformationMapper implements TypeInformationMapper { * @return the type to be used for the given {@link String} representation or {@literal null} if nothing found or the * class cannot be loaded. */ - public TypeInformation resolveTypeFrom(Object source) { + public TypeInformation resolveTypeFrom(Object alias) { - if (!(source instanceof String)) { + if (!(alias instanceof String)) { return null; } - String value = (String) source; + String value = (String) alias; if (!StringUtils.hasText(value)) { return null; diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeAliasAccessor.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeAliasAccessor.java index 6372408dc..7da305e25 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeAliasAccessor.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeAliasAccessor.java @@ -26,7 +26,7 @@ public interface TypeAliasAccessor { * Reads the type alias to be used from the given source. * * @param source - * @return + * @return can be {@literal null} in case no alias was found. */ Object readAliasFrom(S source); diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeInformationMapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeInformationMapper.java index 29207f5f2..0246d4f52 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeInformationMapper.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/convert/TypeInformationMapper.java @@ -27,7 +27,7 @@ public interface TypeInformationMapper { /** * Returns the actual {@link TypeInformation} to be used for the given alias. * - * @param alias + * @param alias can be {@literal null}. * @return */ TypeInformation resolveTypeFrom(Object alias); diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java index 42979ead2..a9c6a5a05 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -408,6 +408,10 @@ public abstract class AbstractMappingContext candidate : property.getPersistentEntityType()) { addPersistentEntity(candidate); } diff --git a/spring-data-commons-core/src/test/java/org/springframework/data/convert/ConfigurableTypeInformationMapperUnitTests.java b/spring-data-commons-core/src/test/java/org/springframework/data/convert/ConfigurableTypeInformationMapperUnitTests.java index 25f82770d..bffb7239d 100644 --- a/spring-data-commons-core/src/test/java/org/springframework/data/convert/ConfigurableTypeInformationMapperUnitTests.java +++ b/spring-data-commons-core/src/test/java/org/springframework/data/convert/ConfigurableTypeInformationMapperUnitTests.java @@ -18,8 +18,6 @@ package org.springframework.data.convert; import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; -import java.beans.PropertyDescriptor; -import java.lang.reflect.Field; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -27,18 +25,8 @@ import java.util.Map; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; -import org.springframework.context.ApplicationContext; -import org.springframework.context.event.ContextRefreshedEvent; -import org.springframework.data.annotation.TypeAlias; -import org.springframework.data.mapping.Association; import org.springframework.data.mapping.PersistentProperty; -import org.springframework.data.mapping.context.AbstractMappingContext; -import org.springframework.data.mapping.context.MappingContext; -import org.springframework.data.mapping.model.AnnotationBasedPersistentProperty; -import org.springframework.data.mapping.model.BasicPersistentEntity; -import org.springframework.data.mapping.model.SimpleTypeHolder; import org.springframework.data.util.ClassTypeInformation; import org.springframework.data.util.TypeInformation; @@ -51,8 +39,6 @@ import org.springframework.data.util.TypeInformation; public class ConfigurableTypeInformationMapperUnitTests> { ConfigurableTypeInformationMapper mapper; - @Mock - ApplicationContext context; @Before public void setUp() { @@ -61,16 +47,12 @@ public class ConfigurableTypeInformationMapperUnitTests, String>) null); - } - - @Test(expected = IllegalArgumentException.class) - public void rejectsNullMappingContext() { - new ConfigurableTypeInformationMapper((MappingContext) null); + new ConfigurableTypeInformationMapper(null); } @Test(expected = IllegalArgumentException.class) public void rejectsNonBijectionalMap() { + Map, String> map = new HashMap, String>(); map.put(String.class, "1"); map.put(Object.class, "1"); @@ -78,40 +60,6 @@ public class ConfigurableTypeInformationMapperUnitTests, T> mappingContext = new AbstractMappingContext, T>() { - - @Override - protected BasicPersistentEntity createPersistentEntity(TypeInformation typeInformation) { - return (BasicPersistentEntity) new BasicPersistentEntity(typeInformation); - } - - @Override - protected T createPersistentProperty(Field field, PropertyDescriptor descriptor, - BasicPersistentEntity owner, SimpleTypeHolder simpleTypeHolder) { - return (T) new AnnotationBasedPersistentProperty(field, descriptor, owner, simpleTypeHolder) { - @Override - protected Association createAssociation() { - return null; - } - }; - } - }; - - ContextRefreshedEvent event = new ContextRefreshedEvent(context); - - mappingContext.setInitialEntitySet(Collections.singleton(Entity.class)); - mappingContext.setApplicationContext(context); - mappingContext.onApplicationEvent(event); - - mapper = new ConfigurableTypeInformationMapper(mappingContext); - - assertThat(mapper.createAliasFor(ClassTypeInformation.from(Entity.class)), is((Object) "foo")); - } - @Test public void writesMapKeyForType() { @@ -126,9 +74,4 @@ public class ConfigurableTypeInformationMapperUnitTests entity = mappingContext.getPersistentEntity(Entity.class); + + assertThat(entity, is(notNullValue())); + assertThat(mapper.resolveTypeFrom("foo"), is((TypeInformation) from(Entity.class))); + } + + @TypeAlias("foo") + static class Entity { + + } +} diff --git a/spring-data-commons-core/src/test/java/org/springframework/data/mapping/MappingMetadataTests.java b/spring-data-commons-core/src/test/java/org/springframework/data/mapping/MappingMetadataTests.java index e0f238418..5cd152961 100644 --- a/spring-data-commons-core/src/test/java/org/springframework/data/mapping/MappingMetadataTests.java +++ b/spring-data-commons-core/src/test/java/org/springframework/data/mapping/MappingMetadataTests.java @@ -73,7 +73,7 @@ public class MappingMetadataTests { public interface SampleProperty extends PersistentProperty { } - public class SampleMappingContext extends + public static class SampleMappingContext extends AbstractMappingContext, SampleProperty> { @Override @@ -89,7 +89,8 @@ public class MappingMetadataTests { } } - public class SamplePropertyImpl extends AnnotationBasedPersistentProperty implements SampleProperty { + public static class SamplePropertyImpl extends AnnotationBasedPersistentProperty implements + SampleProperty { public SamplePropertyImpl(Field field, PropertyDescriptor propertyDescriptor, PersistentEntity owner, SimpleTypeHolder simpleTypeHolder) {