[
https://issues.apache.org/jira/browse/WW-5284?focusedWorklogId=845383&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-845383
]
ASF GitHub Bot logged work on WW-5284:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Feb/23 12:51
Start Date: 14/Feb/23 12:51
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #659:
URL: https://github.com/apache/struts/pull/659#discussion_r1105771667
##########
core/src/main/java/com/opensymphony/xwork2/validator/DefaultActionValidatorManager.java:
##########
@@ -43,64 +60,292 @@
* @author James House
* @author Rainer Hermanns
*/
-public class DefaultActionValidatorManager extends
AbstractActionValidatorManager {
+public class DefaultActionValidatorManager implements ActionValidatorManager {
+
+ /**
+ * The file suffix for any validation file.
+ */
+ protected static final String VALIDATION_CONFIG_SUFFIX = "-validation.xml";
- private final static Logger LOG =
LogManager.getLogger(DefaultActionValidatorManager.class);
+ protected final Map<String, List<ValidatorConfig>> validatorCache =
synchronizedMap(new HashMap<>());
+ protected final Map<String, List<ValidatorConfig>> validatorFileCache =
synchronizedMap(new HashMap<>());
+ private static final Logger LOG =
LogManager.getLogger(DefaultActionValidatorManager.class);
+
+ protected ValidatorFactory validatorFactory;
+ protected ValidatorFileParser validatorFileParser;
+ protected FileManager fileManager;
+ protected boolean reloadingConfigs;
+ protected TextProviderFactory textProviderFactory;
+
+ @Inject
+ public void setValidatorFactory(ValidatorFactory fac) {
+ this.validatorFactory = fac;
+ }
+
+ @Inject
+ public void setValidatorFileParser(ValidatorFileParser parser) {
+ this.validatorFileParser = parser;
+ }
+
+ @Inject
+ public void setFileManagerFactory(FileManagerFactory fileManagerFactory) {
+ this.fileManager = fileManagerFactory.getFileManager();
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, required
= false)
+ public void setReloadingConfigs(String reloadingConfigs) {
+ this.reloadingConfigs = Boolean.parseBoolean(reloadingConfigs);
+ }
+
+ @Inject
+ public void setTextProviderFactory(TextProviderFactory
textProviderFactory) {
+ this.textProviderFactory = textProviderFactory;
+ }
@Override
- public synchronized List<Validator> getValidators(Class clazz, String
context) {
- return getValidators(clazz, context, null);
+ public void validate(Object object, String context) throws
ValidationException {
+ validate(object, context, (String) null);
+ }
+
+ @Override
+ public void validate(Object object, String context, String method) throws
ValidationException {
+ ValidatorContext validatorContext = new
DelegatingValidatorContext(object, textProviderFactory);
+ validate(object, context, validatorContext, method);
+ }
+
+ @Override
+ public void validate(Object object, String context, ValidatorContext
validatorContext) throws ValidationException {
+ validate(object, context, validatorContext, null);
+ }
+
+ /**
+ * Builds a key for validators - used when caching validators.
+ *
+ * @param clazz the action.
+ * @param context context
+ * @return a validator key which is the class name plus context.
+ */
+ protected String buildValidatorKey(Class clazz, String context) {
+ return clazz.getName() + "/" + context;
+ }
+
+ protected Validator getValidatorFromValidatorConfig(ValidatorConfig
config, ValueStack stack) {
+ Validator validator = validatorFactory.getValidator(config);
+ validator.setValidatorType(config.getType());
+ validator.setValueStack(stack);
+ return validator;
}
@Override
public synchronized List<Validator> getValidators(Class clazz, String
context, String method) {
Review Comment:
Given that both `AnnotationActionValidatorManager` and
`DefaultActionValidatorManager` use the same caching mechanism I presume both
should be synchronised on this method. Previously
`AnnotationActionValidatorManager` was not. Regardless, I don't think it will
hurt to add it here for the purpose of simplifying the code.
##########
core/src/main/java/com/opensymphony/xwork2/validator/DefaultActionValidatorManager.java:
##########
@@ -43,64 +60,292 @@
* @author James House
* @author Rainer Hermanns
*/
-public class DefaultActionValidatorManager extends
AbstractActionValidatorManager {
+public class DefaultActionValidatorManager implements ActionValidatorManager {
+
+ /**
+ * The file suffix for any validation file.
+ */
+ protected static final String VALIDATION_CONFIG_SUFFIX = "-validation.xml";
- private final static Logger LOG =
LogManager.getLogger(DefaultActionValidatorManager.class);
+ protected final Map<String, List<ValidatorConfig>> validatorCache =
synchronizedMap(new HashMap<>());
+ protected final Map<String, List<ValidatorConfig>> validatorFileCache =
synchronizedMap(new HashMap<>());
+ private static final Logger LOG =
LogManager.getLogger(DefaultActionValidatorManager.class);
+
+ protected ValidatorFactory validatorFactory;
+ protected ValidatorFileParser validatorFileParser;
+ protected FileManager fileManager;
+ protected boolean reloadingConfigs;
+ protected TextProviderFactory textProviderFactory;
+
+ @Inject
+ public void setValidatorFactory(ValidatorFactory fac) {
+ this.validatorFactory = fac;
+ }
+
+ @Inject
+ public void setValidatorFileParser(ValidatorFileParser parser) {
+ this.validatorFileParser = parser;
+ }
+
+ @Inject
+ public void setFileManagerFactory(FileManagerFactory fileManagerFactory) {
+ this.fileManager = fileManagerFactory.getFileManager();
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, required
= false)
+ public void setReloadingConfigs(String reloadingConfigs) {
+ this.reloadingConfigs = Boolean.parseBoolean(reloadingConfigs);
+ }
+
+ @Inject
+ public void setTextProviderFactory(TextProviderFactory
textProviderFactory) {
+ this.textProviderFactory = textProviderFactory;
+ }
@Override
- public synchronized List<Validator> getValidators(Class clazz, String
context) {
- return getValidators(clazz, context, null);
+ public void validate(Object object, String context) throws
ValidationException {
+ validate(object, context, (String) null);
+ }
+
+ @Override
+ public void validate(Object object, String context, String method) throws
ValidationException {
+ ValidatorContext validatorContext = new
DelegatingValidatorContext(object, textProviderFactory);
+ validate(object, context, validatorContext, method);
+ }
+
+ @Override
+ public void validate(Object object, String context, ValidatorContext
validatorContext) throws ValidationException {
+ validate(object, context, validatorContext, null);
+ }
+
+ /**
+ * Builds a key for validators - used when caching validators.
+ *
+ * @param clazz the action.
+ * @param context context
+ * @return a validator key which is the class name plus context.
+ */
+ protected String buildValidatorKey(Class clazz, String context) {
+ return clazz.getName() + "/" + context;
+ }
+
+ protected Validator getValidatorFromValidatorConfig(ValidatorConfig
config, ValueStack stack) {
+ Validator validator = validatorFactory.getValidator(config);
+ validator.setValidatorType(config.getType());
+ validator.setValueStack(stack);
+ return validator;
}
@Override
public synchronized List<Validator> getValidators(Class clazz, String
context, String method) {
String validatorKey = buildValidatorKey(clazz, context);
- if (validatorCache.containsKey(validatorKey)) {
- if (reloadingConfigs) {
- validatorCache.put(validatorKey, buildValidatorConfigs(clazz,
context, true, null));
- }
- } else {
+ if (!validatorCache.containsKey(validatorKey)) {
Review Comment:
Same logic, just reduced nesting
##########
core/src/main/java/com/opensymphony/xwork2/validator/AnnotationActionValidatorManager.java:
##########
@@ -37,56 +37,11 @@
* @author Rainer Hermanns
* @author jepjep
*/
-public class AnnotationActionValidatorManager extends
AbstractActionValidatorManager {
+public class AnnotationActionValidatorManager extends
DefaultActionValidatorManager {
Review Comment:
It's now much clearer how this class differs to the default one.
##########
core/src/test/java/com/opensymphony/xwork2/validator/DefaultActionValidatorManagerTest.java:
##########
@@ -87,7 +87,7 @@ protected void tearDown() throws Exception {
public void testBuildValidatorKey() {
- String validatorKey =
DefaultActionValidatorManager.buildValidatorKey(SimpleAction.class, alias);
+ String validatorKey =
actionValidatorManager.buildValidatorKey(SimpleAction.class, alias);
Review Comment:
Changed this to an instance method for consistency between the 2
implementations
##########
core/src/main/java/com/opensymphony/xwork2/validator/AbstractActionValidatorManager.java:
##########
@@ -1,289 +0,0 @@
-/*
- * 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 com.opensymphony.xwork2.validator;
-
-import com.opensymphony.xwork2.FileManager;
-import com.opensymphony.xwork2.FileManagerFactory;
-import com.opensymphony.xwork2.TextProviderFactory;
-import com.opensymphony.xwork2.inject.Inject;
-import com.opensymphony.xwork2.util.ClassLoaderUtil;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
-import org.apache.struts2.StrutsConstants;
-
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.URL;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
-
-import static java.util.Collections.synchronizedMap;
-
-public abstract class AbstractActionValidatorManager implements
ActionValidatorManager {
-
- /**
- * The file suffix for any validation file.
- */
- protected static final String VALIDATION_CONFIG_SUFFIX = "-validation.xml";
-
- protected final Map<String, List<ValidatorConfig>> validatorCache =
synchronizedMap(new HashMap<>());
- protected final Map<String, List<ValidatorConfig>> validatorFileCache =
synchronizedMap(new HashMap<>());
- private static final Logger LOG =
LogManager.getLogger(AbstractActionValidatorManager.class);
-
- protected ValidatorFactory validatorFactory;
- protected ValidatorFileParser validatorFileParser;
- protected FileManager fileManager;
- protected boolean reloadingConfigs;
- protected TextProviderFactory textProviderFactory;
-
- @Inject
- public void setValidatorFactory(ValidatorFactory fac) {
- this.validatorFactory = fac;
- }
-
- @Inject
- public void setValidatorFileParser(ValidatorFileParser parser) {
- this.validatorFileParser = parser;
- }
-
- @Inject
- public void setFileManagerFactory(FileManagerFactory fileManagerFactory) {
- this.fileManager = fileManagerFactory.getFileManager();
- }
-
- @Inject(value = StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, required
= false)
- public void setReloadingConfigs(String reloadingConfigs) {
- this.reloadingConfigs = Boolean.parseBoolean(reloadingConfigs);
- }
-
- @Inject
- public void setTextProviderFactory(TextProviderFactory
textProviderFactory) {
- this.textProviderFactory = textProviderFactory;
- }
-
- @Override
- public void validate(Object object, String context) throws
ValidationException {
- validate(object, context, (String) null);
- }
-
- @Override
- public void validate(Object object, String context, String method) throws
ValidationException {
- ValidatorContext validatorContext = new
DelegatingValidatorContext(object, textProviderFactory);
- validate(object, context, validatorContext, method);
- }
-
- @Override
- public void validate(Object object, String context, ValidatorContext
validatorContext) throws ValidationException {
- validate(object, context, validatorContext, null);
- }
-
- @Override
- public void validate(Object object, String context, ValidatorContext
validatorContext, String method) throws ValidationException {
- List<Validator> validators = getValidators(object.getClass(), context,
method);
- Set<String> shortcircuitedFields = null;
-
- for (Validator validator : validators) {
- try {
- validator.setValidatorContext(validatorContext);
-
- LOG.debug("Running validator: {} for object {} and method {}",
validator, object, method);
-
- FieldValidator fValidator = null;
- String fullFieldName = null;
-
- if (validator instanceof FieldValidator) {
- fValidator = (FieldValidator) validator;
- fullFieldName =
fValidator.getValidatorContext().getFullFieldName(fValidator.getFieldName());
-
- if ((shortcircuitedFields != null) &&
shortcircuitedFields.contains(fullFieldName)) {
- LOG.debug("Short-circuited, skipping");
- continue;
- }
- }
-
- if (validator instanceof ShortCircuitableValidator &&
((ShortCircuitableValidator) validator).isShortCircuit()) {
- // get number of existing errors
- List<String> errs = null;
-
- if (fValidator != null) {
- if (validatorContext.hasFieldErrors()) {
- Collection<String> fieldErrors =
validatorContext.getFieldErrors().get(fullFieldName);
-
- if (fieldErrors != null) {
- errs = new ArrayList<>(fieldErrors);
- }
- }
- } else if (validatorContext.hasActionErrors()) {
- Collection<String> actionErrors =
validatorContext.getActionErrors();
-
- if (actionErrors != null) {
- errs = new ArrayList<>(actionErrors);
- }
- }
-
- validator.validate(object);
-
- if (fValidator != null) {
- if (validatorContext.hasFieldErrors()) {
- Collection<String> errCol =
validatorContext.getFieldErrors().get(fullFieldName);
-
- if ((errCol != null) && !errCol.equals(errs)) {
- LOG.debug("Short-circuiting on field
validation");
-
- if (shortcircuitedFields == null) {
- shortcircuitedFields = new TreeSet<>();
- }
-
- shortcircuitedFields.add(fullFieldName);
- }
- }
- } else if (validatorContext.hasActionErrors()) {
- Collection<String> errCol =
validatorContext.getActionErrors();
-
- if ((errCol != null) && !errCol.equals(errs)) {
- LOG.debug("Short-circuiting");
- break;
- }
- }
-
- continue;
- }
-
- validator.validate(object);
- } finally {
- validator.setValidatorContext(null);
Review Comment:
As far as I can tell this doesn't do anything. Probably a remnant from when
we used to cache the Validators instead of the ValidatorConfigs.
##########
core/src/main/java/com/opensymphony/xwork2/validator/AnnotationActionValidatorManager.java:
##########
@@ -120,10 +66,20 @@ protected String buildValidatorKey(Class clazz, String
context) {
} else {
sb.append(context);
}
-
return sb.toString();
}
+ @Override
+ protected Validator getValidatorFromValidatorConfig(ValidatorConfig
config, ValueStack stack) {
+ Validator validator = validatorFactory.getValidator(
+ new ValidatorConfig.Builder(config)
+ .removeParam("methodName")
Review Comment:
Is this override necessary? I'm not completely clear on what it's achieving.
Would be nice to delete it if it doesn't do anything.
Issue Time Tracking
-------------------
Worklog Id: (was: 845383)
Time Spent: 20m (was: 10m)
> Further clean up ActionValidatorManager implementations
> -------------------------------------------------------
>
> Key: WW-5284
> URL: https://issues.apache.org/jira/browse/WW-5284
> Project: Struts 2
> Issue Type: Task
> Components: Core Interceptors
> Reporter: Kusal Kithul-Godage
> Priority: Trivial
> Fix For: 6.2.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
