Unify validation failure framework.

src/main/java/htsjdk/variant/vcf/VCFCompoundHeaderLine.java

@@ -180,16 +180,14 @@ public Optional<VCFValidationFailure<VCFHeaderLine>> validateForVersion(final VC
         // However, the key values correspond to INFO/FORMAT header lines defining the attribute and its type,
         // so we do the validation here
         if (vcfTargetVersion.isAtLeastAsRecentAs(VCFHeaderVersion.VCF4_3)) {
-            final Optional<VCFValidationFailure<VCFHeaderLine>> validationFailure = validateKeyOrID(getID())
-                .map(e -> new VCFValidationFailure<>(vcfTargetVersion, this, e));
+            final Optional<String> validationFailure = validateID(getID());
             if (validationFailure.isPresent()) {
-                // TODO thinking that these getValidationFailure should be a pure function and its caller
-                //  decides whether to pass the error up or just log if not using strict validation
                 if (VCFUtils.isStrictVCFVersionValidation()) {
-                    return validationFailure;
+                    return Optional.of(
+                            new VCFValidationFailure<>(vcfTargetVersion, this, validationFailure.get()));
                 } else {
                     // warn for older versions - this line can't be used as a v4.3 line
-                    logger.warn(validationFailure.get().getFailureMessage());
+                    logger.warn(validationFailure.get());
                 }
             }
         }
@@ -199,13 +197,13 @@ public Optional<VCFValidationFailure<VCFHeaderLine>> validateForVersion(final VC
 
     /**
      * @param id the candidate ID
-     * @return true if ID conforms to header line id requirements, otherwise false
+     * @return an Optional error message indicating the reason for the validation failure
      */
     @Override
-    protected Optional<String> validateKeyOrID(final String id) {
+    protected Optional<String> validateID(final String id) {
         return VALID_HEADER_ID_PATTERN.matcher(id).matches()
-            ? Optional.empty()
-            : Optional.of(String.format("Key: %s does not match header line key regex: %s", id, VALID_HEADER_ID_PATTERN));
+            ? super.validateID(id)
+            : Optional.of(String.format("ID: %s does not match header line ID regex: %s", id, VALID_HEADER_ID_PATTERN));
     }
 
     /**

src/main/java/htsjdk/variant/vcf/VCFHeader.java

@@ -605,8 +605,14 @@ private VCFHeaderVersion initializeHeaderVersion() {
         return metaDataVersion;
     }
 
-    public Collection<VCFValidationFailure<VCFHeaderLine>> getValidationErrors(final VCFHeaderVersion targetVersion) {
-        return mMetaData.getValidationErrors(targetVersion);
+    /**
+     * Return any validation failures that would result from upgrading this header to {@code targetVersion}
+     *
+     * @param targetVersion target version to use for validation
+     * @return collection of {@link VCFValidationFailure} objects
+     */
+     public Collection<VCFValidationFailure<VCFHeaderLine>> getVersionValidationFailures(final VCFHeaderVersion targetVersion) {
+        return mMetaData.getVersionValidationFailures(targetVersion);
     }
 
     private void validateVersionTransition(
@@ -649,7 +655,7 @@ public VCFHeader upgradeVersion(final VCFVersionUpgradePolicy policy) {
                 return this;
             case UPGRADE_OR_FALLBACK: {
                 final Collection<VCFValidationFailure<VCFHeaderLine>> errors =
-                    this.mMetaData.getValidationErrors(VCFHeader.DEFAULT_VCF_VERSION);
+                    mMetaData.getVersionValidationFailures(VCFHeader.DEFAULT_VCF_VERSION);
                 if (errors.isEmpty()) {
                     final VCFHeader newHeader = new VCFHeader(this);
                     // If validation fails, simply pass the exception through

src/main/java/htsjdk/variant/vcf/VCFHeaderMerger.java

@@ -124,7 +124,10 @@ private static Set<VCFHeaderLine> makeMergedMetaDataSet(
             final HeaderMergeConflictWarnings conflictWarner) {
 
         if (conflictWarner.emitWarnings) {
-            mergedMetaData.getValidationErrors(newestVersion)
+            //TODO: any header contains a line that fails version validation, then the merge should fail...just like
+            // a version upgrade would fail for that same header. We can't honor the fallback policy i.e., fallback to
+            // the old version) here because that would require knowing how to back-version the OTHER headers being merged
+            mergedMetaData.getVersionValidationFailures(newestVersion)
                     .forEach(validationError -> conflictWarner.warn(validationError.getFailureMessage()));
         }
 

src/main/java/htsjdk/variant/vcf/VCFInfoHeaderLine.java

@@ -104,10 +104,10 @@ public static VCFInfoHeaderLine getMergedInfoHeaderLine(
     }
 
     @Override
-    protected Optional<String> validateKeyOrID(final String id) {
+    protected Optional<String> validateID(final String id) {
         return id.equals(VCFConstants.THOUSAND_GENOMES_KEY)
             ? Optional.empty()
-            : super.validateKeyOrID(id);
+            : super.validateID(id);
     }
 
     @Override

src/main/java/htsjdk/variant/vcf/VCFMetaDataLines.java

@@ -183,7 +183,6 @@ public VCFHeaderLine findEquivalentHeaderLine(final VCFHeaderLine queryLine) {
 
     /**
      * Validate all metadata lines, excluding the file format line against a target version.
-     * Throws {@link TribbleException.VersionValidationFailure} if any line is incompatible with the given version.
      * @param targetVersion the target version to validate against
      * @throws {@link TribbleException.VersionValidationFailure} if any existing line fails to validate against
      * {@code targetVersion}
@@ -206,7 +205,7 @@ public void validateMetaDataLinesOrThrow(final VCFHeaderVersion targetVersion) {
      * @return an Collection<VCFValidationFailure> describing the lines that failed to validate
      * incompatible with targetVersion. The collections is empty if validation succeeded for all lines.
      */
-    public Collection<VCFValidationFailure> getValidationErrors(final VCFHeaderVersion targetVersion) {
+    public Collection<VCFValidationFailure<VCFHeaderLine>> getVersionValidationFailures(final VCFHeaderVersion targetVersion) {
         return mMetaData.values().stream()
                 .filter(line -> !VCFHeaderVersion.isFormatString(line.getKey()))
                 .map(l -> l.validateForVersion(targetVersion))

src/main/java/htsjdk/variant/vcf/VCFSimpleHeaderLine.java

@@ -61,14 +61,15 @@ public class VCFSimpleHeaderLine extends VCFHeaderLine implements VCFIDHeaderLin
 
     // Map used to retain the attribute/value pairs, in original order. The first entry in the map must be
     // an ID field. The entire map must be immutable to prevent hash values from changing, since these are
-    // often stored in Sets. Its not ACTUALLY immutable in orderto allow for special cases where subclasses
-    // have to be able to "repair" header lines (via a call to updateGenericField) during constructor validation.
+    // often stored in Sets. Note that this value is not ACTUALLY immutable, in order to allow for special
+    // cases where subclasses need to be able to "repair" header lines (via a call to updateGenericField)
+    // during constructor validation.
     //
     // Otherwise the values here should never change during the lifetime of the header line.
     private final Map<String, String> genericFields = new LinkedHashMap();
 
     /**
-     * Constructor that accepts a key and string that represetns the rest of the line (after the ##KEY=").
+     * Constructor that accepts a key and string that represents the rest of the line (after the ##KEY=").
      * @param key the key to use for this line
      * @param line the value part of the line
      * @param version the target version to validate the line against
@@ -236,15 +237,15 @@ protected boolean isPercentEncodableAttribute(final String attributeName) {
             attributeName.equals(VCFCompoundHeaderLine.TYPE_ATTRIBUTE));
     }
 
-    private void validate() {
-        if ( genericFields.isEmpty() || !genericFields.keySet().stream().findFirst().get().equals(ID_ATTRIBUTE)) {
-            throw new TribbleException(
-                    String.format("The required ID tag is missing or not the first attribute: key=%s", super.getKey()));
-        }
-        final Optional<String> validationFailure = validateKeyOrID(getGenericFieldValue(ID_ATTRIBUTE));
-        if (validationFailure.isPresent()) {
-            throw new TribbleException.VersionValidationFailure(validationFailure.get());
-        }
+    /**
+     * Validate a string as an ID field.
+     *
+     * Note: this returns an Optional String rather than a {@link VCFValidationFailure}, because it needs to work
+     * in contexts where no VCF header version is supplied (specifically, in constructors that do not require
+     * a version)
+     */
+    protected Optional<String> validateID(final String id) {
+        return validateAttributeName(id, "structured header line ID");
     }
 
     // Perform all text transformations required to encode an attribute value

src/test/java/htsjdk/variant/vcf/VCFCompoundHeaderLineUnitTest.java

@@ -89,7 +89,7 @@ public Object[][] getInvalidLines() {
         };
     }
 
-    @Test(dataProvider = "invalidIDs", expectedExceptions = TribbleException.VersionValidationFailure.class)
+    @Test(dataProvider = "invalidIDs", expectedExceptions = TribbleException.class)
     public void testGetValidationError(final String lineString) {
         new VCFInfoHeaderLine(lineString, VCFHeader.DEFAULT_VCF_VERSION);
     }

src/test/java/htsjdk/variant/vcf/VCFHeaderLineUnitTest.java

@@ -118,7 +118,7 @@ public Object[][] vcfVersions() {
     }
 
     @Test(dataProvider = "vcfVersions")
-    public void testValidateForVersion(final VCFHeaderVersion vcfVersion) {
+    public void testValidateForVersionOrThrowPositive(final VCFHeaderVersion vcfVersion) {
         VCFHeaderLine headerLine = new VCFHeaderLine(vcfVersion.getFormatString(), vcfVersion.getVersionString());
         headerLine.validateForVersionOrThrow(vcfVersion);
     }
@@ -137,7 +137,7 @@ public Object[][] incompatibleVersionPairs() {
     }
 
     @Test(dataProvider="incompatibleVersions", expectedExceptions= TribbleException.VersionValidationFailure.class)
-    public void testValidateForVersionFails(final VCFHeaderVersion vcfVersion, final VCFHeaderVersion incompatibleVersion) {
+    public void testValidateForVersionOrThrowNegative(final VCFHeaderVersion vcfVersion, final VCFHeaderVersion incompatibleVersion) {
         VCFHeaderLine headerLine = new VCFHeaderLine(vcfVersion.getFormatString(), vcfVersion.getVersionString());
         headerLine.validateForVersionOrThrow(incompatibleVersion);
     }