[custom] New check on bundle feature.xml file (#395)

* Changed missing feature warn to debug

With the features moved to the addons it will not find the feature.xml and report this as a warn on each binding, while it should not give this warning as it ok.

* [custom] Add check for feature.xml

The following checks are added:
1) If there is a feature.xml at all.
2) If the name in <features tag is as expected.
3) If the name in <feature tag is as expected.
4) If the value in binding <bundle tag is as expected.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>

Author: Hilbrand

custom-checks/checkstyle/src/main/java/org/openhab/tools/analysis/checkstyle/KarafAddonFeatureCheck.java

@@ -0,0 +1,263 @@
+/**
+ * Copyright (c) 2010-2021 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.tools.analysis.checkstyle;
+
+import static org.openhab.tools.analysis.checkstyle.api.CheckConstants.*;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.function.Function;
+
+import javax.xml.xpath.XPathConstants;
+import javax.xml.xpath.XPathExpression;
+import javax.xml.xpath.XPathExpressionException;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.tools.analysis.checkstyle.api.AbstractStaticCheck;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.w3c.dom.Document;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+
+import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
+import com.puppycrawl.tools.checkstyle.api.FileText;
+
+/**
+ * Checks if an add-on has a valid feature.xml file.
+ *
+ * @author Hilbrand Bouwkamp - Initial contribution
+ */
+@NonNullByDefault
+public class KarafAddonFeatureCheck extends AbstractStaticCheck {
+
+    public static final String FEATURE_XML = "feature.xml";
+    public static final String FEATURE_XML_PATH = "src/main/feature/" + FEATURE_XML;
+    public static final String MSG_MISSING_FEATURE_XML = "Missing feature file {0}";
+    public static final String MSG_FEATURES_NAME_INVALID = "Invalid features name, expected name starting with: {0}";
+    public static final String MSG_FEATURE_NAME_INVALID = "Invalid feature name, expected name: {0}";
+    public static final String BUNDLE_VALUE = "mvn:org.openhab.addons.bundles/{0}/$'{'project.version'}'";
+    public static final String MSG_BUNDLE_INVALID = "Invalid or missing bundle entry. Expected <bundle start-level=\"80\">{0}</bundle>";
+
+    private static final String FEATURES_NAME_EXPRESSION = "//features[@name]/@name";
+    private static final String FEATURES_SEARCH = "<features";
+    private static final String FEATURE_NAME_EXPRESSION = "//features/feature[@name]/@name";
+    private static final String FEATURE_SEARCH = "<feature ";
+    private static final String BUNDLE_EXPRESSION = "//features/feature/bundle/text()";
+    private static final String BUNDLE_SEARCH = "mvn:org.openhab.addons.bundles";
+
+    private static final String POM_ARTIFACT_ID_XPATH_EXPRESSION = "//project/artifactId/text()";
+    private static final String POM_PARENT_ARTIFACT_ID = "org.openhab.addons.reactor.bundles";
+    private static final String POM_PARENT_ARTIFACT_ID_XPATH_EXPRESSION = "//project/parent/artifactId/text()";
+
+    private final Logger logger = LoggerFactory.getLogger(KarafAddonFeatureCheck.class);
+
+    private final Map<String, String> featureNamePatternsMap = new LinkedHashMap<>();
+    private final List<String> excludeFeatureNamesList = new ArrayList<>();
+
+    public KarafAddonFeatureCheck() {
+        setFileExtensions(XML_EXTENSION);
+    }
+
+    /**
+     * Comma separated list of binding names (name after pattern applied) of feature names that will be ignored for
+     * checking.
+     *
+     * @param excludeFeatureNames command separated list
+     */
+    public void setExcludeFeatureNames(String excludeFeatureNames) {
+        if (excludeFeatureNames.trim().isBlank()) {
+            return;
+        }
+        for (String exclude : excludeFeatureNames.split(",")) {
+            excludeFeatureNamesList.add(exclude);
+        }
+    }
+
+    /**
+     * Comma separated list of key value pairs (separated by colon) transform the calculated expected feature name in
+     * the actual feature name.
+     * For example openhab-transform-map:openhab-transformation-map.
+     * the openhab-transform-map is as expected as derived from the artifactId, but all bundles use transformation.
+     * So with the pattern expected feature name can be constructed.
+     *
+     * @param featureNamePatterns list of key/value pairs of patterns
+     */
+    public void setFeatureNamePatterns(String featureNamePatterns) {
+        if (featureNamePatterns.trim().isBlank()) {
+            return;
+        }
+        for (String pattern : featureNamePatterns.split(",")) {
+            final String[] keypair = pattern.split(":");
+
+            if (keypair.length == 2) {
+                featureNamePatternsMap.put(keypair[0], keypair[1]);
+            } else {
+                logger.warn("{} check pattern for option featureNamePatterns is invalid. Value set: {}",
+                        getClass().getName(), featureNamePatterns);
+            }
+        }
+    }
+
+    @Override
+    protected void processFiltered(@Nullable File file, @Nullable FileText fileText) throws CheckstyleException {
+        if (file == null || fileText == null) {
+            return;
+        }
+        switch (file.getName()) {
+            case POM_XML_FILE_NAME:
+                checkMissingFeatureFile(file, fileText);
+                break;
+            case FEATURE_XML:
+                checkFeatureFile(file, fileText);
+                break;
+        }
+    }
+
+    private void checkMissingFeatureFile(File file, FileText fileText) throws CheckstyleException {
+        final String artifactId = getArtifactId(fileText);
+
+        if (artifactId == null) {
+            logger.warn("{} will be skipped. Could not find Maven group ID (parent group ID) or artifact ID in {}",
+                    getClass().getSimpleName(), file.getAbsolutePath());
+            return;
+        }
+        final File featureFile = new File(file.getParent(), FEATURE_XML_PATH);
+
+        if (!featureFile.exists()) {
+            logMessage(featureFile.toString(), 0, FEATURE_XML,
+                    MessageFormat.format(MSG_MISSING_FEATURE_XML, featureFile));
+        }
+    }
+
+    private void checkFeatureFile(File featureFile, FileText fileText) throws CheckstyleException {
+        try {
+            final String featureFileString = featureFile.getAbsoluteFile().toString();
+            final FileText pomFile = new FileText(
+                    new File(featureFileString.replace(FEATURE_XML_PATH, ""), POM_XML_FILE_NAME),
+                    StandardCharsets.UTF_8.name());
+            final String artifactId = getArtifactId(pomFile);
+
+            if (artifactId == null) {
+                logger.debug("Ignore check on feature.xml with no bundle specific pom.xml: {}", featureFileString);
+            } else {
+                final Document featureXML = parseDomDocumentFromFile(fileText);
+
+                checkFeatures(featureFile, artifactId, fileText, featureXML);
+                checkFeature(featureFile, artifactId, fileText, featureXML);
+                checkBundle(featureFile, artifactId, fileText, featureXML);
+            }
+        } catch (IOException e) {
+            logger.error("Could not read {}", FEATURE_XML_PATH);
+        }
+    }
+
+    private void checkFeatures(File featureFile, String artifactId, FileText fileText, Document featureXML) {
+        checkName(featureFile, artifactId, fileText, featureXML, FEATURES_NAME_EXPRESSION,
+                name -> !artifactId.equals(name.substring(0, name.indexOf('-'))), MSG_FEATURES_NAME_INVALID,
+                FEATURES_SEARCH);
+    }
+
+    private void checkFeature(File featureFile, String artifactId, FileText fileText, Document featureXML) {
+        final String featureName = adaptedFeatureName(artifactId);
+
+        for (String exclude : excludeFeatureNamesList) {
+            if (exclude.equals(featureName)) {
+                logger.debug("Ignore check on feature name {}", featureName);
+                return;
+            }
+        }
+        checkName(featureFile, featureName, fileText, featureXML, FEATURE_NAME_EXPRESSION,
+                name -> !featureName.equals(name), MSG_FEATURE_NAME_INVALID, FEATURE_SEARCH);
+    }
+
+    private String adaptedFeatureName(String artifactId) {
+        String featureName = artifactId.substring(4).replaceAll("\\.", "-");
+
+        for (Entry<String, String> entry : featureNamePatternsMap.entrySet()) {
+            featureName = featureName.replace(entry.getKey(), entry.getValue());
+        }
+        return featureName;
+    }
+
+    private void checkName(File featureFile, String expectedName, FileText fileText, Document featureXML,
+            String expression, Function<String, Boolean> checkFunction, String message, String lineSearchString) {
+        final Node featuresName = getFirstNode(featureXML, expression);
+        final String errorMessage = MessageFormat.format(message, expectedName);
+
+        if (featuresName == null) {
+            logMessage(featureFile.getAbsolutePath(), 0, featureFile.getName(), errorMessage);
+        } else {
+            String name = featuresName.getNodeValue();
+
+            if (name == null || checkFunction.apply(name)) {
+                logMessage(featureFile.getAbsolutePath(),
+                        findLineNumberSafe(fileText, lineSearchString, 0, errorMessage), featureFile.getName(),
+                        errorMessage);
+            }
+        }
+    }
+
+    private void checkBundle(File featureFile, String artifactId, FileText fileText, Document document) {
+        final String errorMessage = MessageFormat.format(MSG_BUNDLE_INVALID, BUNDLE_VALUE.replace("{0}", artifactId));
+
+        try {
+            final XPathExpression artifactIdExpression = compileXPathExpression(BUNDLE_EXPRESSION);
+            final NodeList bundles = ((NodeList) artifactIdExpression.evaluate(document, XPathConstants.NODESET));
+            final String expectedBundleValue = MessageFormat.format(BUNDLE_VALUE, artifactId);
+            boolean found = false;
+
+            for (int i = 0; i < bundles.getLength(); i++) {
+                final Node item = bundles.item(i);
+                if (expectedBundleValue.equals(item.getNodeValue())) {
+                    found = true;
+                    break;
+                }
+            }
+            if (!found) {
+                logMessage(featureFile.getAbsolutePath(), findLineNumberSafe(fileText, BUNDLE_SEARCH, 0, errorMessage),
+                        featureFile.getName(), errorMessage);
+            }
+        } catch (CheckstyleException | XPathExpressionException e) {
+            logMessage(featureFile.getAbsolutePath(), 0, featureFile.getName(), errorMessage);
+        }
+    }
+
+    private @Nullable String getArtifactId(FileText fileText) throws CheckstyleException {
+        final Document documentXML = parseDomDocumentFromFile(fileText);
+        final Node artifactId = getFirstNode(documentXML, POM_ARTIFACT_ID_XPATH_EXPRESSION);
+        final Node parentArtifactId = getFirstNode(documentXML, POM_PARENT_ARTIFACT_ID_XPATH_EXPRESSION);
+
+        return artifactId == null || parentArtifactId == null
+                || !POM_PARENT_ARTIFACT_ID.equals(parentArtifactId.getNodeValue()) ? null : artifactId.getNodeValue();
+    }
+
+    private @Nullable Node getFirstNode(Document document, String xpathExpression) {
+        try {
+            final XPathExpression artifactIdExpression = compileXPathExpression(xpathExpression);
+
+            return ((NodeList) artifactIdExpression.evaluate(document, XPathConstants.NODESET)).item(0);
+        } catch (CheckstyleException | XPathExpressionException e) {
+            logger.error("Could not evaluate XPath expression {}", xpathExpression, e);
+            return null;
+        }
+    }
+}

custom-checks/checkstyle/src/main/java/org/openhab/tools/analysis/checkstyle/KarafFeatureCheck.java

@@ -87,7 +87,7 @@ protected void processFiltered(File file, FileText fileText) throws CheckstyleEx
                 Path featurePath = resolveRecursively(file.toPath(), Paths.get(singlePath));
 
                 if (featurePath == null) {
-                    logger.warn("Could not find file feature file {}", singlePath);
+                    logger.debug("Could not find file feature file {}", singlePath);
                     continue;
                 }
 

custom-checks/checkstyle/src/test/java/org/openhab/tools/analysis/checkstyle/test/KarafAddonFeatureCheckTest.java

@@ -0,0 +1,95 @@
+/**
+ * Copyright (c) 2010-2021 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.tools.analysis.checkstyle.test;
+
+import static org.openhab.tools.analysis.checkstyle.KarafAddonFeatureCheck.*;
+import static org.openhab.tools.analysis.checkstyle.api.CheckConstants.POM_XML_FILE_NAME;
+
+import java.io.File;
+import java.text.MessageFormat;
+
+import org.apache.commons.lang.ArrayUtils;
+import org.junit.Test;
+import org.openhab.tools.analysis.checkstyle.KarafAddonFeatureCheck;
+import org.openhab.tools.analysis.checkstyle.api.AbstractStaticCheckTest;
+
+import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
+
+/**
+ * Tests for {@link KarafAddonFeatureCheck}
+ *
+ * @author Hilbrand Bouwkamp - Initial contribution
+ */
+public class KarafAddonFeatureCheckTest extends AbstractStaticCheckTest {
+
+    private static final String ARTIFACT_ID = "org.openhab.binding.example";
+    private static final DefaultConfiguration CONFIGURATION = createModuleConfig(KarafAddonFeatureCheck.class);
+
+    @Override
+    protected String getPackageLocation() {
+        return "checkstyle/karafAddonFeatureCheck";
+    }
+
+    @Test
+    public void testValid() throws Exception {
+        verify("valid", ArrayUtils.EMPTY_STRING_ARRAY);
+    }
+
+    @Test
+    public void testMissingFeatureFile() throws Exception {
+        final File featureFile = new File(getPath("missingFeature"), FEATURE_XML_PATH);
+
+        verify(createChecker(CONFIGURATION), getPath("missingFeature" + File.separator + POM_XML_FILE_NAME),
+                featureFile.getAbsolutePath(),
+                generateExpectedMessages(0, MessageFormat.format(MSG_MISSING_FEATURE_XML, featureFile)));
+    }
+
+    @Test
+    public void testInvalidBundleName() throws Exception {
+        verify("invalidBundle", generateExpectedMessages(7,
+                MessageFormat.format(MSG_BUNDLE_INVALID, MessageFormat.format(BUNDLE_VALUE, ARTIFACT_ID))));
+    }
+
+    @Test
+    public void testInvalidFeaturesName() throws Exception {
+        verify("invalidFeaturesName",
+                generateExpectedMessages(2, MessageFormat.format(MSG_FEATURES_NAME_INVALID, ARTIFACT_ID)));
+    }
+
+    @Test
+    public void testInvalidFeatureName() throws Exception {
+        verify("invalidFeatureName", generateExpectedMessages(5,
+                MessageFormat.format(MSG_FEATURE_NAME_INVALID, ARTIFACT_ID.replaceAll("\\.", "-").substring(4))));
+    }
+
+    @Test
+    public void testPatternFeatureName() throws Exception {
+        DefaultConfiguration config = createModuleConfig(KarafAddonFeatureCheck.class);
+        config.addAttribute("featureNamePatterns", "openhab-binding-example:org.openhab.binding.someother");
+        verify(config, getPath("invalidFeatureName" + File.separator + FEATURE_XML_PATH),
+                ArrayUtils.EMPTY_STRING_ARRAY);
+    }
+
+    @Test
+    public void testExcludedFeatureName() throws Exception {
+        DefaultConfiguration config = createModuleConfig(KarafAddonFeatureCheck.class);
+        config.addAttribute("featureNamePatterns", "openhab-binding-example:org.openhab.binding.someother");
+        config.addAttribute("excludeFeatureNames", "org.openhab.binding.someother");
+        verify(config, getPath("invalidFeatureName" + File.separator + FEATURE_XML_PATH),
+                ArrayUtils.EMPTY_STRING_ARRAY);
+    }
+
+    private void verify(String fileDirectory, String[] expectedMessages) throws Exception {
+        verify(CONFIGURATION, getPath(fileDirectory + File.separator + FEATURE_XML_PATH), expectedMessages);
+    }
+}

custom-checks/checkstyle/src/test/resources/checkstyle/karafAddonFeatureCheck/invalidBundle/pom.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xmlns="http://maven.apache.org/POM/4.0.0"
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+
+  <parent>
+    <groupId>org.openhab.addons.bundles</groupId>
+    <artifactId>org.openhab.addons.reactor.bundles</artifactId>
+    <version>3.0.0-SNAPSHOT</version>
+  </parent>
+
+  <artifactId>org.openhab.binding.example</artifactId>
+
+  <name>openHAB Add-ons :: Bundles :: Example Binding</name>
+
+</project>

custom-checks/checkstyle/src/test/resources/checkstyle/karafAddonFeatureCheck/invalidBundle/src/main/feature/feature.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<features name="org.openhab.binding.example-${project.version}" xmlns="http://karaf.apache.org/xmlns/features/v1.4.0">
+    <repository>mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-core/${project.version}/xml/features</repository>
+
+    <feature name="openhab-binding-example" description="Example Binding" version="${project.version}">
+        <feature>openhab-runtime-base</feature>
+        <bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.wrong/${project.version}</bundle>
+    </feature>
+</features>