diff --git a/builder-api/src/main/java/org/acme/controller/EligibilityCheckResource.java b/builder-api/src/main/java/org/acme/controller/EligibilityCheckResource.java index f57bc902..0b1cf342 100644 --- a/builder-api/src/main/java/org/acme/controller/EligibilityCheckResource.java +++ b/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 VERSION_ORDER = Comparator - .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 publishedChecks) { + Set 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 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 parsePublishedVersion(String version) { + Optional parsed = CheckVersion.parse(version); + if (parsed.isEmpty()) { + Log.warn("Ignoring malformed published check version: " + version); } - return nums; + return parsed; } } diff --git a/builder-api/src/main/java/org/acme/model/domain/CheckVersion.java b/builder-api/src/main/java/org/acme/model/domain/CheckVersion.java new file mode 100644 index 00000000..4d45b5a0 --- /dev/null +++ b/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 ORDER = Comparator + .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 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 parsed1 = parse(version1); + Optional 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]; + } +} diff --git a/builder-api/src/main/java/org/acme/persistence/EligibilityCheckRepository.java b/builder-api/src/main/java/org/acme/persistence/EligibilityCheckRepository.java index 7329f02a..b15ef1cd 100644 --- a/builder-api/src/main/java/org/acme/persistence/EligibilityCheckRepository.java +++ b/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); } diff --git a/builder-api/src/main/java/org/acme/persistence/impl/EligibilityCheckRepositoryImpl.java b/builder-api/src/main/java/org/acme/persistence/impl/EligibilityCheckRepositoryImpl.java index 94c7d754..fb65769d 100644 --- a/builder-api/src/main/java/org/acme/persistence/impl/EligibilityCheckRepositoryImpl.java +++ b/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 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 getPublishedCheckVersions(EligibilityCheck workingCustomCheck) throws Exception { Map 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; } } diff --git a/builder-api/src/test/java/org/acme/controller/EligibilityCheckResourceTest.java b/builder-api/src/test/java/org/acme/controller/EligibilityCheckResourceTest.java index ef9f8c82..b4e64e7c 100644 --- a/builder-api/src/test/java/org/acme/controller/EligibilityCheckResourceTest.java +++ b/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.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; } diff --git a/builder-api/src/test/java/org/acme/model/domain/CheckVersionTest.java b/builder-api/src/test/java/org/acme/model/domain/CheckVersionTest.java new file mode 100644 index 00000000..e65f9663 --- /dev/null +++ b/builder-api/src/test/java/org/acme/model/domain/CheckVersionTest.java @@ -0,0 +1,67 @@ +package org.acme.model.domain; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.List; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class CheckVersionTest { + + @Test + void parsesADottedVersion() { + assertArrayEquals(new int[]{2, 3, 4}, CheckVersion.parse("2.3.4").orElseThrow()); + } + + @Test + void treatsMissingPartsAsZero() { + assertArrayEquals(new int[]{2, 0, 0}, CheckVersion.parse("2").orElseThrow()); + assertArrayEquals(new int[]{2, 3, 0}, CheckVersion.parse("2.3").orElseThrow()); + } + + /* Stored versions are free-form strings, so anything at all can come back from Firestore */ + @ParameterizedTest + @ValueSource(strings = {"", " ", ".", "..", "...", ".1", "not-a-version", "v2.0.0", "1.invalid.0", + "1.2.3.4", "-1.0.0", "1.0.0-rc1", "99999999999999"}) + void rejectsAnythingThatIsNotAVersion(String version) { + assertTrue(CheckVersion.parse(version).isEmpty(), "expected \"" + version + "\" to be rejected"); + } + + @Test + void rejectsAMissingVersion() { + assertEquals(Optional.empty(), CheckVersion.parse(null)); + } + + @Test + void ordersVersionsByEachPart() { + assertTrue(CheckVersion.compare("2.0.0", "1.9.9") > 0); + assertTrue(CheckVersion.compare("1.2.3", "1.3.0") < 0); + assertEquals(0, CheckVersion.compare("1.2.3", "1.2.3")); + } + + /* An unreadable version must not crash the comparison it takes part in */ + @Test + void ordersUnreadableVersionsBelowReadableOnes() { + assertTrue(CheckVersion.compare("1.0.0", "v2.0.0") > 0); + assertTrue(CheckVersion.compare("v2.0.0", "1.0.0") < 0); + assertEquals(0, CheckVersion.compare("v2.0.0", "also-bad")); + assertEquals(0, CheckVersion.compare(null, null)); + } + + @Test + void incrementsTheMajorPartAndResetsTheRest() { + assertArrayEquals(new int[]{3, 0, 0}, CheckVersion.nextMajor(new int[]{2, 4, 6})); + assertEquals("3.0.0", CheckVersion.format(CheckVersion.nextMajor(new int[]{2, 4, 6}))); + } + + @Test + void startsAtTheVersionNewChecksAreCreatedWith() { + EligibilityCheck newCheck = new EligibilityCheck("a-check", "a-module", "", List.of(), "owner-1"); + assertEquals(newCheck.getVersion(), CheckVersion.format(CheckVersion.initial())); + } +}