TextValidator/SdkAnalyzer: name offending label ids and summarise missing labels

src/main/java/eu/europa/ted/eforms/sdk/analysis/CliCommand.java

@@ -11,6 +11,7 @@
 import picocli.CommandLine.Command;
 import picocli.CommandLine.IVersionProvider;
 import picocli.CommandLine.Model.CommandSpec;
+import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
 import picocli.CommandLine.Spec;
 
@@ -25,9 +26,13 @@ class CliCommand implements Callable<Integer> {
   @Parameters(index = "0", description = "SDK resources root folder.")
   private Path sdkRoot;
 
+  @Option(names = {"-v", "--verbose"},
+      description = "Print every individual missing-label error in addition to the summary.")
+  private boolean verbose;
+
   @Override
   public Integer call() throws Exception {
-    return SdkAnalyzer.analyze(sdkRoot);
+    return SdkAnalyzer.analyze(sdkRoot, verbose);
   }
 
   @Command(name = "benchmark", mixinStandardHelpOptions = true, 

src/main/java/eu/europa/ted/eforms/sdk/analysis/SdkAnalyzer.java

@@ -2,11 +2,17 @@
 
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
+import java.util.TreeMap;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import eu.europa.ted.eforms.sdk.analysis.enums.MissingLabelKind;
 import eu.europa.ted.eforms.sdk.analysis.validator.EfxValidator;
 import eu.europa.ted.eforms.sdk.analysis.validator.SchematronValidator;
 import eu.europa.ted.eforms.sdk.analysis.validator.SdkValidator;
@@ -21,6 +27,10 @@ public class SdkAnalyzer {
   private SdkAnalyzer() {}
 
   public static int analyze(final Path sdkRoot) throws Exception {
+    return analyze(sdkRoot, false);
+  }
+
+  public static int analyze(final Path sdkRoot, final boolean verbose) throws Exception {
     logger.info("Analyzing SDK under folder [{}]", sdkRoot);
 
     List<ValidationResult> warnings = new ArrayList<>();
@@ -32,7 +42,7 @@ public static int analyze(final Path sdkRoot) throws Exception {
         new SchematronValidator(sdkRoot),
         new SdkValidator(sdkRoot),
         new EfxValidator(sdkRoot));
-    
+
     for (Validator validator : validators) {
       String validatorName = validator.getClass().getSimpleName();
       logger.info("Starting validation with {}", validatorName);
@@ -43,8 +53,9 @@ public static int analyze(final Path sdkRoot) throws Exception {
         logger.warn("Warnings from {}:\n{}", validatorName, StringUtils.join(foundWarnings, '\n'));
       }
       Set<ValidationResult> foundErrors = validator.getErrors();
-      if (!foundErrors.isEmpty()) {
-        logger.error("Errors from {}:\n{}", validatorName, StringUtils.join(foundErrors, '\n'));
+      List<ValidationResult> errorsToLog = forDisplay(foundErrors, verbose);
+      if (!errorsToLog.isEmpty()) {
+        logger.error("Errors from {}:\n{}", validatorName, StringUtils.join(errorsToLog, '\n'));
       }
 
       warnings.addAll(foundWarnings);
@@ -57,10 +68,79 @@ public static int analyze(final Path sdkRoot) throws Exception {
     }
 
     if (!errors.isEmpty() && logger.isErrorEnabled()) {
-      logger.error("All validation errors:\n{}", StringUtils.join(errors, '\n'));
+      List<ValidationResult> errorsToLog = forDisplay(errors, verbose);
+      if (!errorsToLog.isEmpty()) {
+        logger.error("All validation errors:\n{}", StringUtils.join(errorsToLog, '\n'));
+      }
       logger.error("Total number of validation errors: {}", errors.size());
     }
 
+    reportMissingLabels(errors, verbose);
+
     return errors.isEmpty() ? 0 : 1;
   }
+
+  /**
+   * The errors to print individually. Without verbose output the per-occurrence missing-label errors
+   * are left out, since they are summarised by {@link #reportMissingLabels}; all other errors are
+   * always printed.
+   */
+  private static List<ValidationResult> forDisplay(Collection<ValidationResult> results,
+      boolean verbose) {
+    if (verbose) {
+      return new ArrayList<>(results);
+    }
+    return results.stream()
+        .filter(result -> result.getMissingLabelIds().isEmpty())
+        .collect(Collectors.toList());
+  }
+
+  /**
+   * Logs a deduplicated report of the labels that have no text, split by how they were detected:
+   * referenced by the SDK but absent, and assumed missing because the exporter left their identifier
+   * inside another label's text. Each identifier is listed once with the number of references, so
+   * the labels that actually need to be added stand out from the thousands of individual errors.
+   */
+  private static void reportMissingLabels(List<ValidationResult> errors, boolean verbose) {
+    if (!logger.isErrorEnabled()) {
+      return;
+    }
+
+    Map<String, Long> found = countMissingLabels(errors, MissingLabelKind.FOUND);
+    Map<String, Long> assumed = countMissingLabels(errors, MissingLabelKind.ASSUMED);
+
+    if (found.isEmpty() && assumed.isEmpty()) {
+      return;
+    }
+
+    logMissingLabels("Labels found missing from the export, referenced by the SDK but not present",
+        found);
+    logMissingLabels(
+        "Labels assumed missing during export, identifier left in label text by the exporter",
+        assumed);
+
+    if (!verbose) {
+      logger.error("Re-run the analyzer with --verbose to see the individual occurrences behind the"
+          + " missing label detection.");
+    }
+  }
+
+  private static Map<String, Long> countMissingLabels(List<ValidationResult> errors,
+      MissingLabelKind kind) {
+    return errors.stream()
+        .filter(error -> error.getMissingLabelKind() == kind)
+        .flatMap(error -> error.getMissingLabelIds().stream())
+        .collect(Collectors.groupingBy(Function.identity(), TreeMap::new, Collectors.counting()));
+  }
+
+  private static void logMissingLabels(String title, Map<String, Long> missingLabels) {
+    if (missingLabels.isEmpty()) {
+      return;
+    }
+    StringBuilder report = new StringBuilder();
+    missingLabels.forEach(
+        (labelId, count) -> report.append(String.format("%n  %s (%d)", labelId, count)));
+    logger.error("{} ({} unique, identifier and number of references):{}", title,
+        missingLabels.size(), report);
+  }
 }

src/main/java/eu/europa/ted/eforms/sdk/analysis/enums/MissingLabelKind.java

@@ -0,0 +1,16 @@
+package eu.europa.ted.eforms.sdk.analysis.enums;
+
+/**
+ * How a missing label was detected.
+ */
+public enum MissingLabelKind {
+  /** The result does not concern a missing label. */
+  NONE,
+  /** A label is referenced by the SDK but is not present in the export. */
+  FOUND,
+  /**
+   * A label identifier was left inside another label's text by the exporter, which happens when the
+   * label could not be resolved during export.
+   */
+  ASSUMED;
+}

src/main/java/eu/europa/ted/eforms/sdk/analysis/validator/TextValidator.java

@@ -3,8 +3,11 @@
 import java.io.FileNotFoundException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
+import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.apache.commons.lang3.Validate;
@@ -14,6 +17,7 @@
 import eu.europa.ted.eforms.sdk.analysis.SdkLoader;
 import eu.europa.ted.eforms.sdk.analysis.domain.enums.Language;
 import eu.europa.ted.eforms.sdk.analysis.domain.label.Label;
+import eu.europa.ted.eforms.sdk.analysis.enums.MissingLabelKind;
 import eu.europa.ted.eforms.sdk.analysis.enums.ValidationStatusEnum;
 import eu.europa.ted.eforms.sdk.analysis.fact.LabelFact;
 import eu.europa.ted.eforms.sdk.analysis.vo.ValidationResult;
@@ -24,8 +28,9 @@
 public class TextValidator implements Validator {
   private static final Logger logger = LoggerFactory.getLogger(TextValidator.class);
 
-  // Match label identifiers, so | with characters before and after.
-  private static Pattern labelIdPattern = Pattern.compile(".*[a-z0-9]\\|[a-z0-9].*");
+  // Match a label identifier token, e.g. "expression|name|906" or "business-entity|name|UBO".
+  private static final Pattern labelIdPattern =
+      Pattern.compile("[a-z][a-z-]*\\|[a-z]+(?:\\|\\S+)?");
 
   private final SdkLoader sdkLoader;
 
@@ -71,9 +76,16 @@ private void checkCharacters(Label l, Language lang, String text) {
   }
 
   private void checkIdReference(Label l, Language lang, String text) {
-    if (labelIdPattern.matcher(text).matches()) {
-      String msg = String.format("Label in %s contains label identifier", lang);
-      results.add(new ValidationResult(new LabelFact(l), msg, ValidationStatusEnum.ERROR));
+    Matcher matcher = labelIdPattern.matcher(text);
+    List<String> ids = new ArrayList<>();
+    while (matcher.find()) {
+      ids.add(matcher.group());
+    }
+    if (!ids.isEmpty()) {
+      String msg = String.format("Label in %s contains label identifier(s): %s", lang,
+          String.join(", ", ids));
+      results.add(new ValidationResult(new LabelFact(l), msg, ValidationStatusEnum.ERROR, ids,
+          MissingLabelKind.ASSUMED));
     }
   }
 

src/main/java/eu/europa/ted/eforms/sdk/analysis/vo/ValidationResult.java

@@ -1,19 +1,38 @@
 package eu.europa.ted.eforms.sdk.analysis.vo;
 
 import java.text.MessageFormat;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
 import eu.europa.ted.eforms.sdk.analysis.Identifiable;
+import eu.europa.ted.eforms.sdk.analysis.enums.MissingLabelKind;
 import eu.europa.ted.eforms.sdk.analysis.enums.ValidationStatusEnum;
 import eu.europa.ted.eforms.sdk.analysis.fact.SdkComponentFact;
 
 public class ValidationResult {
   private final SdkComponentFact<?> fact;
   private final String message;
   private final ValidationStatusEnum status;
+  private final Set<String> missingLabelIds;
+  private final MissingLabelKind missingLabelKind;
 
   public ValidationResult(SdkComponentFact<?> fact, String message, ValidationStatusEnum status) {
+    this(fact, message, status, Collections.emptySet(), MissingLabelKind.NONE);
+  }
+
+  public ValidationResult(SdkComponentFact<?> fact, String message, ValidationStatusEnum status,
+      Collection<String> missingLabelIds) {
+    this(fact, message, status, missingLabelIds, MissingLabelKind.FOUND);
+  }
+
+  public ValidationResult(SdkComponentFact<?> fact, String message, ValidationStatusEnum status,
+      Collection<String> missingLabelIds, MissingLabelKind missingLabelKind) {
     this.fact = fact;
     this.message = message;
     this.status = status;
+    this.missingLabelIds = Collections.unmodifiableSet(new LinkedHashSet<>(missingLabelIds));
+    this.missingLabelKind = missingLabelKind;
   }
 
   public Identifiable<?> getFact() {
@@ -28,6 +47,24 @@ public ValidationStatusEnum getStatus() {
     return status;
   }
 
+  /**
+   * The label identifiers that this result reports as missing (referenced but without text). Empty
+   * for results unrelated to missing labels.
+   */
+  public Set<String> getMissingLabelIds() {
+    return missingLabelIds;
+  }
+
+  /**
+   * How the missing labels were detected: {@link MissingLabelKind#FOUND} when referenced by the SDK
+   * but not present, {@link MissingLabelKind#ASSUMED} when the exporter left the identifier inside
+   * another label's text, or {@link MissingLabelKind#NONE} when this result is not about missing
+   * labels.
+   */
+  public MissingLabelKind getMissingLabelKind() {
+    return missingLabelKind;
+  }
+
   @Override
   public String toString() {
     return MessageFormat.format("{0}({1}) - {2}: {3}", fact.getTypeName(), fact.getId(), status,

src/main/resources/eu/europa/ted/eforms/sdk/analysis/drools/codelistRules.drl

@@ -61,7 +61,7 @@ when
   /codelistsIndex[ $cli: this ]/indexItems[ $labelId: labelId ]
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($cli, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($cli, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end
 
 rule "Every column reference exists in the column definition"

src/main/resources/eu/europa/ted/eforms/sdk/analysis/drools/fieldsAndNodesRules.drl

@@ -174,7 +174,7 @@ when
   $labelId : /fields[ $f: this ]/allLabelIds
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($f, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($f, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end
 
 rule "Fields corresponding to XML attributes have the expected information"
@@ -227,7 +227,7 @@ when
   /businessEntities[ $b: this, $labelId: labelId ]
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($b, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($b, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end
 
 rule "Relationships between business entities and nodes are consistent"

src/main/resources/eu/europa/ted/eforms/sdk/analysis/drools/noticeTypeRules.drl

@@ -33,7 +33,7 @@ when
   $labelId: String() from $nti.labelIds
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($nti, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($nti, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end
 
 // TEDEFO-1816: Check that the view templates referenced in the notice-types.json file actually exist.
@@ -72,7 +72,7 @@ when
   $labelId: String() from $nt.labelIds
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($nt, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($nt, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end
 
 // TEDEFO-1820: For every <noticeSubtype>.json file: Check that any entry with contentType="field", has a corresponding field inside fields/fields.json

src/main/resources/eu/europa/ted/eforms/sdk/analysis/drools/viewTemplatesRules.drl

@@ -7,5 +7,5 @@ when
   $labelId : /viewTemplatesIndex[ $v: this ]/labelIds
   not (exists /labels[ id == $labelId ])
 then
-  results.add(new ValidationResult($v, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR));
+  results.add(new ValidationResult($v, "Referenced label " + $labelId + " does not exist", ValidationStatusEnum.ERROR, java.util.List.of($labelId)));
 end

src/test/java/eu/europa/ted/eforms/sdk/analysis/validator/TextValidatorTest.java

@@ -0,0 +1,48 @@
+package eu.europa.ted.eforms.sdk.analysis.validator;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.nio.file.Path;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.junit.jupiter.api.Test;
+
+import eu.europa.ted.eforms.sdk.analysis.enums.MissingLabelKind;
+import eu.europa.ted.eforms.sdk.analysis.vo.ValidationResult;
+
+/**
+ * Verifies that {@link TextValidator} names the offending label identifier(s) in the error
+ * message, rather than only reporting that some identifier is present.
+ */
+class TextValidatorTest {
+
+  @Test
+  void errorMessageNamesTheOffendingLabelIdentifiers() throws Exception {
+    final Path sdkRoot =
+        Path.of(getClass().getResource("/eforms-sdk-tests/tedefo-3301/invalid").toURI());
+
+    final TextValidator validator = new TextValidator(sdkRoot);
+    validator.validate();
+
+    final Set<ValidationResult> errors = validator.getErrors();
+    assertEquals(2, errors.size());
+
+    final Set<String> messages =
+        errors.stream().map(ValidationResult::getMessage).collect(Collectors.toSet());
+
+    assertTrue(messages.stream().anyMatch(m -> m.contains("expression|name|123")),
+        () -> "Expected an error naming expression|name|123, got: " + messages);
+    assertTrue(messages.stream().anyMatch(m -> m.contains("field|name|BT-132-Lot")),
+        () -> "Expected an error naming field|name|BT-132-Lot, got: " + messages);
+
+    final Set<String> missingLabelIds = errors.stream()
+        .flatMap(error -> error.getMissingLabelIds().stream()).collect(Collectors.toSet());
+
+    assertEquals(Set.of("expression|name|123", "field|name|BT-132-Lot"), missingLabelIds);
+
+    assertTrue(errors.stream().allMatch(e -> e.getMissingLabelKind() == MissingLabelKind.ASSUMED),
+        "Text-detected missing labels should be reported as ASSUMED");
+  }
+}