WELD-2776 manually registered extensions in Weld SE should reside in synthetic bean archive

[dev-qnz]

I don't fully understand what exactly happens in the two new tests, so this may just as well be a question only but nevertheless it looks suspicious enough to ask.

In org.jboss.weld.util.Beans.removeDisabledBeans I figure a breakpoint on line 221 reveals that alternative beans are removed from the set of beans because the wrong beanManager's enabled is checked for enabled alternatives.

    public static <T extends Bean<?>> Set<T> removeDisabledBeans(Set<T> beans, final BeanManagerImpl beanManager) {
        if (beans.isEmpty()) {
            return beans;
        } else {
            for (Iterator<T> iterator = beans.iterator(); iterator.hasNext();) {
                if (!isBeanEnabled(iterator.next(), beanManager.getEnabled())) {
                    iterator.remove(); //                <=== breakpoint here or one line above
                }
            }
            return beans;
        }
    }

(While AfterBeanDiscoveryAddBeanTest looks more or less ok in my opinion and limited understanding as a valid test, AlternativeInBeanDeploymentArchiveTest just about reproduces the same or a similar behavior but may have any kind of shortcomings.)

Adding @Priority to @Alternative seems to work around the problem. According to my understanding of https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#bean_archive that should not be necessary.

[manovotn]

Hm, the two different BMs in AfterBeanDiscoveryAddBeanTest are because in Weld SE, there is some special handling for when you manually add bean class - a synthetic bean archive.
If you look into debug within your extension, you can see that the BM for class Foo (which was manually registered with Weld) is something like:

Weld BeanManager for org.jboss.weld.environment.deployment.WeldDeployment.synthetic [bean count=23]

Whereas the BM instance delivered to the extension method looks like this:

Weld BeanManager for org.jboss.weld.environment.deployment.WeldDeployment.additionalClasses [bean count=27]

This seems very deliberate as noted here in the codebase. I cannot say I know why is that from the top of my head though; I'll give it another look tomorrow.

Adding @Priority to @Alternative seems to work around the problem. According to my understanding of https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#bean_archive that should not be necessary.

Note that the specification doesn't place any requirements on which archive does a bean belong to if defined by an extension.
Weld seems to gather them under a separate bean archive which is why you're seeing this.
In other words, your test is enabling the alternative for the synthetic archive (WeldDeployment.synthetic) and no other.
However, the bean defined via extension actually resides in another bean archive (WeldDeployment.additionalClasses) and as such doesn't see the alternative as enabled.

To expand some more, each bean archive has its own bean manager attached and there is certain visibility established between then. This is why swapping out the BMs in your AfterBeanDiscoveryFixExtension makes a difference - with that you're effectively registering the bean under different bean archive.

FTR, the specification part you quoted is correct and so id your understanding of it but the typical use case is in something like a WAR application with additional libraries where you might want to limit visibility of your alternatives. Even then, in most cases you're fine using @Priority which is a lot easier to handle.

[manovotn]

After some more digging I am pretty convinced this is behaving as expected.
Weld is putting certain items that are not coming from a "natural" bean archive into a special archive which is what we're observing here. It's just that in SE you happen to have an extra synthetic archive as well where you can manually place some of the beans.

If you move outside of SE and into servlet/EE territory, you'd see that this split makes a lot more sense as there are all the discovered bean archives and then there's WeldDeployment.additionalClasses housing all of the items that don't belong elsewhere.

[dev-qnz]

Thanks a lot for your reply.

After having read the specs over and over again, I agree that my AfterBeanDiscoveryAddBeanTest test does not necessarily or conclusively demonstrate a specs violation of Weld. On the other hand, I could not come to a conclusion why it actually has to fail, either. I cannot resolve one piece of argument like "... happened to end up in some wrong archive" whereas I'd not have expected having to notice or deal with archives at all, ideally. Despite discussing the specs, some bean could just as well happen to end up in a suitable archive, for which I added a prototype (429622b).

[manovotn]

I'd not have expected having to notice or deal with archives at all, ideally.

Well, CDI is build around bean archives and all components come from some such archive or need to reside in one.
The behavior you are seeing is a very specific case for SE where Weld needs to decide where to place the extension as it is "manually" registered outside of other bean archive.
Not to mention you are deliberately attempting to use per-archive enablement so you definitely need to be aware of bean archives.

