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

Exclude sealed interfaces from auto-proxying (for JDK 17 compatibility) #27027

Closed
dreis2211 opened this issue Jun 6, 2021 · 6 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@dreis2211
Copy link
Contributor

dreis2211 commented Jun 6, 2021

Hi,

I recently opened an issue to track the needed stuff for Spring-Boot in order to support Java 17. spring-projects/spring-boot#26767. I think it makes sense to have the same for Spring Framework.

Known issues:

  1. Sealed interfaces need to be handled e.g. when creating proxies.
@Bean
@Scope(proxyMode = ScopedProxyMode.INTERFACES)
String alpha() {
	return "alpha";
}

Essentially, under the hood it retrieves all interfaces of String here which also includes ConstantDesc which is a sealed interface and cannot be used when creating a proxy. See ScopedProxyFactoryBean.setBeanFactory:

if (!isProxyTargetClass() || beanType.isInterface() || Modifier.isPrivate(beanType.getModifiers())) {
        // This should filter sealed interfaces I guess
	pf.setInterfaces(ClassUtils.getAllInterfacesForClass(beanType, cbf.getBeanClassLoader()));
}

After a first look this probably needs to be handled more centrally and more places are potentially affected by sealed interfaces.

There is likely more to do for Java 17 support, but I wanted to kick this off. Feel free to add to this issue or repurpose it to only handle the specific issue.

Let me know if I can help.

Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 6, 2021
@jhoeller jhoeller self-assigned this Jun 6, 2021
@jhoeller jhoeller added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 6, 2021
@jhoeller jhoeller added this to the 5.3.9 milestone Jun 6, 2021
@jhoeller jhoeller changed the title Add support for Java 17 Add support for sealed interfaces (Java 17) Jun 6, 2021
@jhoeller jhoeller changed the title Add support for sealed interfaces (Java 17) Exclude sealed interfaces from auto-proxying (for JDK 17 compatibility) Jun 8, 2021
@jhoeller jhoeller modified the milestones: 5.3.9, 5.3.10 Jul 7, 2021
@jhoeller
Copy link
Contributor

It seems sufficient to exclude sealed interfaces through a reflective Class.isSealed() check in the AopProxyUtils.completeProxiedInterfaces algorithm. I've locally tested this on JDK 17, seems to work fine so far. This will be in main for 5.3.9 tonight, would be great to know whether it also works for a Boot setup then...

@dreis2211
Copy link
Contributor Author

dreis2211 commented Jul 12, 2021

I will give it a try tomorrow. Thanks @jhoeller

@dreis2211
Copy link
Contributor Author

Seems to work just fine for the failing test in Boot.

@jhoeller
Copy link
Contributor

Cool, thanks for the quick turnaround!

@quaff
Copy link
Contributor

quaff commented Sep 29, 2021

@jhoeller Should sealed interfaces be filtered here?

see https://bugs.openjdk.java.net/browse/JDK-8269396

@LifeIsStrange
Copy link

What about Kotlin sealed classes?
Maybe that the same solution could be used given that Kotlin is making their sealed classes more interoperable/aligned with Java ones
https://youtrack.jetbrains.com/issue/KT-46778

Also note that for kotlin reflection might not be needed in the future https://youtrack.jetbrains.com/issue/KT-25871
And that inline sealed classes are a thing
https://youtrack.jetbrains.com/issue/KT-27576

Maybe that none of those things are relevant (haven't looked into this issue) but I wanted to share this, just in case

lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
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) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants