[XBEAN-453] try to explicit the parameter types matched instead of us…

[rmannibucau]

…ing ...

[rzo1]

I have tested the change with the other change from @jungm in TomEE and encountered some additional failing tests due to the changes for finding a (static) factory method.

org.apache.xbean.recipe.MissingFactoryMethodException: Instance factory method is static: public static org.apache.geronimo.transaction.manager.GeronimoTransactionManager org.apache.openejb.resource.GeronimoTransactionManagerFactory.create(java.lang.Integer,org.apache.openejb.util.Duration,boolean,byte[],java.lang.String,int,boolean,boolean,java.lang.Integer,org.apache.openejb.util.Duration,java.lang.String,java.lang.String,java.lang.String,int,int,int,int,int) throws java.lang.Exception

	at org.apache.xbean.recipe.ReflectionUtil.findInstanceFactory(ReflectionUtil.java:879)
	at org.apache.xbean.recipe.ObjectRecipe.internalCreate(ObjectRecipe.java:305)
	at org.apache.xbean.recipe.AbstractRecipe.create(AbstractRecipe.java:96)
	at org.apache.xbean.recipe.AbstractRecipe.create(AbstractRecipe.java:61)
	at org.apache.xbean.recipe.AbstractRecipe.create(AbstractRecipe.java:49)
	at org.apache.openejb.assembler.classic.Assembler.createTransactionManager(Assembler.java:3587)
	at org.apache.openejb.config.XmlOverridesTest.test(XmlOverridesTest.java:69)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

Test to reproduce would be

package org.apache.xbean.recipe;

import org.junit.Test;

import java.time.Duration;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;

import static org.junit.Assert.assertNotNull;

public class XBean353FactoryTest {

    @Test
    public void shouldFindStaticFactoryForStaticFactory() {
        // given
        Class<?> typeClass = InterfaceTypeFactory.class;

        String factoryMethod = "create";

        List<String> parameterNames = Arrays.asList(
                "defaultTransactionTimeoutSeconds",
                "defaultTransactionTimeout",
                "TxRecovery",
                "tmId",
                "bufferClassName",
                "bufferSizeKb",
                "checksumEnabled",
                "adler32Checksum",
                "flushSleepTimeMilliseconds",
                "flushSleepTime",
                "logFileDir",
                "logFileExt",
                "logFileName",
                "maxBlocksPerFile",
                "maxBuffers",
                "maxLogFiles",
                "minBuffers",
                "threadsWaitingForceThreshold"
        );

        // Explicitly null
        List<Class<?>> parameterTypes = null;

        Set<String> allProperties = new LinkedHashSet<>(Arrays.asList(
                "defaultTransactionTimeout",
                "TxRecovery",
                "bufferSizeKb",
                "checksumEnabled",
                "adler32Checksum",
                "flushSleepTime",
                "logFileDir",
                "logFileExt",
                "logFileName",
                "maxBlocksPerFile",
                "maxBuffers",
                "maxLogFiles",
                "minBuffers",
                "threadsWaitingForceThreshold"
        ));

        EnumSet<Option> options = EnumSet.of(
                Option.PRIVATE_PROPERTIES,
                Option.FIELD_INJECTION,
                Option.IGNORE_MISSING_PROPERTIES,
                Option.CASE_INSENSITIVE_PROPERTIES
        );

        // when
        ReflectionUtil.StaticFactory staticFactory = ReflectionUtil.findStaticFactory(
                typeClass,
                factoryMethod,
                parameterNames,
                parameterTypes,
                allProperties,
                options
        );

        // then
        assertNotNull(staticFactory);
    }

    public interface InterfaceType {
    }

    public static class InterfaceTypeFactory {
        public static InterfaceType create(Integer defaultTransactionTimeoutSeconds,
                                           final Duration defaultTransactionTimeout,
                                           final boolean txRecovery,
                                           final byte[] tmId,
                                           final String bufferClassName,
                                           final int bufferSizeKb,
                                           final boolean checksumEnabled,
                                           final boolean adler32Checksum,
                                           Integer flushSleepTimeMilliseconds, // Deprecated, use flushSleepTime
                                           final Duration flushSleepTime,
                                           final String logFileDir,
                                           final String logFileExt,
                                           final String logFileName,
                                           final int maxBlocksPerFile,
                                           final int maxBuffers,
                                           final int maxLogFiles,
                                           final int minBuffers,
                                           final int threadsWaitingForceThreshold) throws Exception {
            return null;
        }
    }
}

I think the issue is that we (again) run into implicit selection and since we have

   parameterNames = getParameterNames(method);
                if (parameterNames == null || !allProperties.containsAll(parameterNames)) {
                    continue;
                }
                

we are ignoring case insensitive properties (and missing properties) and run into the issue with not finding the correct (static) factory.

[rzo1]

I’ve added some tests for the TomEE cases I encountered while testing the change. Additionally, the change from this PR is required for all new test cases to pass.

Here’s what I observed:

With the initial change

boolean specifiedParameterTypes = parameterTypes != null;

which addresses a similar issue to XBEAN-350, we now encounter the same buggy behavior in findStaticFactory(...). The problem is that we cannot rely solely on the properties and ctor argument names, because sometimes these properties can be ignored and sometimes they cannot, depending on whether the option is specified or not.

To handle this, I added a heuristic to select the "best" matching factory method when multiple factories are available (and the ignore missing properties option is specified). I’m not sure if this is the ideal approach, but when no parameter types are provided, we can only make an educated guess about the user’s intent.

With these changes, combined with the updates from @jungm in #47, all TomEE tests pass, as well as all existing XBean tests. If we can live with that approach, it might be an idea to also add such heuristic to findConstructor(....) if the option ignore missing properties is specified.

Ofc open for discussion / feedback!

[rmannibucau]

@rzo1 while there is no regression all good for me

side note: tomee can need to do its homeworks and fix all factories/constructors used in its service-jar.xml (for user ones we keep current behavior and just document it better?).

[rzo1]

side note: tomee can need to do its homeworks and fix all factories/constructors used in its service-jar.xml (for user ones we keep current behavior and just document it better?).

Yes, I’ve already started fixing constructor argument names and adding explicit parameter types where necessary or possible. I’ve deployed a snapshot of this xbean branch and am running a full build to see whether this exposes any additional regressions in TomEE.

From a documentation perspective, I agree that we should be clearer that users are strongly encouraged to supply parameter types to avoid implicit matching when it’s not required. For that reason, I think we should at least update the JavaDoc to explicitly list the supported options and clearly explain implicit versus explicit selection (also for the other methods / cases in this cass). I can follow up with a separate PR for the Javadoc changes.