Ensure bean definitions are only created for defined beans. Fixes #3561

[jameskleeh]

Note that this is technically a breaking change, however this is more of an extension of the work already done in 2.0.0 to ensure that bean definitions aren't created for things not explicitly defined as a bean. I think the potential impact is low

[graemerocher]

I have concerns about the breaking nature of this change. Also the correct fix for the issue is to check if the bean is isAbstract() and filter it out from being used as a event listener

[jameskleeh]

@graemerocher I think only applying this change to abstract classes leaves out many other undesired possibilities. For example:

Non declared beans can't have executable methods
The validated annotation will currently not be applied to non declared beans with constraints
Post construct will not work in non declared beans
Pre destroy will not work in non declared beans

I think explicitly excluding abstract classes is more of a bandaid around the larger issue. Groovy currently behaves today as Java would with these changes. I simply added the test case to the inject-groovy module and it passed without any further changes.

The changes in our code base were classes that had either @nAmed or @primary or both, but no other bean annotations. I think there is a possible impact, but I think its a minor one and is worth the fix compared to leaving the other possible errors out there until 3.0.0

[graemerocher]

Ok if it is Groovy specific that changes matters. Java and Groovy should be consistent

[jameskleeh]

The change is for the Java/Kotlin. I just pointed out that Groovy already works this way (requires beans to be declared)

[graemerocher]

Hmm then I am unsure frankly, and the bug seems to be in Groovy. In general we should be aiming for less ceremony and having to declare a scope when just a @Named(..) would suffice seems a regression and a breaking change that we shouldn't contemplate. I'm at this stage -1 on this change in patch release.

You could argue that using @Validated (ie an AOP advice and which is the original reported issue) alone shouldn't result in a bean on an abstract class but this change seems to go beyond that.

[ilopmar]

I think it makes sense to fix the current behavior if we consider it a bug in the next 2.0.1 release. Keeping in mind that Micronaut 2.0 was released less than 2 weeks ago, this won't probably affect any (or not many users). In any case, if some folks are affected, the solution for them is pretty simple, annotate the classes they want to make a bean with singleton/prototype as they need.

[graemerocher]

@ilopmar whilst I agree the change here in the PR is more invasive than that and alters behaviour from 1.x. Why do you need to declare a scope for example if you have already declared a qualifier?

[jameskleeh]

@graemerocher It's beyond just a qualifier though. You can apply AOP to a class and it will be a bean as well.

[graemerocher]

That is behaviour that is questionable I agree. Since neither @Validated nor @Around are qualifiers or scopes these shouldn't trigger a bean I agree, but this PR as written goes way beyond that and disables bean generated for @Named(..) which is large breaking change IMO.

[jameskleeh]

Just to give my opinion on the qualifier though - I think the issue is consistency. If we allowed beans to be created just by the presence of a qualifier then beans without qualifiers need a scope and beans with qualifiers don't. That isn't consistent and isn't documented. I think users should be explicit about whether the bean is a prototype or a singleton and not rely on framework defaults.

Edit: In addition the issues I described above with executable methods requiring a scope makes me believe this should go into 2.0.1.

[graemerocher]

I personally disagree, it adds way more ceremony to the framework than is needed. There are other areas where we have too much ceremony for example it shouldn't be necessary to use @Inject on fields when there is already a qualifier. We should be removing ceremony where possible and not adding it.

Regardless of my opinion however, the ship has already sailed with regards to 2.0.0 and if we are following semantic versioning this shouldn't be changed in a patch release.

[jameskleeh]

We have always weighed the potential impact of a breaking change with the benefit and sometimes made changes in patch/minor releases, so I don't see a reason why that wouldn't happen here as well.

Regarding the ceremony, in general I agree. Not requiring @Inject where a qualifier is present makes sense because for example we don't require it on constructors. In addition it's not adding any value because there is no decision to make.

That isn't the case with classes though. A class with @Named and no other annotations will result in a prototype bean, which is most likely not the desired outcome. I think most users would expect it to result in a singleton bean. I think if you would like to remove the ceremony for classes, we should change the default scope to singleton.

[jameskleeh]

In one case even in our code we were using Groovy's @Singleton with @Named and it was resulting in a prototype bean https://github.com/micronaut-projects/micronaut-core/pull/3617/files#diff-13e9911334254d8a91eae0286448feb2R18

[graemerocher]

I agree that is undesirable that that happened, however that code was in a test.

Related to this we had #1859 open for a while and assigned to the 2.0 milestone and in the end didn't implement due to feedback from the community regarding consistency with other DI frameworks, see #1859 (comment)

If we now implement this in a patch release that would be a bad breaking change. If we want to take this forward I suggest altering the PR to change the behaviour of isDeclaredBean and adding concreteClassMetadata.hasStereotype(Qualifier.class) to it

[jameskleeh]

@graemerocher Sounds like a good compromise for this release. If we update isDeclaredBean then the previous issues with executable methods will be fixed as well as the AOP advice causing beans to be generated. At that point the larger decision about them being beans or not, or the default scope can be left for 3.0.