Despite discussing the specs, some bean could just as well happen to end up in a suitable archive, for which I added a prototype (429622b).

The test you are using (AlternativeInBeanDeploymentArchiveTest) is overriding Weld's internal deployment creation process; I am not sure I can see how are it's results relevant to this?
You can obviously end up achieving very different results with it but the default setup is consistent in how it treats archives and any extensions and synth ann. types existing outside of any known bean archives.

[dev-qnz]

I have an impression that org.jboss.weld.environment.se.Weld.addExtension(Extension)

    public Weld addExtension(Extension extension) {
        extensions.add(new MetadataImpl<Extension>(extension, SYNTHETIC_LOCATION_PREFIX + extension.getClass().getName()));
        return this;
    }

means to add the extension to the synthetic bean archive but lateron org.jboss.weld.bootstrap.ExtensionBeanDeployer.deployBean(Metadata<E>, ClassTransformer) does not pass that location prefix information to org.jboss.weld.util.DeploymentStructures.getOrCreateBeanDeployment(Deployment, BeanManagerImpl, BeanDeploymentArchiveMapping, Collection<ContextHolder<? extends Context>>, Class<?>) and then org.jboss.weld.environment.deployment.WeldDeployment.loadBeanDeploymentArchive(Class<?>) defaults to org.jboss.weld.environment.deployment.WeldDeployment.getAndUpdateAdditionalBeanDeploymentArchive(Class<?>) which evaluates to additional bean archive which is different from the archive indicated by SYNTHETIC_LOCATION_PREFIX.

Put differently, all of these

        weld.addBeanClass(Foo.class);
        weld.addBeanClass(FooAlternative.class);
        weld.addAlternative(FooAlternative.class);
        // and
        weld.addExtension(new AfterBeanDiscoveryAddFooInjectedExtension());

should preferably all go into the synthetic bean archive, including the extension, shouldn't they?

A simple quick fix (prototype 429622b) makes all tests pass including AfterBeanDiscoveryAddBeanTest.

Obviously that fix is nowhere near ready but shows it could actually work. AlternativeInBeanDeploymentArchiveTest I see now, too, was testing something different and so I removed it.

[manovotn]

Fair enough.

I cannot say what the original intention was but adding them in synthetic archive makes sense.
We'll have to do the check through all archives for all extensions even outside of SE which is unfortunate but there doesn't seem to be a better way.

I've created https://issues.redhat.com/browse/WELD-2776 to track this and I hope you won't mind that I'm going to alter this PR a bit before we use it as a fix (squashing, editing commit msg and adding a commit of my own).

Last but not least, what version of Weld are you using? Do you need this fixed for 5.1.x or should we just add it to 6.x?

[manovotn]

@dev-qnz I have force-pushed into this PR with following changes:

• Squash all your commits into one and change the commit msg to include Weld JIRA and what's been added in those commits.
• Add my own commit on top which:
  • Moved the test under se/tests and uses Arq. to align with other tests in that module; also renamed it.
  • Removed the licence header from the test which I presume was unintended (you used 2011 year RH licence)
  • Remove the TODO in your code and slightly polished it

Please take a look if you're OK with it in this state.

[dev-qnz]

i'm relieved we eventually found an agreement on a bug after all that fuzz. thanks for your patience.

i originally found it in 5.1.2. for me is fine to fix it in the master branch.

i'll have a look at your changes in a convenient moment.

[dev-qnz]

Even though it seems to work I'm not yet convinced getOrCreateBeanDeploymentForExtension is the best solution we can possibly come up with. Operating with string structures / sub-strings rings a gut feeling bell and that the location prefix corresponds to the bean deployment archive id looks more like coincidence and not as explicit as possible and desired for my taste. The current version or certainly my original version of it was only meant to demonstrate that the test might be appropriate and nothing more.

I could imagine that there is more code operating with prefixes that might somehow benefit in the same way. Or we might want to check that adding bean defining annotations, stereotypes, interceptors and decorators also work with extensions just as without.

• Can we be sure that adding Arquillian and a Shrinkwrap does not have any effect on the test itself? I'm somehow afraid, Weld could detect an archive and it could make a difference. Even though I currently don't specifically see how and see the intention to isolate the test from all the other tests, I'd prefer to doublecheck.

