From 56cb289d7d83af041130ddf2617d8a50a7a716df Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 4 Apr 2014 11:19:09 +0200 Subject: [PATCH 1/3] DATAMONGO-892 - MappingConverter should be configurable as nested bean. Prepare issue branch. --- pom.xml | 2 +- spring-data-mongodb-cross-store/pom.xml | 4 ++-- spring-data-mongodb-distribution/pom.xml | 2 +- spring-data-mongodb-log4j/pom.xml | 2 +- spring-data-mongodb/pom.xml | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index 28e7260e25..2147374802 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT pom Spring Data MongoDB diff --git a/spring-data-mongodb-cross-store/pom.xml b/spring-data-mongodb-cross-store/pom.xml index 69a19c150b..31ffecc4e2 100644 --- a/spring-data-mongodb-cross-store/pom.xml +++ b/spring-data-mongodb-cross-store/pom.xml @@ -6,7 +6,7 @@ org.springframework.data spring-data-mongodb-parent - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT ../pom.xml @@ -48,7 +48,7 @@ org.springframework.data spring-data-mongodb - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT diff --git a/spring-data-mongodb-distribution/pom.xml b/spring-data-mongodb-distribution/pom.xml index 7302043a15..e52ba7bc75 100644 --- a/spring-data-mongodb-distribution/pom.xml +++ b/spring-data-mongodb-distribution/pom.xml @@ -13,7 +13,7 @@ org.springframework.data spring-data-mongodb-parent - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb-log4j/pom.xml b/spring-data-mongodb-log4j/pom.xml index 0415f6eb5e..7826249f54 100644 --- a/spring-data-mongodb-log4j/pom.xml +++ b/spring-data-mongodb-log4j/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb/pom.xml b/spring-data-mongodb/pom.xml index 6e84732769..3cb382e13d 100644 --- a/spring-data-mongodb/pom.xml +++ b/spring-data-mongodb/pom.xml @@ -11,7 +11,7 @@ org.springframework.data spring-data-mongodb-parent - 1.5.0.BUILD-SNAPSHOT + 1.5.0.DATAMONGO-892-SNAPSHOT ../pom.xml From 3ea2dd45040e963a0bd9b39ad42a1e0a7c424635 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 4 Apr 2014 14:25:46 +0200 Subject: [PATCH 2/3] DATAMONGO-892 - MappingConverter should be configurable as nested bean. Register mapping converter as uniquely named bean in context and wire conversions correctly to avoid overriding existing ones. --- .../config/MappingMongoConverterParser.java | 44 ++++++++++++++++--- ...gMongoConverterParserIntegrationTests.java | 12 +++++ .../config/NestedMongoConverterWrapper.java | 35 +++++++++++++++ .../test/resources/namespace/converter.xml | 18 ++++++-- 4 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/NestedMongoConverterWrapper.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java index 6b601c66d7..ac89ab6e8a 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java @@ -71,6 +71,7 @@ * @author Oliver Gierke * @author Maciej Walkowiak * @author Thomas Darimont + * @author Christoph Strobl */ public class MappingMongoConverterParser implements BeanDefinitionParser { @@ -84,13 +85,12 @@ public class MappingMongoConverterParser implements BeanDefinitionParser { public BeanDefinition parse(Element element, ParserContext parserContext) { BeanDefinitionRegistry registry = parserContext.getRegistry(); - - String id = element.getAttribute(AbstractBeanDefinitionParser.ID_ATTRIBUTE); - id = StringUtils.hasText(id) ? id : DEFAULT_CONVERTER_BEAN_NAME; + String id = resolveMongoConverterBeanId(element, parserContext); parserContext.pushContainingComponent(new CompositeComponentDefinition("Mapping Mongo Converter", element)); - BeanDefinition conversionsDefinition = getCustomConversions(element, parserContext); + + // should we use a nested bean definition instead of context reference in case parserContext.isNested() ? String ctxRef = potentiallyCreateMappingContext(element, parserContext, conversionsDefinition, id); createIsNewStrategyFactoryBeanDefinition(ctxRef, parserContext, element); @@ -138,9 +138,28 @@ public BeanDefinition parse(Element element, ParserContext parserContext) { VALIDATING_EVENT_LISTENER_BEAN_NAME)); } - parserContext.registerBeanComponent(new BeanComponentDefinition(converterBuilder.getBeanDefinition(), id)); + BeanComponentDefinition definition = new BeanComponentDefinition(converterBuilder.getBeanDefinition(), id); + parserContext.registerBeanComponent(definition); parserContext.popAndRegisterContainingComponent(); - return null; + return definition.getBeanDefinition(); + } + + /** + * Create valid mongo converter bean id using given {@literal id} or default converter bean name. + * + * @param element + * @param parserContext + * @return + */ + private String resolveMongoConverterBeanId(Element element, ParserContext parserContext) { + + String id = element.getAttribute(AbstractBeanDefinitionParser.ID_ATTRIBUTE); + if (!parserContext.isNested()) { + id = StringUtils.hasText(id) ? id : DEFAULT_CONVERTER_BEAN_NAME; + } else { + id = parserContext.getContainingBeanDefinition().getBeanClassName() + ":" + DEFAULT_CONVERTER_BEAN_NAME; + } + return id; } private BeanDefinition potentiallyCreateValidatingMongoEventListener(Element element, ParserContext parserContext) { @@ -250,6 +269,19 @@ private BeanDefinition getCustomConversions(Element element, ParserContext parse } } + // in case of nested definition register customConversions as nested component + if (parserContext.isNested()) { + + BeanDefinitionBuilder conversionsBuilder = BeanDefinitionBuilder.genericBeanDefinition(CustomConversions.class); + conversionsBuilder.addConstructorArgValue(converterBeans); + + AbstractBeanDefinition conversionsBean = conversionsBuilder.getBeanDefinition(); + parserContext.getContainingComponent().addNestedComponent( + new BeanComponentDefinition(conversionsBean, "customConversions")); + + return conversionsBean; + } + BeanDefinitionBuilder conversionsBuilder = BeanDefinitionBuilder.rootBeanDefinition(CustomConversions.class); conversionsBuilder.addConstructorArgValue(converterBeans); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java index 5a7951f1e8..b107d50063 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java @@ -45,6 +45,7 @@ * * @author Oliver Gierke * @author Thomas Darimont + * @author Christoph Strobl */ public class MappingMongoConverterParserIntegrationTests { @@ -104,6 +105,17 @@ public void activatesAbbreviatingPropertiesCorrectly() { assertThat(strategy.getBeanClassName(), is(CamelCaseAbbreviatingFieldNamingStrategy.class.getName())); } + /** + * @see DATAMONGO-892 + */ + @Test + public void registersNesedConverterBeanDefinitionCorrectly() { + + NestedMongoConverterWrapper configWrapper = factory.getBean(NestedMongoConverterWrapper.class); + assertThat(configWrapper.getConverter().convertToMongoType(new Person()), nullValue()); + assertThat(configWrapper.getConverter().convertToMongoType(new Account()), notNullValue()); + } + @Component public static class SampleConverter implements Converter { public DBObject convert(Person source) { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/NestedMongoConverterWrapper.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/NestedMongoConverterWrapper.java new file mode 100644 index 0000000000..116a909b8b --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/NestedMongoConverterWrapper.java @@ -0,0 +1,35 @@ +/* + * Copyright 2014 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.mongodb.config; + +import org.springframework.data.mongodb.core.convert.MongoConverter; + +/** + * @author Christoph Strobl + */ +public class NestedMongoConverterWrapper { + + private final MongoConverter converter; + + public NestedMongoConverterWrapper(MongoConverter converter) { + this.converter = converter; + } + + public MongoConverter getConverter() { + return converter; + } + +} diff --git a/spring-data-mongodb/src/test/resources/namespace/converter.xml b/spring-data-mongodb/src/test/resources/namespace/converter.xml index 91842765d9..99336bb53e 100644 --- a/spring-data-mongodb/src/test/resources/namespace/converter.xml +++ b/spring-data-mongodb/src/test/resources/namespace/converter.xml @@ -5,14 +5,26 @@ xsi:schemaLocation="http://www.springframework.org/schema/data/mongo http://www.springframework.org/schema/data/mongo/spring-mongo.xsd http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd"> - + - + - + + + + + + + + + + + + + From 2bbcdc344610fead1bd658f20c3283cfad6b9c83 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 9 Apr 2014 15:49:36 +0200 Subject: [PATCH 3/3] DATAMONGO-892 - Provide meaningful error message when registering converter as nested bean. Mapping information is potentially required by multiple instances and should not be registered within a template as nested bean. The current solution did throw a misleading error which has been corrected. Original Pull Request: #165 --- .../config/MappingMongoConverterParser.java | 46 ++++--------------- ...gMongoConverterParserIntegrationTests.java | 32 +++++++++---- .../converter-nested-bean-definition.xml | 22 +++++++++ .../test/resources/namespace/converter.xml | 18 ++------ 4 files changed, 58 insertions(+), 60 deletions(-) create mode 100644 spring-data-mongodb/src/test/resources/namespace/converter-nested-bean-definition.xml diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java index ac89ab6e8a..6ff9b14efb 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/config/MappingMongoConverterParser.java @@ -84,13 +84,17 @@ public class MappingMongoConverterParser implements BeanDefinitionParser { */ public BeanDefinition parse(Element element, ParserContext parserContext) { + if (parserContext.isNested()) { + parserContext.getReaderContext().error("Mongo Converter must not be defined as nested bean.", element); + } + BeanDefinitionRegistry registry = parserContext.getRegistry(); - String id = resolveMongoConverterBeanId(element, parserContext); + String id = element.getAttribute(AbstractBeanDefinitionParser.ID_ATTRIBUTE); + id = StringUtils.hasText(id) ? id : DEFAULT_CONVERTER_BEAN_NAME; parserContext.pushContainingComponent(new CompositeComponentDefinition("Mapping Mongo Converter", element)); - BeanDefinition conversionsDefinition = getCustomConversions(element, parserContext); - // should we use a nested bean definition instead of context reference in case parserContext.isNested() ? + BeanDefinition conversionsDefinition = getCustomConversions(element, parserContext); String ctxRef = potentiallyCreateMappingContext(element, parserContext, conversionsDefinition, id); createIsNewStrategyFactoryBeanDefinition(ctxRef, parserContext, element); @@ -138,28 +142,9 @@ public BeanDefinition parse(Element element, ParserContext parserContext) { VALIDATING_EVENT_LISTENER_BEAN_NAME)); } - BeanComponentDefinition definition = new BeanComponentDefinition(converterBuilder.getBeanDefinition(), id); - parserContext.registerBeanComponent(definition); + parserContext.registerBeanComponent(new BeanComponentDefinition(converterBuilder.getBeanDefinition(), id)); parserContext.popAndRegisterContainingComponent(); - return definition.getBeanDefinition(); - } - - /** - * Create valid mongo converter bean id using given {@literal id} or default converter bean name. - * - * @param element - * @param parserContext - * @return - */ - private String resolveMongoConverterBeanId(Element element, ParserContext parserContext) { - - String id = element.getAttribute(AbstractBeanDefinitionParser.ID_ATTRIBUTE); - if (!parserContext.isNested()) { - id = StringUtils.hasText(id) ? id : DEFAULT_CONVERTER_BEAN_NAME; - } else { - id = parserContext.getContainingBeanDefinition().getBeanClassName() + ":" + DEFAULT_CONVERTER_BEAN_NAME; - } - return id; + return null; } private BeanDefinition potentiallyCreateValidatingMongoEventListener(Element element, ParserContext parserContext) { @@ -269,19 +254,6 @@ private BeanDefinition getCustomConversions(Element element, ParserContext parse } } - // in case of nested definition register customConversions as nested component - if (parserContext.isNested()) { - - BeanDefinitionBuilder conversionsBuilder = BeanDefinitionBuilder.genericBeanDefinition(CustomConversions.class); - conversionsBuilder.addConstructorArgValue(converterBeans); - - AbstractBeanDefinition conversionsBean = conversionsBuilder.getBeanDefinition(); - parserContext.getContainingComponent().addNestedComponent( - new BeanComponentDefinition(conversionsBean, "customConversions")); - - return conversionsBean; - } - BeanDefinitionBuilder conversionsBuilder = BeanDefinitionBuilder.rootBeanDefinition(CustomConversions.class); conversionsBuilder.addConstructorArgValue(converterBeans); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java index b107d50063..e8c6920412 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/MappingMongoConverterParserIntegrationTests.java @@ -21,9 +21,11 @@ import java.util.Collections; import java.util.Set; -import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.parsing.BeanDefinitionParsingException; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.xml.XmlBeanDefinitionReader; import org.springframework.core.convert.TypeDescriptor; @@ -49,13 +51,22 @@ */ public class MappingMongoConverterParserIntegrationTests { + @Rule public ExpectedException expcetedException = ExpectedException.none(); + DefaultListableBeanFactory factory; - @Before - public void setUp() { + private void loadValidConfiguration() { + this.loadConfiguration("namespace/converter.xml"); + } + + private void loadNestedBeanConfiguration() { + this.loadConfiguration("namespace/converter-nested-bean-definition.xml"); + } + + private void loadConfiguration(String configLocation) { factory = new DefaultListableBeanFactory(); XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(factory); - reader.loadBeanDefinitions(new ClassPathResource("namespace/converter.xml")); + reader.loadBeanDefinitions(new ClassPathResource(configLocation)); } /** @@ -64,6 +75,7 @@ public void setUp() { @Test public void allowsDbFactoryRefAttribute() { + loadValidConfiguration(); factory.getBeanDefinition("converter"); factory.getBean("converter"); } @@ -74,6 +86,7 @@ public void allowsDbFactoryRefAttribute() { @Test public void hasCustomTypeMapper() { + loadValidConfiguration(); MappingMongoConverter converter = factory.getBean("converter", MappingMongoConverter.class); MongoTypeMapper customMongoTypeMapper = factory.getBean(CustomMongoTypeMapper.class); @@ -86,6 +99,7 @@ public void hasCustomTypeMapper() { @Test public void scansForConverterAndSetsUpCustomConversionsAccordingly() { + loadValidConfiguration(); CustomConversions conversions = factory.getBean(CustomConversions.class); assertThat(conversions.hasCustomWriteTarget(Person.class), is(true)); assertThat(conversions.hasCustomWriteTarget(Account.class), is(true)); @@ -97,6 +111,7 @@ public void scansForConverterAndSetsUpCustomConversionsAccordingly() { @Test public void activatesAbbreviatingPropertiesCorrectly() { + loadValidConfiguration(); BeanDefinition definition = factory.getBeanDefinition("abbreviatingConverter.mongoMappingContext"); Object value = definition.getPropertyValues().getPropertyValue("fieldNamingStrategy").getValue(); @@ -109,11 +124,12 @@ public void activatesAbbreviatingPropertiesCorrectly() { * @see DATAMONGO-892 */ @Test - public void registersNesedConverterBeanDefinitionCorrectly() { + public void shouldThrowBeanDefinitionParsingExceptionIfConverterDefinedAsNestedBean() { + + expcetedException.expect(BeanDefinitionParsingException.class); + expcetedException.expectMessage("Mongo Converter must not be defined as nested bean."); - NestedMongoConverterWrapper configWrapper = factory.getBean(NestedMongoConverterWrapper.class); - assertThat(configWrapper.getConverter().convertToMongoType(new Person()), nullValue()); - assertThat(configWrapper.getConverter().convertToMongoType(new Account()), notNullValue()); + loadNestedBeanConfiguration(); } @Component diff --git a/spring-data-mongodb/src/test/resources/namespace/converter-nested-bean-definition.xml b/spring-data-mongodb/src/test/resources/namespace/converter-nested-bean-definition.xml new file mode 100644 index 0000000000..1248ad3c98 --- /dev/null +++ b/spring-data-mongodb/src/test/resources/namespace/converter-nested-bean-definition.xml @@ -0,0 +1,22 @@ + + + + + + + + + + + + + + + + + + diff --git a/spring-data-mongodb/src/test/resources/namespace/converter.xml b/spring-data-mongodb/src/test/resources/namespace/converter.xml index 99336bb53e..91842765d9 100644 --- a/spring-data-mongodb/src/test/resources/namespace/converter.xml +++ b/spring-data-mongodb/src/test/resources/namespace/converter.xml @@ -5,26 +5,14 @@ xsi:schemaLocation="http://www.springframework.org/schema/data/mongo http://www.springframework.org/schema/data/mongo/spring-mongo.xsd http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd"> - + - + - - - - - - - - - - - - - +