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

WW-5408 add option to not fallback to empty namespace when unresolved #912

Merged
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
35 changes: 30 additions & 5 deletions core/src/main/java/com/opensymphony/xwork2/XWorkTestCase.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import java.util.Locale;
import java.util.Map;

import static java.util.Collections.singletonMap;

/**
* Base JUnit TestCase to extend for XWork specific JUnit tests. Uses
* the generic test setup for logic.
Expand All @@ -56,23 +58,25 @@ public XWorkTestCase() {
@Override
protected void setUp() throws Exception {
configurationManager = XWorkTestCaseHelper.setUp();
configuration = configurationManager.getConfiguration();
container = configuration.getContainer();
actionProxyFactory = container.getInstance(ActionProxyFactory.class);
reloadConfiguration(configurationManager);
}

@Override
protected void tearDown() throws Exception {
XWorkTestCaseHelper.tearDown(configurationManager);
}

protected void loadConfigurationProviders(ConfigurationProvider... providers) {
configurationManager = XWorkTestCaseHelper.loadConfigurationProviders(configurationManager, providers);
private void reloadConfiguration(ConfigurationManager configurationManager) {
configuration = configurationManager.getConfiguration();
container = configuration.getContainer();
actionProxyFactory = container.getInstance(ActionProxyFactory.class);
}

protected void loadConfigurationProviders(ConfigurationProvider... providers) {
configurationManager = XWorkTestCaseHelper.loadConfigurationProviders(configurationManager, providers);
reloadConfiguration(configurationManager);
}

protected void loadButSet(Map<String, ?> properties) {
loadConfigurationProviders(new StubConfigurationProvider() {
@Override
Expand Down Expand Up @@ -115,4 +119,25 @@ protected Map<String, Object> createContextWithLocale(Locale locale) {
.getContextMap();
}

protected void setStrutsConstant(String constant, String value) {
setStrutsConstant(singletonMap(constant, value));
}

protected void setStrutsConstant(final Map<String, String> overwritePropeties) {
configurationManager.addContainerProvider(new StubConfigurationProvider() {
@Override
public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException {
for (Map.Entry<String, String> stringStringEntry : overwritePropeties.entrySet()) {
props.setProperty(stringStringEntry.getKey(), stringStringEntry.getValue(), null);
}
}

@Override
public void destroy() {
}
});

configurationManager.reload();
reloadConfiguration(configurationManager);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
Expand Down Expand Up @@ -459,9 +460,12 @@ protected synchronized RuntimeConfiguration buildRuntimeConfiguration() throws C
boolean appendNamedParameters = Boolean.parseBoolean(
container.getInstance(String.class, StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS)
);
boolean fallbackToEmptyNamespace = Boolean.parseBoolean(
Optional.ofNullable(container.getInstance(String.class, StrutsConstants.STRUTS_ACTION_CONFIG_FALLBACK_TO_EMPTY_NAMESPACE)).orElse("true")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this fallback I would create a new entry into default.properties with value set to true and document it - this will help others understand the change

Copy link
Member

Choose a reason for hiding this comment

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

@jefferyxhy and I just discussed this one and one of the drawbacks of putting it in default.properties is that it isn't read by unit tests and causes a bunch of test failures, as the unit tests will default to false. To get around this we could additionally add the constant to StrutsDefaultConfigurationProvider as well as default.properties.

I'm personally not too fussed. In the past I've deliberately made constants default to false to try sidestep this issue. Let us know know what you would prefer

Copy link
Member

Choose a reason for hiding this comment

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

Understood, yet default.properties is used as a documentation by users so I would define the constant there with short description and keep fallback to true as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks

);

return new RuntimeConfigurationImpl(Collections.unmodifiableMap(namespaceActionConfigs),
Collections.unmodifiableMap(namespaceConfigs), matcher, appendNamedParameters);
Collections.unmodifiableMap(namespaceConfigs), matcher, appendNamedParameters, fallbackToEmptyNamespace);
}

private void setDefaultResults(Map<String, ResultConfig> results, PackageConfig packageContext) {
Expand Down Expand Up @@ -536,14 +540,17 @@ private static class RuntimeConfigurationImpl implements RuntimeConfiguration {
private final Map<String, ActionConfigMatcher> namespaceActionConfigMatchers;
private final NamespaceMatcher namespaceMatcher;
private final Map<String, String> namespaceConfigs;
private final boolean fallbackToEmptyNamespace;

public RuntimeConfigurationImpl(Map<String, Map<String, ActionConfig>> namespaceActionConfigs,
Map<String, String> namespaceConfigs,
PatternMatcher<int[]> matcher,
boolean appendNamedParameters)
boolean appendNamedParameters,
boolean fallbackToEmptyNamespace)
{
this.namespaceActionConfigs = namespaceActionConfigs;
this.namespaceConfigs = namespaceConfigs;
this.fallbackToEmptyNamespace = fallbackToEmptyNamespace;

this.namespaceActionConfigMatchers = new LinkedHashMap<>();
this.namespaceMatcher = new NamespaceMatcher(matcher, namespaceActionConfigs.keySet(), appendNamedParameters);
Expand Down Expand Up @@ -583,11 +590,10 @@ public ActionConfig getActionConfig(String namespace, String name) {
}

// fail over to empty namespace
if (config == null && StringUtils.isNotBlank(namespace)) {
if (config == null && StringUtils.isNotBlank(namespace) && ("/".equals(namespace) || fallbackToEmptyNamespace)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would extract this logic into a private method to name it, something like

if (config == null && shouldFallbackToEmptyNamespaces(namespace)) {
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

config = findActionConfigInNamespace("", name);
}


return config;
}

Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/struts2/StrutsConstants.java
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@ public final class StrutsConstants {
public static final String STRUTS_XWORKCONVERTER = "struts.xworkConverter";

public static final String STRUTS_ALWAYS_SELECT_FULL_NAMESPACE = "struts.mapper.alwaysSelectFullNamespace";
/** Fallback to empty namespace when request namespace didn't match any in action configuration */
public static final String STRUTS_ACTION_CONFIG_FALLBACK_TO_EMPTY_NAMESPACE = "struts.actionConfig.fallbackToEmptyNamespace";

