Merge pull request #489 from CodeForPhilly/issue/483-malformed-stored-version-can-permanently-break-c

issue/483 malformed stored version can permanently break c

Author: prestoncabe

builder-api/src/main/java/org/acme/controller/EligibilityCheckResource.java

@@ -11,6 +11,7 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import org.acme.auth.AuthUtils;
 import org.acme.constants.CheckStatus;
+import org.acme.model.domain.CheckVersion;
 import org.acme.model.domain.EligibilityCheck;
 import org.acme.model.dto.EligibilityCheck.CheckDmnRequest;
 import org.acme.model.dto.EligibilityCheck.CreateCheckRequest;
@@ -21,12 +22,12 @@
 import org.acme.service.CustomCheckDmnTemplate;
 import org.acme.service.DmnService;
 
-import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 @Path("/api/custom-checks")
 public class EligibilityCheckResource {
@@ -344,7 +345,7 @@ public Response publishCustomCheck(@Context SecurityIdentity identity, @PathPara
                     .entity(Map.of("error", "could not read published versions of Check, published check version was not created"))
                     .build();
         }
-        check.setVersion(versionForPublish(check.getVersion(), publishedChecks));
+        check.setVersion(versionForPublish(check, publishedChecks));
 
         // Update the working check so the extracted input definition and current version are saved.
         try {
@@ -444,34 +445,36 @@ public Response getPublishedVersionsOfWorkingCheck(@Context SecurityIdentity ide
 
     // ========== Private Helper Methods ==========
 
-    private static final Comparator<int[]> VERSION_ORDER = Comparator
-            .<int[]>comparingInt(v -> v[0])
-            .thenComparingInt(v -> v[1])
-            .thenComparingInt(v -> v[2]);
+    /* The first publish keeps the working version; later ones increment past the highest published
+       version, so a working version that lags what is already published cannot produce a duplicate
+       published id. */
+    String versionForPublish(EligibilityCheck check, List<EligibilityCheck> publishedChecks) {
+        Set<String> publishedIds = publishedChecks.stream()
+                .map(EligibilityCheck::getId)
+                .filter(id -> id != null)
+                .collect(Collectors.toSet());
 
-    /* The first publish keeps the working version; later ones increment past the highest published version,
-       so a working version that lags what is already published cannot produce a duplicate published id. */
-    String versionForPublish(String workingVersion, List<EligibilityCheck> publishedChecks) {
-        return publishedChecks.stream()
+        int[] version = publishedChecks.stream()
                 .map(EligibilityCheck::getVersion)
-                .filter(Objects::nonNull)
-                .map(this::normalize)
-                .max(VERSION_ORDER)
-                .map(this::incrementMajorVersion)
-                .orElse(workingVersion);
+                .flatMap(publishedVersion -> parsePublishedVersion(publishedVersion).stream())
+                .max(CheckVersion.ORDER)
+                .map(CheckVersion::nextMajor)
+                .orElseGet(() -> CheckVersion.parse(check.getVersion()).orElseGet(CheckVersion::initial));
+
+        /* A published id is derived from the version and can never be written twice, so skip past
+           versions that are already taken. A version ignored above still holds its id, which is the
+           only remaining record that the version was used. */
+        while (publishedIds.contains(eligibilityCheckRepository.getPublishedId(check, CheckVersion.format(version)))) {
+            version = CheckVersion.nextMajor(version);
+        }
+        return CheckVersion.format(version);
     }
 
-    private String incrementMajorVersion(int[] version) {
-        return (version[0] + 1) + ".0.0";    // increment major, reset minor and patch
-    }
-
-    private int[] normalize(String version) {
-        String[] parts = version.split("\\.");
-        int[] nums = new int[]{0, 0, 0};
-
-        for (int i = 0; i < parts.length && i < 3; i++) {
-            nums[i] = Integer.parseInt(parts[i]);
+    private Optional<int[]> parsePublishedVersion(String version) {
+        Optional<int[]> parsed = CheckVersion.parse(version);
+        if (parsed.isEmpty()) {
+            Log.warn("Ignoring malformed published check version: " + version);
         }
-        return nums;
+        return parsed;
     }
 }

builder-api/src/main/java/org/acme/model/domain/CheckVersion.java

@@ -0,0 +1,68 @@
+package org.acme.model.domain;
+
+import java.util.Comparator;
+import java.util.Optional;
+
+/* Check versions are stored as free-form strings, so a stored version can be anything at all.
+   Parsing yields an empty result for values that are not versions, which lets callers ignore
+   corrupt data instead of failing every later read and publish of the check. */
+public final class CheckVersion {
+
+    private static final int PART_COUNT = 3;
+
+    public static final Comparator<int[]> ORDER = Comparator
+            .<int[]>comparingInt(version -> version[0])
+            .thenComparingInt(version -> version[1])
+            .thenComparingInt(version -> version[2]);
+
+    private CheckVersion() {
+    }
+
+    /* A version is one to three dot-separated non-negative numbers; missing parts count as zero. */
+    public static Optional<int[]> parse(String version) {
+        if (version == null) {
+            return Optional.empty();
+        }
+
+        // Java drops trailing empty parts, so dot-only strings such as "." split into no parts at all
+        String[] parts = version.split("\\.");
+        if (parts.length == 0 || parts.length > PART_COUNT) {
+            return Optional.empty();
+        }
+
+        int[] numbers = new int[PART_COUNT];
+        for (int i = 0; i < parts.length; i++) {
+            try {
+                numbers[i] = Integer.parseInt(parts[i]);
+            } catch (NumberFormatException e) {
+                return Optional.empty();
+            }
+            if (numbers[i] < 0) {
+                return Optional.empty();
+            }
+        }
+        return Optional.of(numbers);
+    }
+
+    /* Orders stored versions, treating one that cannot be read as older than one that can. */
+    public static int compare(String version1, String version2) {
+        Optional<int[]> parsed1 = parse(version1);
+        Optional<int[]> parsed2 = parse(version2);
+        if (parsed1.isPresent() && parsed2.isPresent()) {
+            return ORDER.compare(parsed1.get(), parsed2.get());
+        }
+        return Boolean.compare(parsed1.isPresent(), parsed2.isPresent());
+    }
+
+    public static int[] initial() {
+        return new int[]{1, 0, 0};
+    }
+
+    public static int[] nextMajor(int[] version) {
+        return new int[]{version[0] + 1, 0, 0};    // increment major, reset minor and patch
+    }
+
+    public static String format(int[] version) {
+        return version[0] + "." + version[1] + "." + version[2];
+    }
+}

builder-api/src/main/java/org/acme/persistence/EligibilityCheckRepository.java

@@ -33,4 +33,7 @@ public interface EligibilityCheckRepository {
     void deleteWorkingCustomCheck(String checkId) throws Exception;
 
     void updatePublishedCustomCheck(EligibilityCheck check) throws Exception;
+
+    /* The published document id a check would get at the given version. */
+    String getPublishedId(EligibilityCheck check, String version);
 }

builder-api/src/main/java/org/acme/persistence/impl/EligibilityCheckRepositoryImpl.java

@@ -10,6 +10,7 @@
 import org.acme.constants.CollectionNames;
 import org.acme.constants.FieldNames;
 import org.acme.model.domain.Benefit;
+import org.acme.model.domain.CheckVersion;
 import org.acme.model.domain.EligibilityCheck;
 import org.acme.persistence.EligibilityCheckRepository;
 import org.acme.persistence.FirestoreUtils;
@@ -55,33 +56,11 @@ public List<EligibilityCheck> getLatestVersionPublishedCustomChecks(String userI
             .collect(java.util.stream.Collectors.toMap(
                 check -> getPublishedPrefix(check),
                 check -> check,
-                (check1, check2) -> compareVersions(check1.getVersion(), check2.getVersion()) > 0 ? check1 : check2
+                (check1, check2) -> CheckVersion.compare(check1.getVersion(), check2.getVersion()) > 0 ? check1 : check2
             ));
         return new ArrayList<>(latestVersionMap.values());
     }
 
-    private  static int compareVersions(String v1, String v2) {
-        int[] a = normalize(v1);
-        int[] b = normalize(v2);
-
-        for (int i = 0; i < 3; i++) {
-            if (a[i] != b[i]) {
-                return a[i] - b[i];
-            }
-        }
-        return 0;
-    }
-
-    private static int[] normalize(String version) {
-        String[] parts = version.split("\\.");
-        int[] nums = new int[] {0, 0, 0};
-
-        for (int i = 0; i < parts.length && i < 3; i++) {
-            nums[i] = Integer.parseInt(parts[i]);
-        }
-        return nums;
-    }
-
     public List<EligibilityCheck> getPublishedCheckVersions(EligibilityCheck workingCustomCheck) throws Exception {
         Map<String, String> fieldValues = Map.of(
             "ownerId", workingCustomCheck.getOwnerId(),
@@ -198,6 +177,10 @@ public String getPublishedPrefix(EligibilityCheck check) {
     }
 
     public String getPublishedId(EligibilityCheck check) {
-        return getPublishedPrefix(check) + "-" + check.getVersion().toString();
+        return getPublishedId(check, check.getVersion());
+    }
+
+    public String getPublishedId(EligibilityCheck check, String version) {
+        return getPublishedPrefix(check) + "-" + version;
     }
 }

builder-api/src/test/java/org/acme/controller/EligibilityCheckResourceTest.java

@@ -32,6 +32,7 @@ class EligibilityCheckResourceTest {
 
     private static final String USER_ID = "owner-1";
     private static final String CHECK_ID = "check-1";
+    private static final String PUBLISHED_PREFIX = "P-" + USER_ID + "-my-module-my-check";
 
     private final EligibilityCheckResource resource = new EligibilityCheckResource();
     private final EligibilityCheckRepository repository = mock(EligibilityCheckRepository.class);
@@ -66,6 +67,8 @@ void setUp() throws Exception {
             EligibilityCheck check = invocation.getArgument(0);
             return "W-" + check.getOwnerId() + "-" + check.getModule() + "-" + check.getName();
         });
+        when(repository.getPublishedId(any(), anyString()))
+                .thenAnswer(invocation -> PUBLISHED_PREFIX + "-" + invocation.<String>getArgument(1));
     }
 
     @Test
@@ -218,6 +221,81 @@ void staleWorkingVersionDoesNotReusePublishedVersion() throws Exception {
         assertEquals("3.0.0", capturedPublishedVersion());
     }
 
+    @Test
+    void malformedPublishedVersionsAreIgnored() throws Exception {
+        workingCheck.setVersion("2.0.0");
+        when(repository.getPublishedCheckVersions(workingCheck))
+                .thenReturn(List.of(
+                        publishedVersion(""),
+                        publishedVersion("not-a-version"),
+                        publishedVersion("1.invalid.0"),
+                        publishedVersion("2.0.0")
+                ));
+
+        Response response = resource.publishCustomCheck(identity, CHECK_ID);
+
+        assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+        assertEquals("3.0.0", capturedPublishedVersion());
+    }
+
+    // Dot-only strings split into no parts at all, so they must not be read as version 0.0.0
+    @Test
+    void dotOnlyPublishedVersionsAreIgnored() throws Exception {
+        workingCheck.setVersion("2.0.0");
+        when(repository.getPublishedCheckVersions(workingCheck))
+                .thenReturn(List.of(publishedVersion("."), publishedVersion("...")));
+
+        Response response = resource.publishCustomCheck(identity, CHECK_ID);
+
+        assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+        assertEquals("2.0.0", capturedPublishedVersion());
+    }
+
+    /* Ignoring a corrupt version must not hand back a version whose published id is already taken:
+       that document could never be written, so publishing would fail for good. */
+    @Test
+    void publishSkipsVersionsWhosePublishedIdAlreadyExists() throws Exception {
+        workingCheck.setVersion("2.0.0");
+        when(repository.getPublishedCheckVersions(workingCheck))
+                .thenReturn(List.of(
+                        publishedVersion("1.0.0"),
+                        publishedVersion("v2.0.0", PUBLISHED_PREFIX + "-2.0.0")
+                ));
+
+        Response response = resource.publishCustomCheck(identity, CHECK_ID);
+
+        assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+        assertEquals("3.0.0", capturedPublishedVersion());
+    }
+
+    @Test
+    void publishKeepsSkippingUntilAnUnusedVersionIsFound() throws Exception {
+        workingCheck.setVersion("1.0.0");
+        when(repository.getPublishedCheckVersions(workingCheck))
+                .thenReturn(List.of(
+                        publishedVersion("1.0.0"),
+                        publishedVersion("v2.0.0", PUBLISHED_PREFIX + "-2.0.0"),
+                        publishedVersion("v3.0.0", PUBLISHED_PREFIX + "-3.0.0")
+                ));
+
+        Response response = resource.publishCustomCheck(identity, CHECK_ID);
+
+        assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+        assertEquals("4.0.0", capturedPublishedVersion());
+    }
+
+    // A corrupt working version cannot be published as-is, or the next publish inherits the corruption
+    @Test
+    void firstPublishOfACorruptWorkingVersionStartsAtTheInitialVersion() throws Exception {
+        workingCheck.setVersion("not-a-version");
+        when(repository.getPublishedCheckVersions(workingCheck)).thenReturn(List.of());
+
+        Response response = resource.publishCustomCheck(identity, CHECK_ID);
+
+        assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
+        assertEquals("1.0.0", capturedPublishedVersion());
+    }
+
     // An unreadable version list must not be mistaken for "never published"
     @Test
     void publishFailsWhenPublishedVersionsCannotBeRead() throws Exception {
@@ -232,8 +310,14 @@ void publishFailsWhenPublishedVersionsCannotBeRead() throws Exception {
     }
 
     private EligibilityCheck publishedVersion(String version) {
+        return publishedVersion(version, PUBLISHED_PREFIX + "-" + version);
+    }
+
+    /* The id is fixed when a version is published, so a version field corrupted later no longer matches it */
+    private EligibilityCheck publishedVersion(String version, String id) {
         EligibilityCheck published = new EligibilityCheck("my-check", "my-module", "a check", List.of(), USER_ID);
         published.setVersion(version);
+        published.setId(id);
         return published;
     }