From e2ad3a47d234dbe8e3544539f3f6d63c4cb975c0 Mon Sep 17 00:00:00 2001 From: David Pilar Date: Tue, 19 May 2026 07:19:58 +0200 Subject: [PATCH] Fix user-defined Converter beans were ignored by @Command-annotated methods #1352 Signed-off-by: David Pilar --- .../support/CommandFactoryBean.java | 41 ++++- .../support/CommandFactoryBeanTests.java | 143 +++++++++++++++++- .../CustomConverterShellTests.java | 115 ++++++++++++++ 3 files changed, 295 insertions(+), 4 deletions(-) create mode 100644 spring-shell-test-autoconfigure/src/test/java/org/springframework/shell/test/autoconfigure/CustomConverterShellTests.java diff --git a/spring-shell-core/src/main/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java b/spring-shell-core/src/main/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java index edcd80ef1..070cf728b 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBean.java @@ -32,7 +32,12 @@ import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.core.ResolvableType; import org.springframework.core.annotation.MergedAnnotations; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.ConverterFactory; +import org.springframework.core.convert.converter.GenericConverter; import org.springframework.core.convert.support.ConfigurableConversionService; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.shell.core.command.Command; @@ -211,10 +216,44 @@ private ConfigurableConversionService getConfigurableConversionService() { } catch (BeansException e) { log.debug("No ConfigurableConversionService bean found, using a default conversion service."); - return new DefaultConversionService(); + DefaultConversionService conversionService = new DefaultConversionService(); + registerConverterBeans(conversionService); + return conversionService; } } + private void registerConverterBeans(ConfigurableConversionService conversionService) { + this.applicationContext.getBeansOfType(GenericConverter.class) + .values() + .forEach(conversionService::addConverter); + this.applicationContext.getBeansOfType(ConverterFactory.class) + .values() + .forEach(conversionService::addConverterFactory); + this.applicationContext.getBeansOfType(Converter.class) + .forEach((name, converter) -> addConverter(conversionService, name, converter)); + } + + @SuppressWarnings({ "rawtypes", "unchecked" }) + private void addConverter(ConfigurableConversionService conversionService, String beanName, Converter converter) { + ResolvableType type = beanType(beanName, converter).as(Converter.class); + Class source = type.getGeneric(0).resolve(); + Class target = type.getGeneric(1).resolve(); + if (source != null && target != null) { + conversionService.addConverter(source, target, converter); + } + else { + conversionService.addConverter(converter); + } + } + + private ResolvableType beanType(String beanName, Object bean) { + if (this.applicationContext instanceof ConfigurableApplicationContext cac + && cac.getBeanFactory().containsBeanDefinition(beanName)) { + return cac.getBeanFactory().getMergedBeanDefinition(beanName).getResolvableType(); + } + return ResolvableType.forClass(bean.getClass()); + } + private Object getTagetObject(Class declaringClass) { try { return this.applicationContext.getBean(declaringClass); diff --git a/spring-shell-core/src/test/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBeanTests.java b/spring-shell-core/src/test/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBeanTests.java index afa8fc5bc..dfae79d8d 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBeanTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/core/command/annotation/support/CommandFactoryBeanTests.java @@ -17,16 +17,28 @@ import java.lang.reflect.Method; import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.ApplicationContext; +import org.springframework.core.convert.TypeDescriptor; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.ConverterFactory; +import org.springframework.core.convert.converter.GenericConverter; +import org.springframework.core.convert.support.ConfigurableConversionService; +import org.springframework.shell.core.command.CommandContext; import org.springframework.shell.core.command.CommandOption; +import org.springframework.shell.core.command.adapter.MethodInvokerCommandAdapter; import org.springframework.shell.core.command.annotation.Command; import org.springframework.shell.core.command.annotation.CommandGroup; import org.springframework.shell.core.command.annotation.Option; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -35,10 +47,18 @@ */ class CommandFactoryBeanTests { + private static ApplicationContext mockApplicationContext() { + ApplicationContext context = mock(ApplicationContext.class); + when(context.getBeansOfType(Converter.class)).thenReturn(Collections.emptyMap()); + when(context.getBeansOfType(GenericConverter.class)).thenReturn(Collections.emptyMap()); + when(context.getBeansOfType(ConverterFactory.class)).thenReturn(Collections.emptyMap()); + return context; + } + @Test void testOptionNames() { // given - ApplicationContext context = mock(ApplicationContext.class); + ApplicationContext context = mockApplicationContext(); when(context.getBean(TestClass.class)).thenReturn(new TestClass()); CommandFactoryBean commandFactoryBean = new CommandFactoryBean(TestClass.class.getDeclaredMethods()[0]); commandFactoryBean.setApplicationContext(context); @@ -67,7 +87,7 @@ void testOptionNames() { @Test public void testCommandGroup() { // given - ApplicationContext context = mock(ApplicationContext.class); + ApplicationContext context = mockApplicationContext(); when(context.getBean(GreetingCommands.class)).thenReturn(new GreetingCommands()); Method[] declaredMethods = GreetingCommands.class.getDeclaredMethods(); Method hiMethod = Arrays.stream(declaredMethods) @@ -88,7 +108,7 @@ public void testCommandGroup() { @Test public void testCommandGroupOverride() { // given - ApplicationContext context = mock(ApplicationContext.class); + ApplicationContext context = mockApplicationContext(); when(context.getBean(GreetingCommands.class)).thenReturn(new GreetingCommands()); Method[] declaredMethods = GreetingCommands.class.getDeclaredMethods(); Method byeMethod = Arrays.stream(declaredMethods) @@ -106,6 +126,123 @@ public void testCommandGroupOverride() { assertEquals("Farewell Commands", result.getGroup()); } + @Test + void converterBeansAreAppliedToConfigurableConversionService() throws Exception { + ApplicationContext context = mockConverterApplicationContext(); + doReturn(Map.of("messageConverter", new MessageConverter())).when(context).getBeansOfType(Converter.class); + + runConvertCommandAndAssert(context); + } + + @Test + void genericConverterBeansAreAppliedToConfigurableConversionService() throws Exception { + ApplicationContext context = mockConverterApplicationContext(); + doReturn(Map.of("messageGenericConverter", new MessageGenericConverter())).when(context) + .getBeansOfType(GenericConverter.class); + + runConvertCommandAndAssert(context); + } + + @Test + void converterFactoryBeansAreAppliedToConfigurableConversionService() throws Exception { + ApplicationContext context = mockConverterApplicationContext(); + doReturn(Map.of("messageConverterFactory", new MessageConverterFactory())).when(context) + .getBeansOfType(ConverterFactory.class); + + runConvertCommandAndAssert(context); + } + + private static ApplicationContext mockConverterApplicationContext() { + ApplicationContext context = mockApplicationContext(); + when(context.getBean(ConverterTarget.class)).thenReturn(new ConverterTarget()); + when(context.getBean(ConfigurableConversionService.class)) + .thenThrow(new NoSuchBeanDefinitionException(ConfigurableConversionService.class)); + when(context.getBean(jakarta.validation.Validator.class)) + .thenThrow(new NoSuchBeanDefinitionException(jakarta.validation.Validator.class)); + return context; + } + + private static void runConvertCommandAndAssert(ApplicationContext context) throws Exception { + Method method = Arrays.stream(ConverterTarget.class.getDeclaredMethods()) + .filter(m -> m.getName().equals("run")) + .findFirst() + .orElseThrow(); + CommandFactoryBean factory = new CommandFactoryBean(method); + factory.setApplicationContext(context); + MethodInvokerCommandAdapter command = (MethodInvokerCommandAdapter) factory.getObject(); + + ConverterTarget.lastSeen = null; + CommandContext ctx = mock(CommandContext.class); + when(ctx.getOptionByLongName("msg")).thenReturn(CommandOption.with().longName("msg").value("hello").build()); + when(ctx.outputWriter()).thenReturn(new java.io.PrintWriter(new java.io.StringWriter())); + command.doExecute(ctx); + + assertThat(ConverterTarget.lastSeen).isNotNull(); + assertThat(ConverterTarget.lastSeen.text).isEqualTo("hello"); + } + + static class Message { + + String text; + + } + + static class MessageConverter implements Converter { + + @Override + public Message convert(String source) { + Message m = new Message(); + m.text = source; + return m; + } + + } + + static class MessageGenericConverter implements GenericConverter { + + @Override + public java.util.Set getConvertibleTypes() { + return java.util.Set.of(new GenericConverter.ConvertiblePair(String.class, Message.class)); + } + + @Override + public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) { + Message m = new Message(); + m.text = (String) source; + return m; + } + + } + + static class MessageConverterFactory implements ConverterFactory { + + @Override + public Converter getConverter(Class targetType) { + return source -> { + try { + T m = targetType.getDeclaredConstructor().newInstance(); + m.text = source; + return m; + } + catch (ReflectiveOperationException e) { + throw new IllegalStateException(e); + } + }; + } + + } + + static class ConverterTarget { + + static Message lastSeen; + + @Command(name = "convert") + public void run(@Option(longName = "msg", required = true) Message msg) { + lastSeen = msg; + } + + } + static class TestClass { @Command(name = "hello") diff --git a/spring-shell-test-autoconfigure/src/test/java/org/springframework/shell/test/autoconfigure/CustomConverterShellTests.java b/spring-shell-test-autoconfigure/src/test/java/org/springframework/shell/test/autoconfigure/CustomConverterShellTests.java new file mode 100644 index 000000000..4250f6c5c --- /dev/null +++ b/spring-shell-test-autoconfigure/src/test/java/org/springframework/shell/test/autoconfigure/CustomConverterShellTests.java @@ -0,0 +1,115 @@ +/* + * Copyright 2026-present 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.shell.test.autoconfigure; + +import java.util.function.Function; + +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.SpringBootApplication; +import org.springframework.context.annotation.Bean; +import org.springframework.core.convert.converter.Converter; +import org.springframework.shell.core.command.CommandContext; +import org.springframework.shell.core.command.CommandOption; +import org.springframework.shell.core.command.annotation.Command; +import org.springframework.shell.core.command.annotation.Option; +import org.springframework.shell.test.ShellAssertions; +import org.springframework.shell.test.ShellScreen; +import org.springframework.shell.test.ShellTestClient; +import org.springframework.test.context.ContextConfiguration; + +/** + * Regression coverage for gh-1352. Mirrors the scenarios from the issue reproducer: an + * annotation-based command with a custom-typed option, an annotation-based command with a + * plain String option, and a programmatically-registered command, all running against a + * context that defines a {@code Converter} bean. + * + * @author David Pilar + */ +@ShellTest +@ContextConfiguration(classes = CustomConverterShellTests.App.class) +class CustomConverterShellTests { + + @Test + void customConverterIsAppliedToAnnotatedCommandOption(@Autowired ShellTestClient client) throws Exception { + ShellScreen shellScreen = client.sendCommand("custom --msg hello"); + + ShellAssertions.assertThat(shellScreen).containsText("hello"); + } + + @Test + void plainStringOptionStillWorks(@Autowired ShellTestClient client) throws Exception { + ShellScreen shellScreen = client.sendCommand("simple --msg hello"); + + ShellAssertions.assertThat(shellScreen).containsText("hello"); + } + + @Test + void programmaticCommandStillWorks(@Autowired ShellTestClient client) throws Exception { + ShellScreen shellScreen = client.sendCommand("programmatic --msg hello"); + + ShellAssertions.assertThat(shellScreen).containsText("hello"); + } + + @SpringBootApplication + static class App { + + @Command(name = "custom") + public String custom(@Option(longName = "msg", required = true) Message msg) { + return msg.getText(); + } + + @Command(name = "simple") + public String simple(@Option(longName = "msg", required = true) String msg) { + return msg; + } + + @Bean + org.springframework.shell.core.command.Command programmatic() { + Function action = ctx -> ctx.getOptionByName("msg").value(); + return org.springframework.shell.core.command.Command.builder() + .name("programmatic") + .options(CommandOption.with().longName("msg").type(Message.class).build()) + .execute(action); + } + + @Bean + Converter messageConverter() { + return source -> { + Message m = new Message(); + m.setText(source); + return m; + }; + } + + } + + static class Message { + + private String text; + + public String getText() { + return text; + } + + public void setText(String text) { + this.text = text; + } + + } + +}