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

Make behaviour of @DefaultImplementation consistent #10820

Merged
merged 3 commits into from May 17, 2024

Conversation

graemerocher
Copy link
Contributor

@graemerocher graemerocher commented May 13, 2024

The @DefaultImplementation annotation is currently confusing since it doesn't influence bean selection but only the result of @Replaces. This PR makes it so that the annotation can also be used to control bean selection which makes it more logical and less confusing.

@graemerocher graemerocher added the type: improvement A minor improvement to an existing feature label May 13, 2024
Copy link
Collaborator

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is a good idea but we should document this in the "Default Implementation" section of the docs. We should mention that @primary still has precedence over @DefaultImplementation

@graemerocher graemerocher requested a review from sdelamo May 14, 2024 15:17
Copy link

sonarcloud bot commented May 15, 2024

@dstepanov
Copy link
Contributor

Personally, I don't like the idea of @DefaultImplementation defined on the interface, considering this can be implemented with priority.

I think this will not work correctly in a case class SomeImpl{X,Y, Z} extends SomeBase; abstract class SomeBase implements ISomeWithDefault and injecting SomeBase. The correct way would be to use some annotation processing to map the class name on which the annotation is applied to @DefaultImplementation and check if the inject point is trying to resolve that class.

@graemerocher
Copy link
Contributor Author

right, I added this because the current implementation is confusing and because Guice supports this concept https://github.com/google/guice/blob/2b37f020862e45be58f246a63c2f97fdaff0d433/core/test/com/google/inject/ImplicitBindingTest.java#L54

The example you mention should work fine, we just need to make sure the implementing type matches the type to be injected.

If we don't agree to this change then I can maybe implement is some other way in the Guice bridge, but I would then suggest we deprecate @DefaultImplementation all together because the only place it is used is with @Replaces and the name and behaviour are very misleading

@dstepanov
Copy link
Contributor

I'm OK with this. Does it work to not have any scope defined on the implementation like it's in the JavaDoc of DefaultImplementation?

@graemerocher
Copy link
Contributor Author

yes it does

sdelamo added a commit that referenced this pull request May 17, 2024
Cherry picks the javadoc improvements done in #10820 to `Ordered`
@sdelamo sdelamo merged commit f068cb5 into 4.5.x May 17, 2024
17 checks passed
@sdelamo sdelamo deleted the default-implementation-without-replaces branch May 17, 2024 10:36
sdelamo added a commit that referenced this pull request May 24, 2024
Cherry picks the javadoc improvements done in #10820 to `Ordered`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants