WW-4875 Add ability to use Java based configuration

[aleksandr-m]

Things to todo / improve

• Location for java based configuration is currently UNKNOWN
• ConstantConfig for plugins (convention, rest, spring, etc)
• unknownHandlerStack configuration in current implementation is a list of strings

Usage

web.xml

<filter>
    <filter-name>struts2</filter-name>
    <filter-class>org.apache.struts2.dispatcher.filter.StrutsPrepareAndExecuteFilter</filter-class>
    <init-param>
    	<param-name>javaConfigClasses</param-name>
    	<param-value>com.example.AppConf</param-value>
    </init-param>
</filter>

com.example.AppConf

public class AppConf implements StrutsJavaConfiguration {
    @Override
    public List<ConstantConfig> constants() {
        ConstantConfig c = new ConstantConfig();
        c.setDevMode(true);
        c.setObjectFactory(customObjectFactory());
        return Arrays.asList(c);
    }

    @Override
    public List<BeanConfig> beans() {
        List<BeanConfig> list = new ArrayList<>();
        list.add(new BeanConfig(SomeBean.class));
        list.add(customObjectFactory());
        return list;
    }

    @Override
    public List<String> unknownHandlerStack() {
        return null;
    }

    private BeanConfig customObjectFactory() {
        return new BeanConfig(CustomObjectFactory.class,
                              "customObjectFactory", ObjectFactory.class);
    }
}

[lukaszlenart]

This looks great! The only doubt I have is about the ConstantConfig - maybe we should turn the String constants into a Enum with additional implementation of each?

[sdutry]

This looks great! The only doubt I have is about the ConstantConfig - maybe we should turn the String constants into a Enum with additional implementation of each?

I think the enum idea for the Constants would be a great idea, but i see limitations concerning enums not being able to use generics. My point being that i think it would have value if you were able to define the constants with a type and a defaultValue.

I was thinking something allong the following lines.
This is of course just an idea and it's more than likely i overlooked several things

public final class StrutsConstants<T> {
    public static final StrutsConstants STRUTS_DEVMODE = new StrutsConstants("struts.devMode", Boolean.class, Boolean.FALSE);
    ...
    public static final StrutsConstants STRUTS_URL_HTTPS_PORT = new StrutsConstants("struts.url.https.port", Integer.class, 443);
    ...

    private final String key;
    private final Class<T> type;
    private final T defaultValue;
    
    private StrutsConstants(String key, Class<T> type, T defaultValue) {
        this.key = key;
        this.type = type;
        this.defaultValue = defaultValue;
    }

    public String getKey() {
        return key;
    }

    public Class<T> getType() {
        return type;
    }

    public T getDefaultValue() {
        return defaultValue;
    }
}

[aleksandr-m]

Just a note.

public void setObjectFactory(BeanConfig objectFactory) {
    this.objectFactory = objectFactory;
}
public void setObjectFactory(Class<?> clazz) {
    this.objectFactory = new BeanConfig(clazz, clazz.getName());
}

Currently there are two setters for BeanConfig constants in order to support different ways of setting beans.

struts.objectFactory=spring

and

struts.objectFactory=org.apache.struts2.spring.StrutsSpringObjectFactory

[sdutry]

@aleksandr-m
First off, i'd like to start with saying this is a great starting point and i absolutely love the idea.

The things below are just personal opinions which have a chanche of overlooking things and therefore being completely wrong.

ConstantsConfig

Currently i don't think the List of ConstantConfig makes much sense.
I see a couple of different ways this could be done:

• just a single ConstantsConfig
• Having a Hashmap where the key is of a type that defines the constants
  • this would require the Constants to be an instance of a type (be it an enum or a class)
• Having an @StrutsConstant annotation on a field ( RetentionPolicy.RUNTIME and ElementType.FIELD )

BeanConfig

• Would there be a way of being able to define multiple methods returning a BeanConfig and just annotating methods returning a BeanConfig element with an annotation (similar spring's @Bean annotation)?
  Something like:

    @StrutsBeanConfig
    public BeanConfig customObjectFactory() {
        return new BeanConfig(CustomObjectFactory.class, "customObjectFactory", ObjectFactory.class);
    }
    @StrutsBeanConfig
    public List<BeanConfig> beans() {
        List<BeanConfig> list = new ArrayList<>();
        list.add(new BeanConfig(SomeBean.class));
        list.add(new BeanConfig(SomeOtherBean.class));
        return list;
    }

[aleksandr-m]

First off, i'd like to start with saying this is a great starting point and i absolutely love the idea.

Great! :)

