Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GenericTypeResolver broken in 6.2.0 (possible regression of #24963) #33968

Closed
rPraml opened this issue Nov 26, 2024 · 7 comments
Closed

GenericTypeResolver broken in 6.2.0 (possible regression of #24963) #33968

rPraml opened this issue Nov 26, 2024 · 7 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: superseded An issue that has been superseded by another

Comments

@rPraml
Copy link
Contributor

rPraml commented Nov 26, 2024

Hello, I want to report a possible regression in 6.2.0

We have some rest controllers that use jackson with @JsonTypeInfo annotations.

With 6.1.x the attached code works and returns the expected JSON: [ { "type" : "one" }, { "type" : "two" } ]
With 6.2.0, the type properties are missing and [ {}, {} ] is returned.

I've tracked down, that this commit may have changed the behaviour: e788aeb (#24963)

I'm unsure, if this is "works as designed" or if that code path got lost through this commit:
The method declaration public <T extends BaseType> List<T> get() may be a bit strange, but valid java syntax.
When I use this method declaration public List<BaseType> get(), the controller will work again. (@quaff, @jhoeller FYI)

@RestController
public class TestController {
	@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
	@JsonSubTypes({
			@JsonSubTypes.Type(value = SubType1.class, name = "one"),
			@JsonSubTypes.Type(value = SubType2.class, name = "two")
	})
	public static class BaseType {

	}

	public static class SubType1 extends BaseType {
	}

	public static class SubType2 extends BaseType {
	}

	@GetMapping(value = "/test", produces = MediaType.APPLICATION_JSON_VALUE)
	public <T extends BaseType> List<T> get() {
		List<T> list = new ArrayList<>();
		list.add((T) new SubType1());
		list.add((T) new SubType2());
		return list;
	}
}
@rPraml rPraml changed the title Generic type resolving broken in 6.2.0 (regression of GH-24963) Generic type resolving broken in 6.2.0 (regression of #24963) Nov 26, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 26, 2024
@rPraml rPraml changed the title Generic type resolving broken in 6.2.0 (regression of #24963) Generic type resolving broken in 6.2.0 (possible regression of #24963) Nov 26, 2024
@ThanksForAllTheFish
Copy link

I think this is similar to something I observed too, but tracked to #20727

In our case, a bean defined as fun <V> kafkaProducerFactory(valueSerializer: Serializer<V>): ProducerFactory<String?, V> (org.springframework.kafka.core.ProducerFactory) was not recognized as a valid candidate to https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaAutoConfiguration.java#L92 (expecting a ProducerFactory<Object, Object> kafkaProducerFactory

	@Bean
	@ConditionalOnMissingBean(KafkaTemplate.class)
	public KafkaTemplate<?, ?> kafkaTemplate(ProducerFactory<Object, Object> kafkaProducerFactory,
			ProducerListener<Object, Object> kafkaProducerListener,
			ObjectProvider<RecordMessageConverter> messageConverter) {
		PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull();
		KafkaTemplate<Object, Object> kafkaTemplate = new KafkaTemplate<>(kafkaProducerFactory);
		messageConverter.ifUnique(kafkaTemplate::setMessageConverter);
		map.from(kafkaProducerListener).to(kafkaTemplate::setProducerListener);
		map.from(this.properties.getTemplate().getDefaultTopic()).to(kafkaTemplate::setDefaultTopic);
		map.from(this.properties.getTemplate().getTransactionIdPrefix()).to(kafkaTemplate::setTransactionIdPrefix);
		map.from(this.properties.getTemplate().isObservationEnabled()).to(kafkaTemplate::setObservationEnabled);
		return kafkaTemplate;
	}

I changed our bean definition to return ProducerFactory<out Any, Any>, but I am also worried there might other impacts and I just have to trust our test to cover those

@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 27, 2024
@jhoeller jhoeller self-assigned this Nov 27, 2024
@jhoeller jhoeller added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 27, 2024
@jhoeller jhoeller added this to the 6.2.1 milestone Nov 27, 2024
@sbrannen sbrannen changed the title Generic type resolving broken in 6.2.0 (possible regression of #24963) Generic type resolution broken in 6.2.0 (possible regression of #24963) Nov 30, 2024
@jhoeller
Copy link
Contributor

jhoeller commented Dec 5, 2024

@ThanksForAllTheFish see #33982 (comment) for a tweak that might fix your scenario. Feel free to give the upcoming 6.2.1 snapshot a try...

@rPraml if your case is affected by #24963, it might require a separate fix. You could try a 6.2.1 snapshot with #33982 for a start. If the problem remains, a reproducer along the lines of a ResolvableType unit test would be great.

@jhoeller
Copy link
Contributor

@ThanksForAllTheFish @rPraml any chance you could give the latest 6.2.1 snapshot (to be found at repo.spring.io) a try before the 6.2.1 release on Thursday?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 11, 2024
@jhoeller jhoeller modified the milestones: 6.2.1, 6.2.x Dec 11, 2024
@rPraml
Copy link
Contributor Author

rPraml commented Dec 12, 2024

@jhoeller Thanks for the reminder. Unfortunately the problem is not yet fixed.
I've written a small test case:

package org.springframework.test.web.servlet.samples.standalone;

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import org.junit.jupiter.api.Test;
import org.springframework.http.MediaType;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;

import java.util.ArrayList;
import java.util.List;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup;

/**
 * Demonstrates if returning a List&gt;? extends SomeType&lt> can be correctly serialized.
 *
 * @author Roland Praml
 */
public class GenericReturnTest {
	@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
	@JsonSubTypes({
			@JsonSubTypes.Type(value = SubType1.class, name = "one"),
			@JsonSubTypes.Type(value = SubType2.class, name = "two")
	})
	public static class BaseType {

	}

	public static class SubType1 extends BaseType {
	}

	public static class SubType2 extends BaseType {
	}

	@Test
	public void genericReturnTest() throws Exception {

		standaloneSetup(new GenericReturnTest.Controller()).build()
				.perform(get("/genericReturnList").accept(MediaType.APPLICATION_JSON))
				.andExpect(status().isOk())
				.andExpect(content().contentType("application/json"))
				.andExpect(jsonPath("$[0].type").value("one"))
				.andExpect(jsonPath("$[1].type").value("two"));
	}

	@RestController
	public static class Controller {
		@GetMapping(value = "/genericReturnList", produces = MediaType.APPLICATION_JSON_VALUE)
		public <T extends BaseType> List<T> get() {
			List<T> list = new ArrayList<>();
			list.add((T) new SubType1());
			list.add((T) new SubType2());
			return list;
		}

		@GetMapping(value = "/genericReturnCs", produces = MediaType.APPLICATION_JSON_VALUE)
		public <T extends String> List<T> get2() {
			List<T> list = new ArrayList<>();
			list.add((T) "a");
			list.add((T) "b");
			return list;
		}

	}
}

I've tracked down the difference to ResolvableType.forClassWithGenerics(Class<?> clazz, ResolvableType... generics)
and I found this commit e788aeb with bisect.

With the failing commit I get as input:
grafik

Before the commit, I get as input:
grafik

You see, that in the failing case, the type.toString() is ? in the other case it is the BaseType

I assume, that there is a resolve missing. Changing the method as below will fix my problem (but breaks other tests)

        Type[] arguments = new Type[generics.length];
        for (int i = 0; i < generics.length; i++) {
            ResolvableType generic = generics[i];
-           Type argument = (generic != null ? generic.getType() : null);
+           Type argument = (generic != null ? generic.resolve() : null);
            arguments[i] = (argument != null && !(argument instanceof TypeVariable) ? argument : variables[i]);
        }   

Unfortunately, I don't know enough about what's going on here in detail, so it's hard for me to provide a good solution, but I hope my research can help you resolve the issue.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 12, 2024
@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.2 Dec 12, 2024
@jhoeller
Copy link
Contributor

@rPraml thanks for the research and the test case! Let's sort this out for 6.2.2 then.

rPraml added a commit to FOCONIS/spring-framework that referenced this issue Dec 13, 2024
rPraml added a commit to FOCONIS/spring-framework that referenced this issue Dec 13, 2024
@rPraml
Copy link
Contributor Author

rPraml commented Dec 13, 2024

This didn't leave me alone and I made a second attempt.
Maybe you can take a look at the provided PR.

(It's OK, if this comes with 6.2.2 or later, as we have a workaround for now)

@jhoeller jhoeller changed the title Generic type resolution broken in 6.2.0 (possible regression of #24963) GenericTypeResolver broken in 6.2.0 (possible regression of #24963) Jan 13, 2025
@snicoll
Copy link
Member

snicoll commented Jan 15, 2025

Closing in favor of the PR #34086

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2025
@snicoll snicoll removed the type: regression A bug that is also a regression label Jan 15, 2025
@snicoll snicoll added status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided labels Jan 15, 2025
@snicoll snicoll removed this from the 6.2.2 milestone Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

5 participants