/** The {@link com.opensymphony.xwork2.LocaleProviderFactory} implementation class */
public static final String STRUTS_LOCALE_PROVIDER_FACTORY = "struts.localeProviderFactory";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ public class ConstantConfig {
private Boolean freemarkerWrapperAltMap;
private BeanConfig xworkConverter;
private Boolean mapperAlwaysSelectFullNamespace;
private Boolean actionConfigFallbackToEmptyNamespace;
private BeanConfig localeProviderFactory;
private String mapperIdParameterName;
private Boolean ognlAllowStaticFieldAccess;
Expand Down Expand Up @@ -225,6 +226,7 @@ public Map<String, String> getAllAsStringsMap() {
map.put(StrutsConstants.STRUTS_FREEMARKER_WRAPPER_ALT_MAP, Objects.toString(freemarkerWrapperAltMap, null));
map.put(StrutsConstants.STRUTS_XWORKCONVERTER, beanConfToString(xworkConverter));
map.put(StrutsConstants.STRUTS_ALWAYS_SELECT_FULL_NAMESPACE, Objects.toString(mapperAlwaysSelectFullNamespace, null));
map.put(StrutsConstants.STRUTS_ACTION_CONFIG_FALLBACK_TO_EMPTY_NAMESPACE, Objects.toString(actionConfigFallbackToEmptyNamespace, null));
map.put(StrutsConstants.STRUTS_LOCALE_PROVIDER_FACTORY, beanConfToString(localeProviderFactory));
map.put(StrutsConstants.STRUTS_ID_PARAMETER_NAME, mapperIdParameterName);
map.put(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Objects.toString(ognlAllowStaticFieldAccess, null));
Expand Down Expand Up @@ -812,6 +814,14 @@ public void setMapperAlwaysSelectFullNamespace(Boolean mapperAlwaysSelectFullNam
this.mapperAlwaysSelectFullNamespace = mapperAlwaysSelectFullNamespace;
}

public Boolean getActionConfigFallbackToEmptyNamespace() {
return actionConfigFallbackToEmptyNamespace;
}

public void setActionConfigFallbackToEmptyNamespace(Boolean actionConfigFallbackToEmptyNamespace) {
this.actionConfigFallbackToEmptyNamespace = actionConfigFallbackToEmptyNamespace;
}

public BeanConfig getLocaleProviderFactory() {
return localeProviderFactory;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.opensymphony.xwork2.mock.MockInterceptor;
import com.opensymphony.xwork2.test.StubConfigurationProvider;
import com.opensymphony.xwork2.util.location.LocatableProperties;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.config.StrutsXmlConfigurationProvider;
import org.apache.struts2.dispatcher.HttpParameters;

Expand Down Expand Up @@ -239,6 +240,41 @@ public void testMultipleContainerProviders() {
mockContainerProvider.verify();
}

public void testGetActionConfigFallbackToEmptyNamespaceWhenNamespaceDontMatchAndEmptyNamespaceFallbackIsEnabled() {
// struts.actionConfig.fallbackToEmptyNamespace default to true, so it is enabled
RuntimeConfiguration configuration = configurationManager.getConfiguration().getRuntimeConfiguration();

// check namespace that doesn't match fallback to empty namespace
ActionConfig actionConfig = configuration.getActionConfig("/something/that/is/not/in/the/namespace/config", "LazyFoo");
assertEquals("default", actionConfig.getPackageName()); // fallback to empty namespace (package name is default)
assertEquals("LazyFoo", actionConfig.getName());

// check non-empty namespace and name in config still matches
assertNotNull(configuration.getActionConfig("includeTest", "Foo"));

// check root namespace and name in config still matches
actionConfig = configuration.getActionConfig("/", "LazyFoo");
assertEquals("default", actionConfig.getPackageName());
assertEquals("LazyFoo", actionConfig.getName());
}

public void testGetActionConfigReturnNullWhenNamespaceDontMatchAndEmptyNamespaceFallbackIsDisabled() {
// set the struts.actionConfig.fallbackToEmptyNamespace to false and reload the configuration
setStrutsConstant(StrutsConstants.STRUTS_ACTION_CONFIG_FALLBACK_TO_EMPTY_NAMESPACE, "false");
RuntimeConfiguration configuration = configurationManager.getConfiguration().getRuntimeConfiguration();

// check namespace that doesn't match NOT fallback to empty namespace and return null
assertNull(configuration.getActionConfig("/something/that/is/not/in/the/namespace/config", "LazyFoo"));

// check non-empty namespace and name in config still matches
assertNotNull(configuration.getActionConfig("includeTest", "Foo"));

// check root namespace and name in config still matches
ActionConfig actionConfig = configuration.getActionConfig("/", "LazyFoo");
assertEquals("default", actionConfig.getPackageName());
assertEquals("LazyFoo", actionConfig.getName());
}

public void testInitForPackageProviders() {

loadConfigurationProviders(new StubConfigurationProvider() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,23 +217,9 @@ private void setDevMode(final boolean devMode) {
/**
* Overwrite the Struts Constant and reload container
*/
private void setStrutsConstant(final Map<String, String> overwritePropeties) {
configurationManager.addContainerProvider(new StubConfigurationProvider() {
@Override
public boolean needsReload() {
return true;
}

@Override
public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException {
for (Map.Entry<String, String> stringStringEntry : overwritePropeties.entrySet()) {
props.setProperty(stringStringEntry.getKey(), stringStringEntry.getValue(), null);
}
}
});

configurationManager.reload();
container = configurationManager.getConfiguration().getContainer();
@Override
protected void setStrutsConstant(final Map<String, String> overwritePropeties) {
super.setStrutsConstant(overwritePropeties);
stack.getActionContext().withContainer(container);
}
}
}