Skip to content

Introduce ProxyHints.registerJdkProxy(String...) #28781

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

Closed
sbrannen opened this issue Jul 10, 2022 · 6 comments
Closed

Introduce ProxyHints.registerJdkProxy(String...) #28781

sbrannen opened this issue Jul 10, 2022 · 6 comments
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement

Comments

@sbrannen
Copy link
Member

Since users might not have a concrete need to work with TypeReference, we should introduce ProxyHints.registerJdkProxy(String...) to simplify use of the API for registering a dynamic proxy based on fully qualified class names of the required interfaces.

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Jul 10, 2022
@sbrannen sbrannen added this to the 6.0.0-M5 milestone Jul 10, 2022
@sbrannen sbrannen self-assigned this Jul 10, 2022
@snicoll
Copy link
Member

snicoll commented Jul 10, 2022

I don't think we should do this. If we do, then every place that accepts a TypeReference must add a String variant, and I don't think that we should do that either as it would bloat the API quite a bit.

If you don't have a class handy, then a TypeReference is the way to go.

@sbrannen
Copy link
Member Author

I had already implemented the feature and committed it before I saw your comments, so I'm reopening this issue to discuss the topic within the team.

I don't think we should do this. If we do, then every place that accepts a TypeReference must add a String variant, and I don't think that we should do that either as it would bloat the API quite a bit.

If you don't have a class handy, then a TypeReference is the way to go.

I'd argue that TypeReference is more of an internal implementation detail that normal users need not be bothered with.

There may be some particular use cases where normal users need to work with TypeReference, but for the most common use cases users will be happier with supplying a Class or String. Furthermore, supporting String will remove lots of superfluous TypeReference.of(<string>) occurrences within user code.

Playing devil's advocate... we don't require users to supply a TypeReference for a Class, so why should we require them to supply a TypeReference for a String?

As a concrete example, this issue was a requirement for #28745 to improve the developer experience. The newly introduced AopProxyUtils.completeJdkProxyInterfaces(*) methods could of course be refactored to return TypeReference[] instead of Class<?>[] and String[], but that seemed unnatural and unnecessary.

@sbrannen sbrannen reopened this Jul 10, 2022
sbrannen added a commit that referenced this issue Jul 11, 2022
Since gh-28781 is still open for team discussion, this commit removes
the `String...` variant for AopProxyUtils.completeJdkProxyInterfaces.

Depending on the outcome of gh-28781, this method may be reintroduced
in 6.0 M6.

Closes gh-28745
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Jul 11, 2022
@sbrannen sbrannen modified the milestones: 6.0.0-M5, 6.0.0-M6 Jul 11, 2022
@sbrannen
Copy link
Member Author

Team Decision: revert the changes made in commit b560c10 and revisit this general topic (i.e., use of TypeReference in public-facing APIs) in the 6.0 M6 time frame.

@snicoll
Copy link
Member

snicoll commented Jul 11, 2022

I'd argue that TypeReference is more of an internal implementation detail that normal users need not be bothered with.

Why is that? It's a public class with public of convenient methods. Nothing there seems to indicate it is an internal implementation.

Furthermore, supporting String will remove lots of superfluous TypeReference.of() occurrences within user code.

If that is a problem, then let's address it separately. SimpleTypeReference is essentially a wrapper around the String value and I don't expect a lot of them to be created. Also, this should only be triggered at build time when AOT runs.

It doesn't feel right that we focus on one of the many methods that accept either a Class or a TypeReference in the hint package.

Playing devil's advocate... we don't require users to supply a TypeReference for a Class, so why should we require them to supply a TypeReference for a String?

Because using Class should be the vast majority of use cases and therefore it seemed a good idea to offer a shortcut for it. If you start offering a String-based approach, you're covering 99.9% of the use cases so why offer a variant for TypeReference then? You could then remove those and keep that indeed private (for real this time) but what happens if you want to create a hint for a class you've generated. Surely, going from ClassName to the name of the class shouldn't be that hard but it can be tricky if you have an inner class. That's why TypeReference was created in the first place.

