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

OWB-1448 Fix Issue with Cdi annotation and alternatives #126

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,16 @@
import jakarta.enterprise.inject.spi.ObserverMethod;
import jakarta.enterprise.inject.spi.Producer;

import java.io.IOException;
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.net.URL;
import java.net.URLConnection;
import java.net.URLStreamHandler;
import java.security.PrivilegedActionException;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -1756,6 +1759,34 @@ protected void deployFromXML(ScannerService scanner)
}

logger.fine("Deploying configurations from XML has ended successfully.");

try
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder why the url is not in the scanner since it should so we do not need that hack at all in impl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping there would be a public static final URL CDI_STANDALONE = ... somewhere, in order to avoid this. org.apache.openwebbeans.se.CDISeBeanArchiveService#EMBEDDED_URL was the closest I could find, but is a String as opposed to a URL.

Maybe we could make this a constant URL in org.apache.webbeans.xml.DefaultBeanArchiveService?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm not sure I got it right but my point is that this code shouldn't be since the url should be injected in the scanner in SE mode so in owb-impl the url should already be seen (thanks addDeploymentUrl call)

what we likely do not want is impl to depend on se (like in this pr)

hope it makes more sense phrased this way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I understand :-).

Happy to debug around addDeploymentUrl, and see what I can find. What I can tell you is that if you revert out the BeansDeployer change in this PR, the tests in this PR will fail (or at least they do for me). I'm very happy to look at a different fix (I might have questions.... :-) ), but I do think a fix is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think we all agree the API should be respected and the regression be fixed if any (maybe java related, didnt check if latest jre changed url handling), but think we should stick to the original design of se on top of impl and not a cycle dep and url 100% taken from the scanner, even virtual ones which were just a hack to make it work without a new spi. This part can change if it help - but think we can make it work, there should be something fishy making the fake url dropped somewhere in the process.
can try to help to debug next week if there is no new finding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addDeploymentUrl is called from org.apache.openwebbeans.se.CDISeScannerService#addClassesDeploymentUrl with the embedded URL (great), however, the scanner.getBeanXmls() call from org.apache.webbeans.config.BeansDeployer#deployFromXML does not return that URL, as addClassesDeploymentUrl adds the URL to org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanDeploymentUrls and scanner.getBeanXmls() gets the URLs from org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations.

Could we just include the embedded URL in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations?

Copy link
Contributor Author

@jgallimore jgallimore Feb 21, 2025

Choose a reason for hiding this comment

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

Doing this:

    protected void addDeploymentUrl(String beansXml, URL cpUrl)
    {
        beanDeploymentUrls.put(beansXml, cpUrl);
        beanArchiveLocations.add(cpUrl);
    }

in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#addDeploymentUrl

works. Would that change be ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

guess it will need to call doAddWebBeansXmlLocation but looks like the additional call to do

that said doing it in addDeploymentUrl will change the lifecycle for the nominal case so i'd just align on the nominal case so adjust CDISeScannerService (a bit like web scanner has web-inb url handling specifically)

hope it makes sense

{
final URL url = new URL("openwebbeans", null, 0, "cdi-standalone", new URLStreamHandler()
{
@Override
protected URLConnection openConnection(URL u) throws IOException
{
return null;
}
});

final BeanArchiveInformation beanArchiveInformation = beanArchiveService.getBeanArchiveInformation(url);
if (beanArchiveInformation != null)
{
configureDecorators(url, beanArchiveInformation.getDecorators());
configureInterceptors(url, beanArchiveInformation.getInterceptors());
configureAlternatives(url, beanArchiveInformation.getAlternativeClasses(), false);
configureAlternatives(url, beanArchiveInformation.getAlternativeStereotypes(), true);
configureAllowProxying(url, beanArchiveInformation.getAllowProxyingClasses());
}

logger.fine("Deploying embedded configurations has ended successfully.");
}
catch (Exception e)
{
logger.info("Error occurred: " + e.getMessage());
}
}

private void configureAlternatives(URL bdaLocation, List<String> alternatives, boolean isStereotype)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.openwebbeans.junit5.features;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.inject.Alternative;
import jakarta.enterprise.inject.Default;
import jakarta.inject.Inject;
import org.apache.openwebbeans.junit5.Cdi;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

@Cdi(disableDiscovery = true, classes = {
AlternativeTest.Service.class, AlternativeTest.Provider.class, AlternativeTest.DefaultProvider.class, AlternativeTest.AlternativeProvider.class
}, alternatives = AlternativeTest.AlternativeProvider.class)
public class AlternativeTest
{
@Inject
private Service service;

@Test
void test1()
{
assertEquals("alternative", service.run());
}

public interface Provider
{
String provide();
}

@Alternative
public static class AlternativeProvider implements Provider
{
@Override
public String provide()
{
return "alternative";
}
}

@Default
public static class DefaultProvider implements Provider
{
@Override
public String provide()
{
return "default";
}
}

@ApplicationScoped
public static class Service
{

@Inject
private Provider provider;

public String run()
{
return provider.provide();
}

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.openwebbeans.junit5.features;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.context.NormalScope;
import jakarta.enterprise.inject.Alternative;
import jakarta.enterprise.inject.Default;
import jakarta.inject.Inject;
import jakarta.interceptor.AroundInvoke;
import jakarta.interceptor.Interceptor;
import jakarta.interceptor.InterceptorBinding;
import jakarta.interceptor.InvocationContext;
import org.apache.openwebbeans.junit5.Cdi;
import org.junit.jupiter.api.Test;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import static org.junit.jupiter.api.Assertions.assertEquals;

@Cdi(disableDiscovery = true, classes = {
InterceptorTest.Service.class, InterceptorTest.MyInterceptor.class, InterceptorTest.Wrap.class
}, interceptors = InterceptorTest.MyInterceptor.class)
public class InterceptorTest
{
@Inject
private Service service;

@Test
void test1()
{
assertEquals("Intercepted Hello World", service.run());
}

@Target({ElementType.TYPE, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@InterceptorBinding
public @interface Wrap {

}

@Interceptor
@Wrap
public static class MyInterceptor {
@AroundInvoke
public Object restrictAccessBasedOnTime(InvocationContext ctx) throws Exception {
final Object result = ctx.proceed();
if (result instanceof String) {
return "Intercepted " + result;
} else {
return result;
}
}
}

@ApplicationScoped
public static class Service
{
@Wrap
public String run()
{
return "Hello World";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;

@Cdi(classes = MyService.class)
@Cdi(classes = MyService.class, disableDiscovery = true)
class ParameterResolutionTest
{
@Inject MyService service;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public Property[] properties()
@Override
public boolean disableDiscovery()
{
return false;
return true;
}

@Override
Expand Down