About the list of ConstantConfig:
Plugins have other constants (e.g. convention has CONVENTION_ACTION_CONFIG_BUILDER), so next will come the ConventionConstantConf and SpringConstantConf which extend ConstantConf. They should be used together hence the list.

Would there be a way of being able to define multiple methods returning a BeanConfig

I was thinking about that too. Did you see any benefit in that?

About annotations:
Same. Any advantage to use annotations in that case? Currently I kept it as simple as possible. :)

[lukaszlenart]

struts.objectFactory=org.apache.struts2.spring.StrutsSpringObjectFactory

I think this one is obsolete and should be avoided

[lukaszlenart]

StrutsConstants & @StrutsConstant

I love these ideas and we can incorporate them into the internal DI so we can inject a proper value type and not just Strings, e.g:

@Inject
public void setDevMode(String devMode) {
    this.devMode = Boolean.parse(devMode);
}

// vs

@StrutsConstant(StrutsConstants.DEV_MODE)
public void setDevMode(Boolean devMode) {
    this.devMode = devMode
}

but probably this will break backward compatibility ... anyway this PR is a good starting point and I would keep it simple and add other options in another PRs.

[sdutry]

@aleksandr-m

ConstantConfig

About the list of ConstantConfig:
Plugins have other constants (e.g. convention has CONVENTION_ACTION_CONFIG_BUILDER), so next will come the ConventionConstantConf and SpringConstantConf which extend ConstantConf. They should be used together hence the list.

The current strutsconstants are declared as what can be compared as a HashMap<String,String> inside the struts.xml. I'm still thinking if this gives much benefit to just having a HashMap of constants. I do understand you're trying to provide some typesafety to the constants, It might be overkill.

I'll keep trying to think about a possible solution that doesn't break backwards compatibility, there must be one we're overlooking.

Beanconfig

Did you see any benefit in that?

Now that you ask, there's not much benefit to it. so just having a method to override should be fine.

Annotations

About annotations:
Same. Any advantage to use annotations in that case? Currently I kept it as simple as possible. :)

The only one i'd personaly use would be to define constants, but on second thought, that might not be the best idea, given you can't realy require the annotated fields to be of a certain type.
So for the moment, i think it would be just fine without having the annotations for creating the configuration.

StrutsJavaConfiguration

I was wondering if it wouldn't be beneficial to have it as an abstract class rather than an interface.
I don't think it would make sense to extend any other class in your configuration class. Plus, you could add a default implementation returning empty lists for the configurations, not having to define the methods when not using the options.

@lukaszlenart

StrutsConstants & @strutsconstant

I love these ideas and we can incorporate them into the internal DI so we can inject a proper value type and not just Strings, e.g:
...
but probably this will break backward compatibility ... anyway this PR is a good starting point and I would keep it simple and add other options in another PRs.

I think i might have an idea about how we could use some of the benefits without breaking anything. Give me a little time to try and work it out in some detail and i'll get back to you with a proposal. (This could possible be a way to add some type-safety to the proposed Constants declarations without requiring the ConstantConfig)

[aleksandr-m]

@lukaszlenart

I love these ideas and we can incorporate them into the internal DI so we can inject a proper value type and not just Strings, e.g:

That is the ultimate goal so we can do all the conversions in one place and inject proper value types when asked. But I don't think we need separate annotation for that, @Inject will do the job.
And yes, that is a huge change because currently all constants are in the Properties - string key, string value. We can think about that after this PR is merged.

struts.objectFactory=org.apache.struts2.spring.StrutsSpringObjectFactory

I think this one is obsolete and should be avoided

Not sure. Looking from java based configuration this c.setObjectFactory(StrutsSpringObjectFactory.class); is the most natural thing to set constant with the bean defined elsewhere.

@sdutry

I do understand you're trying to provide some typesafety to the constants, It might be overkill.

It might look a bit clumsy right now, but I still think it is way better than string, string.

[sdutry]

I think i might have an idea about how we could use some of the benefits without breaking anything. Give me a little time to try and work it out in some detail and i'll get back to you with a proposal. (This could possible be a way to add some type-safety to the proposed Constants declarations without requiring the ConstantConfig)

The idea i had did not account for some of the complexities involved, so it's not executable as is.

I'm still going to put my ideas with the encountered problems here, be it just to provide a different angle to possibly spark ideas.

It involved having an interface for typed constants

public interface TypedConstant<T> {
    String getKey();
    Class<T> getType();
    T getDefaultValue();
}

Then the actual constants could be written as follows, no longer limiting things to a single Constant defining Class, but any class implementing the TypedConstant interface:

public final class StrutsTypedConstant<T> implements TypedConstant {
    public static final StrutsTypedConstant STRUTS_DEVMODE = new StrutsTypedConstant(StrutsConstants.STRUTS_DEVMODE, Boolean.class, Boolean.FALSE);
    public static final StrutsTypedConstant STRUTS_I18N_RELOAD = new StrutsTypedConstant(StrutsConstants.STRUTS_I18N_RELOAD, Boolean.class, Boolean.FALSE);
    public static final StrutsTypedConstant STRUTS_I18N_ENCODING = new StrutsTypedConstant(StrutsConstants.STRUTS_I18N_ENCODING, String.class, "UTF-8");
    public static final StrutsTypedConstant STRUTS_CONFIGURATION_XML_RELOAD = new StrutsTypedConstant(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.class, Boolean.FALSE);
    public static final StrutsTypedConstant STRUTS_ACTION_EXTENSION = new StrutsTypedConstant(StrutsConstants.STRUTS_ACTION_EXTENSION, List.class, Collections.singletonList(".action"));
    public static final StrutsTypedConstant STRUTS_ACTION_EXCLUDE_PATTERN = new StrutsTypedConstant(StrutsConstants.STRUTS_ACTION_EXCLUDE_PATTERN, List.class, null);
    public static final StrutsTypedConstant STRUTS_TAG_ALTSYNTAX = new StrutsTypedConstant(StrutsConstants.STRUTS_TAG_ALTSYNTAX, Boolean.class, false);
    public static final StrutsTypedConstant STRUTS_URL_HTTP_PORT = new StrutsTypedConstant(StrutsConstants.STRUTS_URL_HTTP_PORT, Integer.class, 80);
    public static final StrutsTypedConstant STRUTS_URL_HTTPS_PORT = new StrutsTypedConstant(StrutsConstants.STRUTS_URL_HTTPS_PORT, Integer.class, 443);
    ...

    private final String key;
    private final Class<T> type;
    private final T defaultValue;
    
    private StrutsTypedConstant(String key, Class<T> type, T defaultValue) {
        this.key = key;
        this.type = type;
        this.defaultValue = defaultValue;
    }

    public String getKey() {
        return key;
    }

    public Class<T> getType() {
        return type;
    }

    public T getDefaultValue() {
        return defaultValue;
    }
}

problems encountered:

• Types for Typed lists
• The need to have the properties as <String, String> combinations in order not to have problems
  • Possible workaround: utility class for creating strings based on the type?

public class TypedConstantUtilities {
    public static <T> Map<String,String> getConstantsAsStringMap(Map<TypedConstant<T>, T> typedConstants) {
        ...
    }
}

If this would have worked, this could have made the constants part look like:

public interface StrutsJavaConfiguration {
    Map<TypedConstant<T>, T> constants();
    ...
}

and

public class StrutsJavaConfigurationProvider implements ConfigurationProvider {
    ...
   public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException {
        ...
        Map<TypedConstant<T>, T> typedConstantsMap = javaConfig.constants();
        if (typedConstantsMap != null) {
            Map<String, String> constantMap = TypedConstantUtilities.getConstantsAsStringMap(typedConstantsMap);
            for (Entry<String, String> entr : constantMap.entrySet()) {
                registerConstant(props, entr.getKey(), entr.getValue());
            }
        }
        ...
    }
    ...
}

Which would remove the need for a list of different configurations and would not iterate over constants that weren't set.

[lukaszlenart]

What about having a custom mechanism to allow define these constants in the plugins?

[aleksandr-m]

What do you mean? ConstantConfig can be extended.

[lukaszlenart]

Sorry, I noticed just now that this is inside the plugin, nice 👍

[aleksandr-m]

And I spotted a bug. :) Yes. ConstantConfig is in the core and others in the plugins. Also they extend ConstantConfig and getAllAsStringsMap will return core constants along with the plugins constants.

[yasserzamani]

I see currently Struts has two level of inheritance for configuration providers. I though maybe we should have JavaConfigurationProvider then StrutsJavaConfigurationProvider inherits it.

[lukaszlenart]

I would rather want to reduce the number of interfaces, it can be confusing for user which one to use.

[coveralls]

Changes Unknown when pulling b2ff4bb on aleksandr-m:feature/WW-4875 into ** on apache:master**.

[aleksandr-m]

I think we can merge this and improve later. @sdutry @lukaszlenart Can you take a look at it one more time?

[lukaszlenart]

Sure but maybe we should wait till 2.6?

[aleksandr-m]

It should be backward compatible, but we can wait till 2.6. Speaking of which, what is the plan when do we start working on 2.6?

[lukaszlenart]

I hope it will be soon, just one issue left to push 2.5.15 out :)

[lukaszlenart]

LGTM 👍