As a concrete example, this issue was a requirement for #28745 to improve the developer experience. The newly introduced AopProxyUtils.completeJdkProxyInterfaces(*) methods could of course be refactored to return TypeReference[] instead of Class<?>[] and String[], but that seemed unnatural and unnecessary.

I don't think this method should have been designed this way. Rather, I think it should have returned a Customizer of the JdkProxyHint.Builder. This way you could register the proxy the usual way, passing the customizer of the builder to complete the set of interfaces that it needs. Doing it this way also means you don't have to care what input the caller use for user interface.

@sbrannen
Copy link
Member Author

I'd argue that TypeReference is more of an internal implementation detail that normal users need not be bothered with.

Why is that? It's a public class with public of convenient methods. Nothing there seems to indicate it is an internal implementation.

We're not questioning whether TypeReference should be public, and we are not questioning its utility.

Rather, we are debating whether TypeReference should be so prevalent in the entry points into the AOT APIs.

Furthermore, supporting String will remove lots of superfluous TypeReference.of() occurrences within user code.

If that is a problem, then let's address it separately. SimpleTypeReference is essentially a wrapper around the String value and I don't expect a lot of them to be created. Also, this should only be triggered at build time when AOT runs.

In this regard, we are not talking about performance but rather about the developer experience and the API presented to the user for the most common use cases.

It doesn't feel right that we focus on one of the many methods that accept either a Class or a TypeReference in the hint package.

Indeed, and that's why we reverted the change for M5 in order to discuss the broader topic for M6.

Playing devil's advocate... we don't require users to supply a TypeReference for a Class, so why should we require them to supply a TypeReference for a String?

Because using Class should be the vast majority of use cases and therefore it seemed a good idea to offer a shortcut for it.

That's a good point and explains the original rationale for the decision. In your absence, we realized we might not have the full picture. So input like this helps to shape the discussion.

If you start offering a String-based approach, you're covering 99.9% of the use cases so why offer a variant for TypeReference then? You could then remove those and keep that indeed private (for real this time)

That is something the team wishes to explore in greater detail.

but what happens if you want to create a hint for a class you've generated. Surely, going from ClassName to the name of the class shouldn't be that hard but it can be tricky if you have an inner class. That's why TypeReference was created in the first place.

We definitely do not plan to make existing use cases more difficult. Rather, our goal is to simplify common use cases and simplify the entry points into the AOT APIs. If there are existing use cases that effectively require the use of TypeReference outside of internal framework code, we would want to retain that functionality.

As a concrete example, this issue was a requirement for #28745 to improve the developer experience. The newly introduced AopProxyUtils.completeJdkProxyInterfaces(*) methods could of course be refactored to return TypeReference[] instead of Class<?>[] and String[], but that seemed unnatural and unnecessary.

I don't think this method should have been designed this way. Rather, I think it should have returned a Customizer of the JdkProxyHint.Builder. This way you could register the proxy the usual way, passing the customizer of the builder to complete the set of interfaces that it needs. Doing it this way also means you don't have to care what input the caller use for user interface.

For that particular convenience feature, the team wanted to add new methods to AopProxyUtils while not making AopProxyUtils directly dependent on AOT-specific APIs. If we decide to follow your recommendations, we may want to move the code from AopProxyUtils to a new AOT-specific class in spring-aop.

In summary, there are many facets to discuss. In light of that, we should probably dedicate a portion of a team meeting to dive deeper into this topic.

@sbrannen
Copy link
Member Author

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2022
@sbrannen sbrannen removed their assignment Aug 25, 2022
@sbrannen sbrannen removed this from the 6.0.0-M6 milestone Aug 25, 2022
@sbrannen sbrannen added the status: superseded An issue that has been superseded by another label Aug 25, 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) status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants