Allow excluding OverrideBean classes

[kdubb]

Reinstate OverrideBean’s ability to exclude the class its overriding from scanning; now as an annotation option. This allows mock producers, such as:

@Produces
@OverrideBean(exclude = true)
@Mock
SomethingWithInjectedDeps something;

To mock the class, while not requiring the mocking of its dependencies.

[manovotn]

Hello,

from our docs, we state that OverrideBean is an always enabled alternative stereotype.
What you describe in this PR sounds more like what specialization is in CDI, am I right?

What is the use case where you need to get rid of the original bean instead of just overriding it? To disable some producer on it?

[kdubb]

In the example code I provided, the idea is that we want to mock SomethingWithInjectedDeps. As its name attempts to imply, it has CDI dependencies (e.g. @Inject) that need to be satisfied. Since we are mocking the instance we don't care about those dependencies. The way ClassScanning and OverrieBean currently work, it adds the class SomethingWithInjectedDeps and searches it for dependencies.

The end result is that, with the current code, even though I am mocking the object I still have to satisfy all the dependencies of SomethingWithInjectedDeps because its added as a normal bean and according to CDI it may be injected at some point.

Allowing exclusion of SomethingWithInjectedDeps via OverrideBean allows us to produce a mocked version of SomethingWithInjectedDeps without concerning ourselves with any of its dependencies.

[manovotn]

I see, thanks for explanation.

The end result is that, with the current code, even though I am mocking the object I still have to satisfy all the dependencies of SomethingWithInjectedDeps because its added as a normal bean and according to CDI it may be injected at some point.

That's designed behaviour though - OverrideBean is a parallel approach to alternatives. For instance, you may have a producer on a such bean and you want that one to still work.

Rather then twisting this approach with alternatives, I think it would be better to have a look a specialization - can't your mock specialize the original bean? Then you could just add the new bean and it would work out of the box without additional changes.

If the answer is no, then we will probably do something as you suggested, only I would aim to document it as OverrideBean working as specialization (and naming the parameter in accordance with that).

[kdubb]

@manovotn After trying numerous things related to @Alternative and @Specializes. I realized that the definition of @OverrideBean as I had envisioned it was co-opted into basically a @EnableAlternative annotation.

We already have a more "CDI" way of enabling alternatives via @EnableAlternatives. The reason I say it is a more "CDI" way of doing things is because in CDI alternatives are generally enabled externally (e.g. via beans.xml). So using an external annotation like @EnableAlternatives is very similar and easy.

I suggest we return @OverrideBean to its original intention of replacing the definition of a bean. The name includes override which implies something beyond enabling an alternative; again you can easily enable alternatives via annotation. To replace a bean means we must exclude it from scanning (so that CDI never sees the original bean), which is what my original PR did when using @OverrideBean.

Interestingly another thing I noticed is that @OverrideBean, strictly speaking, need not be an enabled alternative stereotype as it is now. Since we are excluding the bean class from scanning CDI never sees the original and therefore need not work out which alternative is correct. Although, if the original class is added via other annotations (e.g. @AddBeanClasses), leaving the annotation be an enabled alternative marker ensures the bean marked with it is always used.

Once we agree on the correct direction I'll submit a PR to implement the changes. Our current test set requires this exclusion functionality. I only recently discovered the differences when I attempted to switch back to the latest published version.

[kdubb]

FYI, I forgot to explicitly mention why @Specializes doesn't work for this.

1. @Specializes requires that you subclass the original bean class or the class in which its producer method is defined.
   • This can require changes to non-test code and only succeeds in skipping dependency requirements when the bean is using constructor injection.
2. @Specializes doesn't allow you to replace a bean defined by class annotation with a bean produced by a field or method; given the above requirement, it simply doesn't work that way.
   • Using @OverrideBean with exclusion specifically allows this and provides easy "no change" mocking as demonstrated previously.

[manovotn]

Hmm I can see why specialization doesn't quite cut it for you.

It is true that the functionality of OverrideBean overlaps with the simpler approach of declaring an enabled alternative in the test.
What's bugging me is that this will break any app possibly using OverrideBean as a selected alternative. In that case, having a separate annotation for what you are trying to achieve might be safer?

Also, your solution of excluding the classes might have certain flaws - it operates on return types which mean that should you return interface as a type, any actual bean with the implementation type will still be "discovered and active", right? That is actually one of the reasons why true specialization requires subclassing as without it you bump into weird situations like this.

@mkouba @nziakova thoughts?

[mkouba]

Hey @kdubb!

1. Why do you actually add the bean class you're trying to "override" to your test configuration? If you want to mock a bean backed by a class you don't really need to add the bean class to the bean archive.
2. A simple example would be really helpful. I don't quite understand the use case.

[kdubb]

@mkouba The example at the beginning is the example. I'm not adding the bean I want to override manually but the auto-configuration scanning picks it up due to it's @Produces annotation.

The OverrideBean annotation is needed to "disable" adding the overridden class via scanning. It's also why I originally chose a name (OverrideBean) that had no counterpart in CDI because this is solely about controlling the scanning.

[mkouba]

@kdubb Ok, so I think I finally understand your use case. I wonder if we could add a special annotation like @Exclude to handle a more general use case -> to specify the set of bean classes that shoudn't be added by WeldJunit5AutoExtension, i.e. your example could be rewritten as follows:

@EnableAutoWeld
@Exclude(Foo.class) // -> Foo is normally added due to fakeFoo producer field
class ProducesAlternativeTest {

  @Produces
  @Mock
  Foo fakeFoo = new Foo("non-baz");
}

[kdubb]

@mkouba Due to the seeming resistance to reverting @OverrideBean to what I originally intended, I've returned @OverrideBean to its previous released state and added ProducesOverride in its place & added proper Javadoc to it.

Using something like your suggested @Exclude(classes) means one has to collect all the overrides in one place at the top of the test. It's a bit easier to just add @ProducesOverride to the producer method or producer field that you want to override (see aforementioned Javadoc).

Note At this point OverrideBean's documentation is essentially wrong. It uses the word "override" to reference activating an alternative; which, as I've shown, doesn't override anything. Also, there is already a method to do this with EnableAlternatives/EnableAlternativeStereotypes which more closely match CDI's configuration style. It would seem deprecating and removing @OverrideBean in the future would be in order.

[manovotn]

It's not resistance, there simply wasn't any activity for some while.

I checked your PR and have few comments:

• There are no tests included
• The solution doesn't work
  • I actually created a test using what you describe in javadoc and it didn't work, both beans will be found by Weld - the original one and the one from producer leading to an exception
  • This is probably because you are still adding packages to Weld when scanning @AddPackages as can be seen here; this is done on the very first pass when you are scanning the test class itself
  • There are many ways to add beans to deployment so not only @AddPackages is affected
• Even if this worked, then you will always be excluding just the type you see on producer, what about hierarchies of bean? You can have Bar implements Foo and Foo implements Something, then you create an override for Foo but the Bar will still be found (via package addition or something) so you won't really prevent that becoming a bean and will end up with an exception again.

[erilor]

I stumbled upon this PR looking for a solution for the same problem as kdubb (trying to mock dependencies from injected beans). I actually think the proposed solution could work with a name change of the annotation to @ProduceWithoutScan, @ExcludeFromClassScan or something to that effect. That seems to be what I does and I think it would be enough for me at least.

If I added the class again some other way (e.g. addPackages) I would actually prefer an exception. Since then I’ve probably made a mistake.

Do you think that would work?

[manovotn]

If I added the class again some other way (e.g. addPackages) I would actually prefer an exception. Since then I’ve probably made a mistake.

I don't think this is possible. There is automagical scanning in play which simply will discover the bean and then add all its dependencies. You can see the login inside the ClassScanning class in the project. After that is done you can try and mock some of them by providing alternative implementations by it will always be a partial solution as you cannot solve all the cases (see my comment above) and combinations.

I'll have to take yet another look at the proposed solution here and how to actually make it work because last time I checked it didn't work at all. For next release (mid month to end of April?) I would like to have this solved one way or the other. If we can manage to have a supplement for what you need, then I think we should also deprecate @OverrideBean along the way as it doesn't have much value.

Obviously, if you want to contribute, be my guest :)
This PR is a good starting point, but needs testing and fixes to how the scanning works.

[kdubb]

@manovotn Apologies, I haven't had time to circle back to this PR. I will say this... you suggested this PR "doesn't work"; it actually does. We are using it currently (actually required for us) in a local version of the project to solve the problem as I've described it. Hopefully I'll get time soon to bring this PR inline with requirements for acceptance.

[manovotn]

Then you probably use different scenario from what I tested. Like I said, I tried to put what was in javadoc into actual test, and that failed to pass. Was a long time ago, so I don't recall the details from the top of my head.

[erilor]

@manovotn I pushed an example building on the change of @kdubb. I agree with him that his change works (even though the example does not). Hopefully my test will illustrate what I intended (and perhaps also what @kdup intended).

I also apologize in advance for any GitHub-mistakes, this was my first push =)

[kdubb]

@manovotn, @erilor Please take a look at my latest changes. I think (hope!) they are something we can all agree upon is a better direction

@manovotn You were correct. The code worked in our "complex" cases but didn't work in a simple case; even the simple that was case cited as an example. I've reimplemented exclusion via a CDI extension using veto to address this.

