diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java index c8f5ab6d4d..ea2988ec3f 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java @@ -4,17 +4,13 @@ package net.sourceforge.pmd; -import static net.sourceforge.pmd.properties.PropertyDescriptorField.DEFAULT_VALUE; - import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.Properties; import java.util.Set; import java.util.logging.Level; @@ -26,10 +22,8 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import org.apache.commons.lang3.StringUtils; -import org.w3c.dom.Attr; import org.w3c.dom.Document; import org.w3c.dom.Element; -import org.w3c.dom.NamedNodeMap; import org.w3c.dom.Node; import org.w3c.dom.NodeList; import org.xml.sax.InputSource; @@ -41,9 +35,6 @@ import net.sourceforge.pmd.lang.LanguageRegistry; import net.sourceforge.pmd.lang.rule.MockRule; import net.sourceforge.pmd.lang.rule.RuleReference; import net.sourceforge.pmd.lang.rule.XPathRule; -import net.sourceforge.pmd.properties.PropertyDescriptor; -import net.sourceforge.pmd.properties.PropertyDescriptorField; -import net.sourceforge.pmd.properties.PropertyDescriptorUtil; import net.sourceforge.pmd.ruleset.RuleFactory; import net.sourceforge.pmd.util.ResourceLoader; @@ -61,9 +52,6 @@ public class RuleSetFactory { private static final String DESCRIPTION = "description"; private static final String UNEXPECTED_ELEMENT = "Unexpected element <"; private static final String PRIORITY = "priority"; - private static final String FOR_RULE = "' for Rule "; - private static final String MESSAGE = "message"; - private static final String EXTERNAL_INFO_URL = "externalInfoUrl"; private final ResourceLoader resourceLoader; private final RulePriority minimumPriority; @@ -538,7 +526,7 @@ public class RuleSetFactory { && !isRuleName(ruleElement, ruleSetReferenceId.getRuleName())) { return; } -<< Rule rule = RuleFactory.INSTANCE.buildRule(ruleElement); + Rule rule = RuleFactory.INSTANCE.buildRule(ruleElement); rule.setRuleSetName(ruleSetBuilder.getName()); if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName()) @@ -547,9 +535,6 @@ public class RuleSetFactory { } } - private static boolean hasAttributeSetTrue(Element element, String attributeId) { - return element.hasAttribute(attributeId) && "true".equalsIgnoreCase(element.getAttribute(attributeId)); - } /** * Parse a rule node as a RuleReference. A RuleReference is a single Rule @@ -622,39 +607,9 @@ public class RuleSetFactory { RuleSetReference ruleSetReference = new RuleSetReference(otherRuleSetReferenceId.getRuleSetFileName(), false); - RuleReference ruleReference = new RuleReference(); + RuleReference ruleReference = RuleFactory.INSTANCE.decorateRule(referencedRule, ruleElement); ruleReference.setRuleSetReference(ruleSetReference); - ruleReference.setRule(referencedRule); - if (ruleElement.hasAttribute("deprecated")) { - ruleReference.setDeprecated(Boolean.parseBoolean(ruleElement.getAttribute("deprecated"))); - } - if (ruleElement.hasAttribute("name")) { - ruleReference.setName(ruleElement.getAttribute("name")); - } - if (ruleElement.hasAttribute(MESSAGE)) { - ruleReference.setMessage(ruleElement.getAttribute(MESSAGE)); - } - if (ruleElement.hasAttribute(EXTERNAL_INFO_URL)) { - ruleReference.setExternalInfoUrl(ruleElement.getAttribute(EXTERNAL_INFO_URL)); - } - for (int i = 0; i < ruleElement.getChildNodes().getLength(); i++) { - Node node = ruleElement.getChildNodes().item(i); - if (node.getNodeType() == Node.ELEMENT_NODE) { - if (node.getNodeName().equals(DESCRIPTION)) { - ruleReference.setDescription(parseTextNode(node)); - } else if (node.getNodeName().equals("example")) { - ruleReference.addExample(parseTextNode(node)); - } else if (node.getNodeName().equals(PRIORITY)) { - ruleReference.setPriority(RulePriority.valueOf(Integer.parseInt(parseTextNode(node)))); - } else if (node.getNodeName().equals("properties")) { - parsePropertiesNode(ruleReference, node); - } else { - throw new IllegalArgumentException(UNEXPECTED_ELEMENT + node.getNodeName() - + "> encountered as child of element for Rule " + ruleReference.getName()); - } - } - } if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName()) || referencedRule.getPriority().compareTo(minimumPriority) <= 0) { @@ -699,110 +654,6 @@ public class RuleSetFactory { return node.getNodeType() == Node.ELEMENT_NODE && node.getNodeName().equals(name); } - /** - * Parse a properties node. - * - * @param rule - * The Rule to which the properties should be added. - * @param propertiesNode - * Must be a properties element node. - */ - private static void parsePropertiesNode(Rule rule, Node propertiesNode) { - for (int i = 0; i < propertiesNode.getChildNodes().getLength(); i++) { - Node node = propertiesNode.getChildNodes().item(i); - if (isElementNode(node, "property")) { - parsePropertyNodeBR(rule, node); - } - } - } - - private static String valueFrom(Node parentNode) { - - final NodeList nodeList = parentNode.getChildNodes(); - - for (int i = 0; i < nodeList.getLength(); i++) { - Node node = nodeList.item(i); - if (isElementNode(node, "value")) { - return parseTextNode(node); - } - } - return null; - } - - - /** - * Sets the value of a property. - * - * @param rule The rule which has the property - * @param desc The property descriptor - * @param strValue The string value of the property, converted to a T - * @param The type of values of the property descriptor - */ - private static void setValue(Rule rule, PropertyDescriptor desc, String strValue) { - T realValue = desc.valueFrom(strValue); - rule.setProperty(desc, realValue); - } - - - /** - * Parse a property node. - * - * @param rule The Rule to which the property should be added. - * @param propertyNode Must be a property element node. - */ - private static void parsePropertyNodeBR(Rule rule, Node propertyNode) { - - Element propertyElement = (Element) propertyNode; - String typeId = propertyElement.getAttribute(PropertyDescriptorField.TYPE.attributeName()); - String strValue = propertyElement.getAttribute(DEFAULT_VALUE.attributeName()); - if (StringUtils.isBlank(strValue)) { - strValue = valueFrom(propertyElement); - } - - // Setting of existing property, or defining a new property? - if (StringUtils.isBlank(typeId)) { - String name = propertyElement.getAttribute(PropertyDescriptorField.NAME.attributeName()); - - PropertyDescriptor propertyDescriptor = rule.getPropertyDescriptor(name); - if (propertyDescriptor == null) { - throw new IllegalArgumentException( - "Cannot set non-existant property '" + name + "' on Rule " + rule.getName()); - } else { - setValue(rule, propertyDescriptor, strValue); - } - return; - } - - PropertyDescriptorExternalBuilder pdFactory = PropertyDescriptorUtil.factoryFor(typeId); - if (pdFactory == null) { - throw new RuntimeException("No property descriptor factory for type: " + typeId); - } - - Map values = new HashMap<>(); - NamedNodeMap atts = propertyElement.getAttributes(); - - // populate a map of values for an individual descriptor - for (int i = 0; i < atts.getLength(); i++) { - Attr a = (Attr) atts.item(i); - values.put(PropertyDescriptorField.getConstant(a.getName()), a.getValue()); - } - - if (StringUtils.isBlank(values.get(DEFAULT_VALUE))) { - NodeList children = propertyElement.getElementsByTagName(DEFAULT_VALUE.attributeName()); - if (children.getLength() == 1) { - values.put(DEFAULT_VALUE, children.item(0).getTextContent()); - } else { - throw new RuntimeException("No value defined!"); - } - } - - // casting is not pretty but prevents the interface from having this method - PropertyDescriptor desc = pdFactory.build(values); - - rule.definePropertyDescriptor(desc); - setValue(rule, desc, strValue); - } - /** * Parse a String from a textually type node. * diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/ruleset/RuleFactory.java b/pmd-core/src/main/java/net/sourceforge/pmd/ruleset/RuleFactory.java index a0b65ea5c0..cf974a01e4 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/ruleset/RuleFactory.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/ruleset/RuleFactory.java @@ -18,6 +18,8 @@ import org.w3c.dom.Node; import org.w3c.dom.NodeList; import net.sourceforge.pmd.Rule; +import net.sourceforge.pmd.RulePriority; +import net.sourceforge.pmd.lang.rule.RuleReference; import net.sourceforge.pmd.properties.AbstractPropertyDescriptorFactory; import net.sourceforge.pmd.properties.PropertyDescriptor; import net.sourceforge.pmd.properties.PropertyDescriptorFactory; @@ -40,6 +42,63 @@ public class RuleFactory { } + /** + * Decorates a referenced rule with the values that are overriden in the given rule element. + * + * @param referencedRule Referenced rule + * @param ruleElement Element overriding some metadata about the rule + * + * @return A rule reference to the referenced rule + */ + public RuleReference decorateRule(Rule referencedRule, Element ruleElement) { + RuleReference ruleReference = new RuleReference(); + ruleReference.setRule(referencedRule); + + + if (ruleElement.hasAttribute("deprecated")) { + ruleReference.setDeprecated(Boolean.parseBoolean(ruleElement.getAttribute("deprecated"))); + } + if (ruleElement.hasAttribute("name")) { + ruleReference.setName(ruleElement.getAttribute("name")); + } + if (ruleElement.hasAttribute("message")) { + ruleReference.setMessage(ruleElement.getAttribute("message")); + } + if (ruleElement.hasAttribute("externalInfoUrl")) { + ruleReference.setExternalInfoUrl(ruleElement.getAttribute("externalInfoUrl")); + } + + + for (int i = 0; i < ruleElement.getChildNodes().getLength(); i++) { + Node node = ruleElement.getChildNodes().item(i); + if (node.getNodeType() == Node.ELEMENT_NODE) { + switch (node.getNodeName()) { + case "description": + ruleReference.setDescription(parseTextNode(node)); + break; + case "example": + ruleReference.addExample(parseTextNode(node)); + break; + case "priority": + ruleReference.setPriority(RulePriority.valueOf(Integer.parseInt(parseTextNode(node)))); + break; + case "properties": + setPropertyValues(ruleReference, (Element) node); + break; + default: + throw new IllegalArgumentException("Unexpected element <" + node.getNodeName() + + "> encountered as child of element for Rule " + + ruleReference.getName()); + + } + } + } + + + return ruleReference; + } + + /** * Parses a rule element and returns a new rule instance. * @@ -123,7 +182,7 @@ public class RuleFactory { if (propertiesElement != null) { - overrideProperties(rule, propertiesElement); + setPropertyValues(rule, propertiesElement); } return rule; @@ -131,8 +190,7 @@ public class RuleFactory { private void checkRequiredAttributesArePresent(Element ruleElement) { - final List required = Arrays.asList("name", - "class"); + final List required = Arrays.asList("name", "class"); for (String att : required) { if (!ruleElement.hasAttribute(att)) { @@ -143,20 +201,19 @@ public class RuleFactory { /** - * Parses a properties element looking only for property value overrides. + * Parses a properties element looking only for the values of the properties defined or overridden. * * @param propertiesNode Node to parse * - * @return The map of overridden properties names to their value + * @return A map of property names to their value */ - private Map parsePropertiesForOverrides(Element propertiesNode) { + private Map getPropertyValuesFrom(Element propertiesNode) { Map overridenProperties = new HashMap<>(); for (int i = 0; i < propertiesNode.getChildNodes().getLength(); i++) { Node node = propertiesNode.getChildNodes().item(i); - if (node.getNodeType() == Node.ELEMENT_NODE && node.getNodeName().equals("property") - && !isPropertyDefinition((Element) node)) { - Entry overridden = parsePropertyOverride((Element) node); + if (node.getNodeType() == Node.ELEMENT_NODE && node.getNodeName().equals("property")) { + Entry overridden = getPropertyValue((Element) node); overridenProperties.put(overridden.getKey(), overridden.getValue()); } } @@ -185,7 +242,7 @@ public class RuleFactory { } - private Entry parsePropertyOverride(Element propertyElement) { + private Entry getPropertyValue(Element propertyElement) { String name = propertyElement.getAttribute(PropertyDescriptorField.NAME.attributeName()); return new SimpleEntry<>(name, valueFrom(propertyElement)); } @@ -197,8 +254,8 @@ public class RuleFactory { * @param rule The rule * @param propertiesElt The {@literal } element */ - private void overrideProperties(Rule rule, Element propertiesElt) { - Map overridden = parsePropertiesForOverrides(propertiesElt); + private void setPropertyValues(Rule rule, Element propertiesElt) { + Map overridden = getPropertyValuesFrom(propertiesElt); for (Entry e : overridden.entrySet()) { PropertyDescriptor descriptor = rule.getPropertyDescriptor(e.getKey()); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java index 5278b74f9e..bc86adb912 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java @@ -267,9 +267,6 @@ public class RuleSetFactoryTest { PropertyDescriptor test3Descriptor = r.getPropertyDescriptor("test3"); assertNotNull("test3 descriptor", test3Descriptor); assertEquals("override3", r.getProperty(test3Descriptor)); - PropertyDescriptor test4Descriptor = r.getPropertyDescriptor("test4"); - assertNotNull("test3 descriptor", test4Descriptor); - assertEquals("new property", r.getProperty(test4Descriptor)); } @Test @@ -688,14 +685,14 @@ public class RuleSetFactoryTest { private static final String REF_OVERRIDE = "" + PMD.EOL + "" + PMD.EOL + " testdesc" + PMD.EOL + " " + PMD.EOL + " Test description override" + PMD.EOL + " Test example override" + PMD.EOL + " 3" + PMD.EOL + " " + PMD.EOL - + " " + PMD.EOL - + " override3" - + PMD.EOL + " " + + " " + PMD.EOL + + " override3" + // + PMD.EOL + " " // Nonsense + PMD.EOL + " " + PMD.EOL + " " + PMD.EOL + ""; private static final String REF_INTERNAL_TO_INTERNAL = "" + PMD.EOL diff --git a/pmd-core/src/test/resources/net/sourceforge/pmd/TestRuleset1.xml b/pmd-core/src/test/resources/net/sourceforge/pmd/TestRuleset1.xml index a3925767ef..9dcb040e86 100644 --- a/pmd-core/src/test/resources/net/sourceforge/pmd/TestRuleset1.xml +++ b/pmd-core/src/test/resources/net/sourceforge/pmd/TestRuleset1.xml @@ -32,6 +32,24 @@ Just for test + + + Just for test + + 3 + + + + + + + + + + \ No newline at end of file