• I'd rather have the testAlternatives(weld -> weld.addBeanClass(FooInjected.class)); case removed and the two testAlternatives combined to one. It was useful to show that only the extension did the difference and nothing else, until we agreed on a test.

[dev-qnz]

If we have plenty time and you agree, I might try try to fix it again in a different way (or convince myself of the current solution). In that case I'd appreciate any kind of hint or advice from anyone familiar with Weld, probably you, how to deal with the synthetic archive and Metadata.location prefixes.

[manovotn]

Operating with string structures / sub-strings rings a gut feeling bell and that the location prefix corresponds to the bean deployment archive id looks more like coincidence and not as explicit as possible and desired for my taste.

The extension location prefix is only ever used for extensions added in Weld SE manually, so that's a solid way to detect it.
The location prefix and the deployment archive ID do not correspond, the are just similar. That's why I added the two constants into DeploymentStructures file.
The synthetic archive is again used only in Weld SE and there is always at most one such archive in the whole deployment. Sadly we cannot reference it directly because the DeploymentStructures cannot depend on the module containing WeldDeployment.

I could imagine that there is more code operating with prefixes that might somehow benefit in the same way. Or we might want to check that adding bean defining annotations, stereotypes, interceptors and decorators also work with extensions just as without.

I don't see what you mean here, adding the stereotypes, interceptor etc. isn't affected by this. All this PR does is change where the manually registered extension resides.

Can we be sure that adding Arquillian and a Shrinkwrap does not have any effect on the test itself? I'm somehow afraid, Weld could detect an archive and it could make a difference. Even though I currently don't specifically see how and see the intention to isolate the test from all the other tests, I'd prefer to doublecheck.

It doesn't affect the test; not this Arq. setup - I used one where it just puts together an artificial classpath for Weld but you are still in control of the CDI container bootstrap.

I'd rather have the testAlternatives(weld -> weld.addBeanClass(FooInjected.class)); case removed and the two testAlternatives combined to one. It was useful to show that only the extension did the difference and nothing else, until we agreed on a test.

I understand but I kept it in intentionally - it's a good assertion of the initial setup and clearly shows the intention and the way it should work later with the extension.
It's not like it takes too much time to test it, so I'd just keep that in place :)

If we have plenty time and you agree, I might try try to fix it again in a different way (or convince myself of the current solution).

I asked my colleague for a review but once we get that, I'd rather vote for getting this in as it doesn't seem to break anything.
We can always revisit, so if you, at a later time, propose a better/different approach, we can definitely do that.

In that case I'd appreciate any kind of hint or advice from anyone familiar with Weld, probably you, how to deal with the synthetic archive and Metadata.location prefixes.

The tricky part is that the whole bootstrap process and bean deployment creation is not necessarily aware of any synthetic archive - the same logic is used in SE, servlet and full EE. Which is why I chose the String comparison.
OTOH you can be sure that there isn't more than one synth. archive at play and always has the same ID.
As for Metadata, it's a very general interface used for other parts than just Extensions. One thing we could do is subclass it into ExtensionMetadata and add a belongsToSynthArchive() method, or something to that effect.
It would require Weld API changes and frankly might achieve the same logic by just checking the location info 🤷

[dev-qnz]

After reconsideration is all fine now for me as far as discussed so far.

Still, the following code bothers me:

    // Stored in WeldDeployment.SYNTHETIC_BDA_ID but we cannot access it directly from here
    private static final String SYNTHETIC_BDA_ID = "org.jboss.weld.environment.deployment.WeldDeployment.synthetic";

and so I tried to refactor DeploymentStructures and to separate and move synthetic archive stuff closer to Weld in order just to see what it really means, see https://github.com/dev-qnz/weld-core/compare/after-bean-discovery-add-bean...dev-qnz:weld-core:after-bean-discovery-add-bean-WeldDeploymentStructure?diff=split&w=. Or could maybe parts of it be useful or inspire to an even cleaner solution?

Considering this refactoring we might just as well continue with the current version here (f5d7b1b).

[mkouba]

Looks good.

[manovotn]

Thank you @dev-qnz for bringing this up and for the PR drafts you sent 👍