I've renamed the annotation to ExcludeBean. This should make clear what the annotation is actually doing with respect to automatic discovery/scanning. I've also added ExcludeBeans as a convenience for those that might want to add it to the top of the test instead of on individual fields.

The Javadoc has been changed to describe what it's doing and the limitations of its exclusion abilities have been noted. Hopefully it is clear to all how the annotation works and what its general use case is. I've also implemented 2 simple test cases that validate each of the annotations.

Finally, I removed OverrideBean because its javadoc is also very misleading and competes with ExcludeBean without correctly achieving that goal.

[erilor]

@kdubb Works like charm! Would be greate if this could get in to the next release.

[mkouba]

I really like the idea of @ExcludeBeans but I would rename the annotation to @ExcludeBeanClasses to align with @AddBeanClasses.

I also like the idea of removing @OverrideBean.

However, I would remove the @ExcludeBean annotation as it's more or less redundant and more importantly it wouldn't work for type hierarchies, for beans with qualifiers, for producers that return interface... i.e. for more complex type-safe resolution cases.

[manovotn]

I think we are nearly there, this looks like the best attempt we have had so far ;)

I've added one comment, but one more thing to note here is that you don't take into consideration CDI qualifiers. That's a problem if you have for instance several impls of Foo, each with different qualifier(s), but you need to mock only one. In your current state, you would veto() all of them.
I am not sure this is blocking for us now or if we settle on leaving that unresolved? Handling it will add a lot of complexity and I for instance cannot think of a way to make it work with class level annotation now...

[manovotn]

I really like the idea of @ExcludeBeans but I would rename the annotation to @ExcludeBeanClasses to align with @AddBeanClasses.

That's actually good point, it probably better indicates they have contrary meanings.

However, I would remove the @ExcludeBean annotation as it's more or less redundant and more importantly it wouldn't work for type hierarchies, for beans with qualifiers, for producers that return interface... i.e. for more complex type-safe resolution cases.

It is true that they are redundant, however if we ever go into implementing it with qualifiers, it is the method-level annotation that we will need as on the class-level you do not know which variants you replace.

Would be interesting to know which of the approaches feels more natural to @erilor and @kdubb as they plan to use them?

[kdubb]

Renaming to ExcludeBeanClasses seems ok. That probably means the change to checking the entire type closure that @manovotn suggested should probably not happen for that specific annotation. If it is to be inverse of AddBeanClasses it should operate on classes not type hierarchies.

@manovotn has the right idea for ExcludeBean it can be used on field and method level producers to exclude qualified beans at some point. Due to the complexity of implementation I'd really like it if we could merge with the simple feature set we have now. We can add a feature request to track and give me time to implement an extended version for a future release.

As to how we use it... currently we don't use ExcludeBeans at all; just ExcludeBean. This is because it's very natural to add the producer method for the mock/test bean you need and then just add ExcludeBean to it. If later I decide to erase the producer, I also erase the exclusion.

[erilor]

I agree with @kdubb annotating the method/member feels more natural than @ExcludeBeanClasses. You declare a mock and you want it's class excluded.

Maybe if you changed the name of the annotation to @ExcludeBeanClass it would make more clear that it excludes the class and not the actual bean? But then the name won't fit if the functionality is expanded to included hierarchies etc.

[manovotn]

Ok, for initial version, let's not deal with qualifiers.

Furthermore please incorporate these changes @kdubb :

• rename ExcludeBean -> ExcludeBeanClass
• rename ExcludeBeans -> ExcludeBeanClasses
• do vetoing based on getTypeClosure()
  • this I want because an excluded type Foo can mean a bean whose types are {Bar, Foo, Object} as well as just {Foo, Object}
  • having it will enable more use cases with interfaces and shouldn't affect your use case at all (doesn't affect tests for sure)

Then I think we are good to merge this at long last :)

[kdubb]

@manovotn I was in the processing of finishing my last commit when you commented. I had implemented everything but renaming exclude bean. I think what I have is a small bit better when forward looking.

First ExcludeBeans was renamed ExcludeBeanClasses and the veto was changed to test for the specific class (it was previously base type). This makes it a true inverse to AddBeanClasses.

ExcludeBean retains its name and its veto is now done based on the entire type closure as you suggested. This allows it to work in the broader sense but also prepares it (name as well) for when it is extended to include qualifiers. Renaming it to ExcludeBeanClass isolates it to its current functionality. I also, updated the javadoc to note the current implementation's limitations with regard to qualifiers.

Finally, I added separate tests for each annotation to test each's expressed capabilities.

[manovotn]

Created a backing issue for this #73.

I think this looks solid now. We'll include it in next release.

[erilor]

Nice work everyone and a special thanks to @kdubb for doing the "heavy lifting"!