JCgH4164838Gh792C124B5 commented on a change in pull request #442: URL: https://github.com/apache/struts/pull/442#discussion_r571489536
########## File path: core/src/main/java/org/apache/struts2/views/TagAttribute.java ########## @@ -0,0 +1,79 @@ +/* + * 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.struts2.views; + +public class TagAttribute { + + public static final TagAttribute NULL = new TagAttribute(null, true); + public static final TagAttribute EMPTY = new TagAttribute("", true); + + private final String value; + private final boolean evaluated; + + private TagAttribute(String value, boolean evaluated) { + this.value = value; + this.evaluated = evaluated; + } + + public static TagAttribute raw(String value) { + return new TagAttribute(value, false); + } + + public static TagAttribute evaluated(String evaluatedValue) { + return new TagAttribute(evaluatedValue, true); + } + + public boolean isExpression() { + return value != null && value.contains("%{") && value.contains("}"); + } + + public String stripedExpression() { + if (isExpression()) { + return value.substring(2, value.length() - 1); + } else { + return value; + } + } + + public TagAttribute escaped(){ + // escape any possible values that can make the ID painful to work with in JavaScript + if (value != null) { + return TagAttribute.evaluated(value.replaceAll("[\\/\\.\\[\\]\'\"]", "_")); + } else { + return null; + } + } + + public String getValue() { + return value; + } + + public boolean isEvaluated() { + return evaluated; + } + + public boolean isNull() { + return value == null; + } + + public TagAttribute append(String appendString) { + return TagAttribute.evaluated(value + appendString); + } + Review comment: After trying out a copy of this PR's branch locally, it seems that introducing the `TagAttribute` creates some issues where existing code expects to be dealing with String for an attribute. This causes a failure in the Showcase Webapp UITagExampleTest which can also be seen interactively when the WAR is deployed. Problems may arise with various Component modules, where code uses calls to `Component.getParameters()`. Some modules that might be impacted are: DoubleListUIBean, File, Form, FormButton, InputTransferSelect, OptionTransferSelect, Token, UIBean, UpDownSelect. There might be others that I missed, as well. One possible mitigation strategy could be to add some additional methods to `TagAttribute` in order to handle the possibility that a parameter's type could be either `TagAttribute` or `String`. Here is a possible implementation: ``` /** * Override the toString() method so that the {@link TagAttribute} behaves more like a * {@link String} for processing expecting a String value such as String concatenation. * * @return */ @Override public String toString() { return getValue(); } /** * Convenience method to process attributes/parameters that may be {@link TagAttribute} or {@link String}. * * With mixed processing, one will not know when a given ID (key) might map to a plain String or a TagAttribute. * This method can be used to process objects that may be a mix of Strings and TagAttributes, as if they were both * Strings. * * @param objectToConvert A TagAttribute or String parameter from which to retrieve its String value (may be null). * @return The String value of a TagAttribute, or the String itself if already a String. * @throws IllegalArgumentException if the parameter is not a TagAttribute, String or null. */ public static String tagAttributeValueOrStringAsString(Object objectToConvert) { if (objectToConvert instanceof String) { return ((String) objectToConvert); } else if (objectToConvert instanceof TagAttribute) { return (((TagAttribute) objectToConvert).getValue()); } else if (objectToConvert == null) { return null; } else { throw new IllegalArgumentException("Only TagAttribute or String parameter supported. Cannot convert: " + objectToConvert.getClass().getName()); } } /** * Convenience method to check if an object is a {@link TagAttribute} or not. * * @param objectToCheck The Object to check against. * @return true if parameter is a TagAttribute, false otherwise. */ public static boolean isTagAttribute(Object objectToCheck) { return (objectToCheck instanceof TagAttribute); } ``` The overridden `toString()` method should help in places where a `TagAttribute` is being included in a `String` concatenation or similar operation (something natural when the tag attribute was a `String`). The `tagAttributeValueOrStringAsString()` method should help in places where a retrieval of a parameter might be a `TagAttribute` or `String`, and a guard in case intermediate processing replaced one type of parameter with the other. The `toString()` should be a transparent fix/guard for some situations, while the other two methods could be applied in situations where assuming a cast to a `String` or `TagAttribute` could cause an exception if the value is of the other type. ########## File path: core/src/main/java/org/apache/struts2/views/TagAttribute.java ########## @@ -0,0 +1,79 @@ +/* + * 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.struts2.views; + +public class TagAttribute { + + public static final TagAttribute NULL = new TagAttribute(null, true); + public static final TagAttribute EMPTY = new TagAttribute("", true); + + private final String value; + private final boolean evaluated; + + private TagAttribute(String value, boolean evaluated) { + this.value = value; + this.evaluated = evaluated; + } + + public static TagAttribute raw(String value) { + return new TagAttribute(value, false); + } + + public static TagAttribute evaluated(String evaluatedValue) { + return new TagAttribute(evaluatedValue, true); + } + + public boolean isExpression() { + return value != null && value.contains("%{") && value.contains("}"); + } + + public String stripedExpression() { + if (isExpression()) { + return value.substring(2, value.length() - 1); + } else { + return value; + } + } + + public TagAttribute escaped(){ + // escape any possible values that can make the ID painful to work with in JavaScript + if (value != null) { + return TagAttribute.evaluated(value.replaceAll("[\\/\\.\\[\\]\'\"]", "_")); + } else { + return null; + } + } + + public String getValue() { + return value; + } + + public boolean isEvaluated() { + return evaluated; + } + + public boolean isNull() { + return value == null; + } + + public TagAttribute append(String appendString) { + return TagAttribute.evaluated(value + appendString); + } + +} Review comment: Here are some locations where the introduction of the `TagAttribute `instead of a `String `may need to be reviewed. If something like the `tagAttributeValueOrStringAsString()` and `toString()` changes suggested above were applied, changes to the code at these lines could be considered: https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/Form.java#L173-L176 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/FormButton.java#L109 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/InputTransferSelect.java#L185-L186 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/OptionTransferSelect.java#L303-L304 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/OptionTransferSelect.java#L310-L311 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/UIBean.java#L1002 https://github.com/apache/struts/blob/5077c211187554b4e001f71a3b1c0d99bd4a2183/core/src/main/java/org/apache/struts2/components/UpDownSelect.java#L144 ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
