From cdf0e5e061497f0dd72c26616ea8f29565c11d3c Mon Sep 17 00:00:00 2001 From: Devesh Bhardwaj Date: Mon, 24 Aug 2026 17:16:52 +0530 Subject: [PATCH 1/3] SK-3061: close tokenize patch-coverage gaps flagged by codecov on #420 Traced each flagged line in BulkTokenizeResponse.java and Utils.java via jacoco rather than guessing, and added targeted tests for the ones that were real, in-scope gaps: - BulkTokenizeResponse.buildSummary()/getRecordsToRetry(): the no-payload fallback branch (records present, originalPayload null - reachable via the public 2-arg constructor) was entirely untested, including the partial-outcome and null+null cases. - Utils.tokenizeRecordsFromErrorBody(): 'response' present-but-empty, present-but-explicitly-null, and unparseable-shape all fall back to the status-code summary but none were tested. - Utils.groupTokenizeRows(): the null-batch and empty-(non-null)-batch fallback (one record per row, no correlation possible) was untested. - Utils.handleBulkTokenizeBatchException()/extractBatchErrorMessage(): an explicitly-empty (non-null) token group list, and the nested {'error': {...}} object's 'error'-over-'message' preference and no-string-found fallback, were untested. - Utils.formatBulkTokenizeResponse(null, ...) was untested. Left alone, deliberately: - VaultController.java's uncovered lines are all from #412's exception handling (bulkInsert/bulkInsertAsync), not from the tokenize work - out of scope here. - Utils.java lines in formatBulkDeleteTokensResponse()/isFailedRecord() are DeleteTokens code, not tokenize. - buildTokenizeResponseRecord()'s '.get() != null' check on an Optional already known to be present is structurally unreachable-false per Optional's own contract - not a real gap, can't be forced by a test. --- .../utils/FlatTokenizeResponseTests.java | 71 +++++++++++++++++++ .../java/com/skyflow/utils/UtilsTests.java | 52 ++++++++++++++ .../vault/data/BulkRetryAndSummaryTests.java | 35 +++++++++ 3 files changed, 158 insertions(+) diff --git a/flowvault/src/test/java/com/skyflow/utils/FlatTokenizeResponseTests.java b/flowvault/src/test/java/com/skyflow/utils/FlatTokenizeResponseTests.java index c809063d..4d069d05 100644 --- a/flowvault/src/test/java/com/skyflow/utils/FlatTokenizeResponseTests.java +++ b/flowvault/src/test/java/com/skyflow/utils/FlatTokenizeResponseTests.java @@ -338,6 +338,77 @@ public void testRejectedRequest_withUnfamiliarBodyFallsBackToTheStatusCode() { Assert.assertEquals(Integer.valueOf(504), records.get(0).getHttpCode()); } + @Test + public void testRejectedRequest_emptyResponseArrayFallsBackToTheStatusCode() { + // "response" is present but empty - nothing to rebuild from, so fall back like a body + // without a response array at all + List sent = Collections.singletonList(record("v1", "g1")); + Map body = new LinkedHashMap<>(); + body.put("response", new ArrayList<>()); + + List records = + Utils.handleBulkTokenizeBatchException(rejected(500, body), sent, 0); + + Assert.assertEquals(1, records.size()); + Assert.assertEquals(Integer.valueOf(500), records.get(0).getHttpCode()); + } + + @Test + public void testRejectedRequest_explicitNullResponseArrayFallsBackToTheStatusCode() { + // "response" is present in the map but its value is JSON null, not an array - deserialises + // to an absent Optional rather than an empty one + List sent = Collections.singletonList(record("v1", "g1")); + Map body = new LinkedHashMap<>(); + body.put("response", null); + + List records = + Utils.handleBulkTokenizeBatchException(rejected(500, body), sent, 0); + + Assert.assertEquals(1, records.size()); + Assert.assertEquals(Integer.valueOf(500), records.get(0).getHttpCode()); + } + + @Test + public void testRejectedRequest_unparseableResponseArrayFallsBackToTheStatusCode() { + // "response" is present but the wrong shape to deserialise - must not propagate the crash + List sent = Collections.singletonList(record("v1", "g1")); + Map body = new LinkedHashMap<>(); + body.put("response", "not-an-array"); + + List records = + Utils.handleBulkTokenizeBatchException(rejected(500, body), sent, 0); + + Assert.assertEquals(1, records.size()); + Assert.assertEquals(Integer.valueOf(500), records.get(0).getHttpCode()); + } + + @Test + public void testRejectedRequest_nullBatchWithPerRowBodyStillRebuildsFromTheRows() { + // defensive: a null batch can't be correlated against, but a per-row body still has + // everything needed to report each row directly + Throwable ex = rejected(400, body(row("v1", "g1", "", "bad group", 400))); + + List records = + Utils.handleBulkTokenizeBatchException(ex, null, 0); + + Assert.assertEquals(1, records.size()); + Assert.assertEquals(0, records.get(0).getIndex()); + Assert.assertEquals("bad group", records.get(0).getError()); + } + + @Test + public void testRejectedRequest_emptyBatchWithPerRowBodyStillRebuildsFromTheRows() { + // same as a null batch - an empty one can't be correlated against either + Throwable ex = rejected(400, body(row("v1", "g1", "", "bad group", 400))); + + List records = + Utils.handleBulkTokenizeBatchException(ex, new ArrayList<>(), 0); + + Assert.assertEquals(1, records.size()); + Assert.assertEquals(0, records.get(0).getIndex()); + Assert.assertEquals("bad group", records.get(0).getError()); + } + @Test public void testRejectedRequest_retryableStatusStillSurfacesForRetry() { List sent = Collections.singletonList(record("v1", "g1")); diff --git a/flowvault/src/test/java/com/skyflow/utils/UtilsTests.java b/flowvault/src/test/java/com/skyflow/utils/UtilsTests.java index 55f01a66..3d0ab81d 100644 --- a/flowvault/src/test/java/com/skyflow/utils/UtilsTests.java +++ b/flowvault/src/test/java/com/skyflow/utils/UtilsTests.java @@ -1650,6 +1650,39 @@ public void testHandleBulkTokenizeBatchException_errorFieldAsObjectUsesStructure Assert.assertEquals(Integer.valueOf(404), errors.get(0).getHttpCode()); } + @Test + public void testHandleBulkTokenizeBatchException_errorFieldAsObjectPrefersNestedErrorOverMessage() { + // extractBatchErrorMessage prefers a nested "error" key over "message" when both are present + Map errorObject = new HashMap<>(); + errorObject.put("error", "nested error message"); + errorObject.put("message", "vault not found"); + Map body = new HashMap<>(); + body.put("error", errorObject); + ApiClientApiException apiEx = new ApiClientApiException("tokenize failed", 404, body); + RuntimeException wrapper = new RuntimeException(apiEx); + + List errors = Utils.handleBulkTokenizeBatchException( + wrapper, tokenizeBatch("v1", "group1"), 0); + + Assert.assertEquals("nested error message", errors.get(0).getError()); + } + + @Test + public void testHandleBulkTokenizeBatchException_errorFieldAsObjectWithoutAStringFallsBackToApiMessage() { + // neither "error" nor "message" is a String, so there is nothing usable to read out of it + Map errorObject = new HashMap<>(); + errorObject.put("message", Collections.singletonList("not a string")); + Map body = new HashMap<>(); + body.put("error", errorObject); + ApiClientApiException apiEx = new ApiClientApiException("tokenize failed", 404, body); + RuntimeException wrapper = new RuntimeException(apiEx); + + List errors = Utils.handleBulkTokenizeBatchException( + wrapper, tokenizeBatch("v1", "group1"), 0); + + Assert.assertEquals("tokenize failed", errors.get(0).getError()); + } + @Test public void testHandleBulkTokenizeBatchException_nonMapBodyUsesApiMessage() { // Body is not a map, so extractBatchErrorMessage falls back to the exception's own message. @@ -1673,6 +1706,19 @@ public void testHandleBulkTokenizeBatchException_nullBatchReturnsEmpty() { Assert.assertTrue(errors.isEmpty()); } + @Test + public void testHandleBulkTokenizeBatchException_emptyGroupListStillReportsOneEntry() { + // an explicitly empty token group list, not a null one, must be treated the same way + RuntimeException ex = new RuntimeException("boom"); + List batch = Collections.singletonList( + BulkTokenizeRequestRecord.builder().value("v1").tokenGroupNames(new ArrayList<>()).build()); + + List errors = Utils.handleBulkTokenizeBatchException(ex, batch, 0); + + Assert.assertEquals(1, errors.size()); + Assert.assertNull(errors.get(0).getTokenGroupName()); + } + // ── formatBulkInsertResponse ─────────────────────────────────────────────── @Test @@ -1979,6 +2025,12 @@ public void testFormatBulkTokenizeResponse_emptyResponseReturnsNull() { tokenizeBatch("value1", "group1"), 0, new HashMap<>())); } + @Test + public void testFormatBulkTokenizeResponse_nullResponseReturnsNull() { + Assert.assertNull(Utils.formatBulkTokenizeResponse( + null, tokenizeBatch("value1", "group1"), 0, new HashMap<>())); + } + // Tests for getQueryRequestBody / buildQueryResponse / getGetRequestBody / buildGetResponse // were removed: get and query Utils helpers no longer exist (bulk-only module). diff --git a/flowvault/src/test/java/com/skyflow/vault/data/BulkRetryAndSummaryTests.java b/flowvault/src/test/java/com/skyflow/vault/data/BulkRetryAndSummaryTests.java index eb118a3a..968fe45a 100644 --- a/flowvault/src/test/java/com/skyflow/vault/data/BulkRetryAndSummaryTests.java +++ b/flowvault/src/test/java/com/skyflow/vault/data/BulkRetryAndSummaryTests.java @@ -108,8 +108,43 @@ public void testTokenizeSummary_isNullWhenNoPayloadWasSupplied() { Assert.assertNull(new BulkTokenizeResponse(new ArrayList<>()).getSummary()); } + @Test + public void testTokenizeSummary_classifiesByRowsWhenNoPayloadIsGiven() { + // The two-arg constructor can still be called with a null payload; classification then + // falls back to the indexes actually present in records instead of the submitted list. + List records = Arrays.asList( + row(0, "g1", "t1", 200, null), + row(1, "g1", null, 400, "bad group"), + row(2, "g1", "t2", 200, null), + row(2, "g2", null, 400, "bad group")); + + TokenizeSummary summary = new BulkTokenizeResponse(records, null).getSummary(); + + // without a submitted payload to count values from, totalTokens falls back to the row count + Assert.assertEquals(4, summary.getTotalTokens()); + Assert.assertEquals(1, summary.getTotalTokenized()); + Assert.assertEquals(1, summary.getTotalPartial()); + Assert.assertEquals(1, summary.getTotalFailed()); + } + + @Test + public void testTokenizeSummary_nullRecordsAndNullPayloadYieldsZeroes() { + TokenizeSummary summary = new BulkTokenizeResponse(null, null).getSummary(); + + Assert.assertEquals(0, summary.getTotalTokens()); + Assert.assertEquals(0, summary.getTotalTokenized()); + Assert.assertEquals(0, summary.getTotalPartial()); + Assert.assertEquals(0, summary.getTotalFailed()); + } + // ── BulkTokenizeResponse.getRecordsToRetry ──────────────────────────────── + @Test + public void testTokenizeRetry_withNullRecordsReturnsEmpty() { + Assert.assertTrue(new BulkTokenizeResponse(null, Collections.singletonList(requestRecord("a"))) + .getRecordsToRetry().isEmpty()); + } + @Test public void testTokenizeRetry_only5xxFailuresAreReturned() { List records = Arrays.asList( From b7eb1774a4f3cca12d8ecc5625a3a8da65729246 Mon Sep 17 00:00:00 2001 From: Devesh Bhardwaj Date: Mon, 24 Aug 2026 17:19:54 +0530 Subject: [PATCH 2/3] chore: whitelist deserialises in cspell word list Same British-spelling pattern already whitelisted for serialise/ serialises/deserialise/deserialised; deserialises (third-person form) was missing, flagged from a comment added in this branch's coverage-gap tests. --- .cspell.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.cspell.json b/.cspell.json index 4c434d07..a2f78d62 100644 --- a/.cspell.json +++ b/.cspell.json @@ -108,6 +108,7 @@ "synthesised", "deserialise", "deserialised", + "deserialises", "unmodelled", "recordss", "rarr", From 562f46e484a9b9b65b533afa12f9d97e7431063f Mon Sep 17 00:00:00 2001 From: Devesh Bhardwaj Date: Mon, 24 Aug 2026 17:23:41 +0530 Subject: [PATCH 3/3] chore: whitelist unparseable in cspell word list --- .cspell.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.cspell.json b/.cspell.json index a2f78d62..bda277c3 100644 --- a/.cspell.json +++ b/.cspell.json @@ -109,6 +109,7 @@ "deserialise", "deserialised", "deserialises", + "unparseable", "unmodelled", "recordss", "rarr",