SK-3061: make every bulk operation's README section iterate the response - #411
Merged
Devesh-Skyflow merged 1 commit intoAug 18, 2026
Conversation
…he response The snippet added when getTokens() was made typed (#408) used `record` without ever introducing it - it reads as if it stood alone, but record only exists inside a loop over insertResponse.getRecords(). Wrapped it in that outer loop (plus a null check, since getTokens() is null on a failed record), matching the pattern the Bulk Detokenize section's equivalent metadata snippet already follows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## flowvault-release/26.8.13 #411 +/- ##
============================================================
Coverage ? 91.37%
Complexity ? 28
============================================================
Files ? 158
Lines ? 6448
Branches ? 861
============================================================
Hits ? 5892
Misses ? 362
Partials ? 194
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This was referenced Aug 18, 2026
Merged
Devesh-Skyflow
added a commit
that referenced
this pull request
Aug 24, 2026
* SK-3061: revert fields→tokens rename, restore data field on bulkInsert response (#408) * SK-3061: revert fields->tokens rename, restore data field on bulkInsert response The flowvault SDK renamed the API's `tokens` response key to `fields`, so callers had to use getFields() instead of the API-matching getTokens(). Separately, the API's `data` field was silently dropped from the response entirely. Both were flagged in Slack by a customer comparing SDK output to the raw API contract. - InsertResponseRecord/BulkInsertResponseRecord: add tokens/getTokens() matching the API. getFields() stays as a @deprecated alias that logs a warning and delegates to getTokens() - existing callers keep working unchanged. - Add data/getData(), wired from V1RecordResponseObject.getData() in Utils.formatBulkInsertResponse (the wire type already carried it; nothing read it). - Old constructor overloads (without `data`) are kept, also @deprecated, rather than changing existing constructor signatures - this avoids a binary/source-incompatible change against the japicmp baseline in flowvault/pom.xml. - detokenize/deleteTokens are untouched; they never had this issue. - Updated README and tests accordingly, including a dedicated test for the deprecated constructor + getFields() alias. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: fix flowvault README inaccuracies, drop misleading sample, add response iteration to samples README (flowvault/README.md): - Version snippets said 1.0.0; pom.xml is 1.0.1. - CustomHeaderKey enum names were wrong (PascalCase vs actual SCREAMING_SNAKE_CASE) - the sample code block did not compile. - "vault() takes no arguments, use one client per vault" was false: Skyflow.vault(String vaultId) exists and is tested for multi-vault use on one client. Documented it. - "updateType accepts UPDATE (the default)" overstated what the SDK does - it omits the field when unset rather than sending "UPDATE"; reworded to say so. - getHttpStatus() example used "BAD_REQUEST"; the actual hardcoded validation-error string is "Bad Request". Samples: - Deleted BearerTokenExpiryExample.java: despite its name, it never touches BearerToken or Token.isExpired() at all - it's a generic "retry once on 401" wrapper around bulkDetokenize, redundant with both BearerTokenGenerationExample's real expiry-check pattern and the README's own retry guidance. - Rewrote samples/README.md: it referenced DetokenizeExample.java, InsertExample.java, GetByIdExample.java etc. - files that don't exist anywhere in this module (leftover boilerplate from a different samples layout). Replaced with an accurate index of the actual serviceaccount/ and vault/ samples plus correct Maven run instructions. - Added response iteration (summary + per-record/per-token walk + retry) to BulkInsertSync/Async, BulkMultiTableInsertSync/Async, BulkDetokenizeSync/Async, and CustomHeaderExample - they previously only printed the raw response object. BulkTokenizeSync/Async and BulkDeleteTokensSync/Async already had this and are untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: fix inaccurate JSON response examples in flowvault README - Bulk Insert: tokens example showed a flat string per column ("card_number": "5484-..."), but the value is always a LIST of {token, tokenGroupName} entries - one per token group configured on that column, even when there's only one. This is the exact shape a dedicated regression test (testBulkInsert_successWithListOfMapsTokenShape) guards, and it's what the API's own generic Object typing is for. Added a populated hashedData example and a code snippet showing how to read a tokens entry, since there's no typed accessor for it yet. - Bulk Detokenize: metadata example showed {} on a successful record, but metadata normally carries skyflowId/tableName on success (per the wire type's own Javadoc and the key-rename Utils.java performs on it), and the real "nothing there" case is null, not {}. Field names/order for all four response record types (verified against InsertResponseRecord/BulkInsertResponseRecord, TokenizeResponseRecord/ TokenizeResponseToken, DetokenizeResponseRecord/BaseDetokenizeRecordResponse, DeleteTokensRecord) and the requestId null-on-success/populated-on-error-only behavior were already correct - no changes needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: document schema vs. schemaless vault support per operation This was flagged in the Slack thread (Saketh's clarification point 1: "We have the SDK interface to schema/schemaless vault mapping already but we haven't added it in the readme, we will add it") and was still missing - grepping the README for "schema" turned up nothing. Added a table documenting which of the four bulk operations apply to which vault type, matching Devesh's original clarification in the thread: insert is structured/schema-only, tokenize and deleteTokens are schemaless-only (confirmed by git history - SK-2646 shipped them specifically as "Schemaless vault apis"), and detokenize works with both since it only needs the token itself, not a table. Verified this isn't enforced anywhere in Validations.java, so worded it as supported/intended usage rather than something the SDK validates or blocks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: cover InsertResponseRecord's deprecated constructor directly Codecov flagged 2 uncovered lines in InsertResponseRecord.java. The existing deprecated-constructor test only goes through BulkInsertResponseRecord's deprecated 8-arg constructor, which delegates straight to the new 9-arg constructor -> new 7-arg super constructor, never touching InsertResponseRecord's own deprecated 6-arg constructor. Nothing else in the codebase constructs InsertResponseRecord directly (it's only ever used via the Bulk subclass), so that constructor was genuinely untested. Added a test that instantiates it directly and asserts data defaults to null. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Revert unsubstantiated metadata content claim in flowvault README The previous commit asserted metadata "typically carries skyflowId/ tableName" with equal confidence for both keys. Re-checking: skyflowId is grounded in real code (Utils.java renames a skyflowID key to skyflowId when present, which only exists because that key is known to show up), but tableName is only mentioned in one line of Javadoc on the generated wire type - no transform logic touches it and no test in the suite constructs or asserts a tableName key anywhere in metadata. That's a docstring example, not a verified contract. Reverting the example and the claim rather than asserting a shape I can't actually back up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: repeat vault-type applicability inline on each bulk operation section The "Schema vs. schemaless vaults" table lives under "VaultController - Bulk operations", but a reader jumping straight to e.g. "# Bulk Tokenize" via the TOC or a search never sees it. Added a one-line "Vault type supported" note at the top of each of the four operation sections (Insert/Tokenize/Detokenize/Delete Tokens), linking back to the consolidated table for the full picture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: trim vault-type notes to match the terse delete-tokens style Insert/Tokenize/Detokenize repeated the explanation already in the consolidated table; shortened to one line each, consistent with the delete-tokens note. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: allowlist "codehaus" for cspell CI spellcheck flagged flowvault/samples/README.md:38 - "codehaus" from the org.codehaus.mojo:exec-maven-plugin groupId in the sample run instructions. Legitimate Maven groupId, not a typo; added alongside the other domain-specific terms already in the word list (jfrog, sonatype, etc.). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: fix Skyflow.getVaultConfig() crashing on an empty vault list Skyflow.getVaultConfig() did: Object[] array = this.builder.vaultConfigMap.values().toArray(); return (VaultConfig) array[0]; - an unguarded array[0] access. A client built without any addVaultConfig(...) call (build() never validates this) throws ArrayIndexOutOfBoundsException instead of failing predictably. Traced every call site of "getVaultConfig()" in the codebase first: all of them are on VaultController (which has its own, unrelated, already-safe getVaultConfig() returning its single stored config - no lookup involved), never on Skyflow directly. So this method had zero usages and zero test coverage anywhere in the suite. Fixed by mirroring the sibling method's established, already-correct convention: BaseSkyflow.getVaultConfig(String) is a plain vaultConfigMap.get(vaultId), returning null when absent - no exception, no signature change. Rewrote the no-arg overload the same way (.stream().findFirst().orElse(null)) rather than making it throw SkyflowException like vault()/vault(String) do, since that would diverge from its own sibling's contract and require adding a checked exception to the signature - a source-incompatible change under this module's japicmp gate for no real benefit. This fix needs no baseline update: same signature, implementation-only. Added 7 tests covering: single vault, first-of-several (consistent with vault()'s "first" semantics, and identity-matched against getVaultConfig(id)), the empty-client regression case itself, null after removing the only vault, falling back to the remaining vault after the first is removed, and the two existing gaps on the by-id overload (unknown id, empty client). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: document detokenize metadata shape, now grounded in the proto Previously backed out a claim about metadata's typical content since only skyflowId was grounded (a rename in Utils.java) and tableName was just a Javadoc description with no code or test behind it. flowdb_dp_apis.proto's metadata field has its own literal example value, not just a free-text description: {"table": "table1", "skyflowID": "4524524534623"}. That's stronger evidence than the description text, and it reveals the description itself is misleading - it says "such as tableName or skyflowID" but the actual example key is "table", not "tableName". Utils.java only renames "skyflowID" to "skyflowId"; "table" passes through unrenamed. Documented the real shape and explained the rename explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: add typed Token accessor for insert response tokens Adds getTokenDetails() to InsertResponseRecord (inherited by BulkInsertResponseRecord) alongside the existing generic getTokens(): Map<String, Object>. It parses the same data into Map<String, List<Token>>, so callers get getToken()/getTokenGroupName() instead of casting Map entries by hand - the exact "known gap" flagged in the Slack thread and the earlier README note. Token is a straight reintroduction of flowvault's own pre-rework class (deleted in 685a82f when insert was reworked around the current InsertResponseRecord/tokens map), not a new invention - same shape the user specified, with final fields and a toString() added to match TokenizeResponseToken's established convention in this package. getTokenDetails() is purely additive (new method, no signature changes to anything existing) and computed fresh from getTokens() on every call rather than stored separately, so the generic and typed views can never disagree. It normalizes every shape getTokens()'s value is known to take - a list of {token, tokenGroupName} entries, a single such entry not wrapped in a list, or a bare token value with no group info - into a consistent List<Token>, returning null only when getTokens() itself is null. Fixed a real landmine along the way: ResponseComponentTests.java had a dangling {@link Token} javadoc reference left over from the class's removal - harmless while there was no Token class to resolve to, but it would have silently started resolving to this new class instead (with the surrounding comment still saying it "was removed"). Updated the comment to describe what actually happened. Added 9 tests covering Token itself and every shape getTokenDetails() normalizes, including one against BulkInsertResponseRecord directly (not just the base class) to confirm the inherited method works for the type callers actually receive. Updated the README's Bulk Insert section to document getTokenDetails() in place of the manual cast-it-yourself snippet from the previous commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: use typed Token accessor in samples, cover its parsing branches Two changes landed together since both were already staged: 1. BulkInsertSync/Async, BulkMultiTableInsertSync/Async, and CustomHeaderExample previously printed record.getTokens() (the raw Map<String, Object>) directly. Updated all five to walk getTokenDetails() instead - a nested loop over Map<String, List<Token>>, printing token.getTokenGroupName()/ token.getToken() per column, matching the pattern BulkTokenizeSync.java already uses for its own typed per-token loop. 2. 3 more tests, closing every branch the previous commit's 9 tests left untouched in InsertResponseRecord's new parsing logic: - parseTokenEntries's own null check (a column present in the map with a null value, as opposed to the whole tokens map being null, which was already covered) - that column is now omitted from getTokenDetails() rather than appearing with a null/empty entry. - toToken's final `return null` and the corresponding "skip adding" branch in parseTokenEntries's list loop - a null element sitting inside a column's token-group list, which the loop must skip rather than NPE on. - The tokenGroupName-absent ternary branch in toToken's Map-entry parsing - every existing Map-entry test populated both "token" and "tokenGroupName" keys, so the "key missing" path was never exercised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: make InsertResponseRecord.tokens typed (Map<String, List<Token>>), matching v3 Per explicit direction: retype tokens/getTokens() itself to Map<String, List<Token>> - matching the pre-rework ("v3") shape - instead of keeping it generic and exposing a separate getTokenDetails() accessor alongside it (the previous commit's approach). BREAKING CHANGE - flagged and confirmed before implementing: - getTokens()'s return type changes (Map<String,Object> -> Map<String,List<Token>>). - Both InsertResponseRecord constructors' `tokens` parameter type changes for the same reason. This isn't a choice - Map<String,Object> and Map<String,List<Token>> erase to the same raw `Map` type, so Java forbids two constructor overloads at the same arity that differ only in that generic parameter. There is no way to add this as a new overload alongside the old one; the parameter type has to change in place. Same for BulkInsertResponseRecord's two constructors. - getFields() (deprecated alias) now returns the typed map too, since it just delegates to getTokens(). This diverges from "keep contract exactly like API" (the API's own wire contract types a column's tokens as generic Object) - accepted as the tradeoff for matching v3's typed shape. Moved the parsing logic (a column's raw value can be a list of {token, tokenGroupName} entries, a single such entry, or a bare value) from InsertResponseRecord into a new static utility, Token.parseTokens(Map<String, Object>): Map<String, List<Token>>. Utils.formatBulkInsertResponse now calls it to convert the wire type's raw tokens map before constructing BulkInsertResponseRecord - parsing happens once at construction time instead of on every getTokens() call. Updated every test that constructed an InsertResponseRecord/ BulkInsertResponseRecord with a raw Map<String,Object> tokens value, or asserted on the old generic return type. The former getTokenDetails()-specific tests now test Token.parseTokens() directly. VaultControllerTests.testBulkInsert_successWithListOfMapsTokenShape simplifies nicely: it no longer needs to cast the parsed result, since getTokens() itself is the typed view now. Updated README and the 5 insert samples (BulkInsertSync/Async, BulkMultiTableInsertSync/Async, CustomHeaderExample) to use getTokens() directly instead of the now-removed getTokenDetails(). Needs a japicmp baseline regeneration (scripts/contract-snapshot- update.sh flowvault) before merge - this is a genuine, intentional break of the existing contract, unlike every other change in this branch so far. I could not run mvn/java in this sandbox to regenerate or verify it myself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: regenerate flowvault japicmp baseline for typed getTokens() Regenerated via scripts/contract-snapshot-update.sh flowvault, now that a JDK/Maven are available to run it. Verified mvn -pl common,flowvault -am verify passes clean (692 tests, japicmp included) against this new baseline. The 3 intentional incompatibilities this baseline now accepts, per the full japicmp diff (flowvault/target/japicmp/default-cli.diff): - InsertResponseRecord's deprecated 6-arg constructor: tokens param generics changed (Map<String,Object> -> Map<String,List<Token>>). - BulkInsertResponseRecord's deprecated 8-arg constructor: same. - getFields(): return type generics changed to match. getTokens() itself was never actually flagged - it's reported as a brand new method against this baseline (the pre-SK-3061 codebase only ever had getFields()), so making it typed was non-breaking on its own. The three real breaks above are avoidable (the deprecated constructors are a different arity than the primary ones, so no erasure conflict forces their parameter type to change; getFields() could reverse-map back to the old shape instead of delegating directly) - flagged and declined in favor of just regenerating the baseline, per explicit direction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-b17c4417 * SK-3061: revert getFields() to Map<String, Object>, keep getTokens() typed (#409) getTokens() itself (item 6, commit 8148504) stays Map<String, List<Token>>. But getFields() delegating straight to it changed its return-type generics from Object to List<Token> - a needless japicmp break for a deprecated alias nobody should be adding new calls to anyway, and one this PR had flagged as avoidable but initially left in favor of just regenerating the baseline. Reverted getFields() to its original Map<String, Object> contract by rendering getTokens()'s typed data back into that raw shape via a new package-private Token.toRawTokens(Map<String, List<Token>>) - the inverse of Token.parseTokens(). Package-private keeps it outside the accessModifier=protected japicmp contract, so it doesn't itself become a compatibility commitment. The round trip is lossless for map-shaped wire input (the normal case) but lossy for the bare-value edge case parseTokens() also handles (a raw "tok-abc" string loses its way back to {"token": "tok-abc", "tokenGroupName": null} - still valid data, just not byte-identical to the original wire shape). Updated every test asserting on getFields() accordingly, and added direct toRawTokens() coverage (null input, single/multiple token groups, round-trip-with-map-input) in ResponseComponentTests. Regenerated flowvault/api-report/skyflow-flowvault-java.baseline.jar via scripts/contract-snapshot-update.sh flowvault - this supersedes the baseline regenerated in f326ec2, since that one still had getFields() returning the typed map. mvn -pl common,flowvault -am verify passes clean against it. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-7f1a295d * SK-3061: fix Bulk Insert README's Token snippet to actually iterate the response (#411) The snippet added when getTokens() was made typed (#408) used `record` without ever introducing it - it reads as if it stood alone, but record only exists inside a loop over insertResponse.getRecords(). Wrapped it in that outer loop (plus a null check, since getTokens() is null on a failed record), matching the pattern the Bulk Detokenize section's equivalent metadata snippet already follows. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-f6c3370a * SK-3061: make DetokenizeResponseRecord.getMetadata() typed (#410) * SK-3061: make DetokenizeResponseRecord.getMetadata() typed metadata was a raw Map<String, Object>, requiring callers to cast into it to reach skyflowId/tableName - the same usability gap Token closed for InsertResponseRecord.getTokens(). Added DetokenizeMetadata (getSkyflowId(), getTableName()) and made getMetadata() itself return it, matching how getTokens() was made typed directly rather than adding a second accessor. DetokenizeMetadata.parseMetadata(Map<String, Object>) normalizes the wire shape - the proto's own literal example uses {"table": ..., "skyflowID": ...}, but Utils.java's existing handling already renamed skyflowID -> skyflowId before this reached the record constructor, so parseMetadata accepts either casing for both keys and moves that normalization out of Utils.java and into one place, alongside the typing. This changes DetokenizeResponseRecord/BulkDetokenizeResponseRecord's metadata constructor parameter and getMetadata()'s return type from Map<String, Object> to DetokenizeMetadata - a japicmp-breaking change, same shape as the earlier getTokens() typing. Regenerated flowvault/api-report/skyflow-flowvault-java.baseline.jar via scripts/contract-snapshot-update.sh flowvault. Updated README's Bulk Detokenize section (JSON sample + a typed-access snippet) and every test constructing a record with a raw metadata map. mvn -pl common,flowvault -am test -> 702 tests, 0 failures. mvn -pl common,flowvault -am verify -> BUILD SUCCESS against the regenerated baseline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: add flowdb/unrenamed to cspell dictionary Flagged by the cspell CI check on PR #410 - both words come from the DetokenizeMetadata Javadoc/test comments referencing flowdb_dp_apis.proto and describing the wire's unrenamed table key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: add matching response-iteration snippets to Bulk Tokenize/Delete Tokens Bulk Insert and Bulk Detokenize's README sections each show a code snippet iterating the response; Bulk Tokenize and Bulk Delete Tokens only had a JSON sample with no matching Java. Added one to each, same pattern: - Bulk Tokenize: outer loop over records, inner loop over each record's tokens (it reports at two levels - see the paragraph directly above). - Bulk Delete Tokens: single loop over records, success/error branch on getError(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-05ce8d07 * SK-3061: fix bulkInsert/bulkInsertAsync's inconsistent exception handling (#412) * SK-3061: make bulkInsertAsync's exception handling consistent with the other bulk async methods bulkInsertAsync only caught ApiClientApiException. bulkDetokenizeAsync, bulkDeleteTokensAsync, and bulkTokenizeAsync all additionally catch SkyflowException (rethrow as-is, so it isn't double-wrapped) and generic Exception (wrapped into SkyflowException) - so any unexpected failure in their synchronous setup still surfaces as SkyflowException, matching the method's declared contract. bulkInsertAsync had neither, so anything thrown from its synchronous setup that wasn't ApiClientApiException (e.g. a caller-supplied RequestInterceptor throwing, since it's invoked synchronously per batch inside insertBatchFutures) leaked out as its raw exception type instead of SkyflowException. Added the same two catch blocks. Batch-level failures inside the CompletableFuture pipeline itself are unaffected - insertBatchFutures already turns those into error-shaped BulkInsertResponse records via .exceptionally(), same as before. Added a regression test that reproduces the leak via a throwing interceptor and confirms it stays fixed (fails without the fix, verified by temporarily reverting it locally before writing this up). mvn -pl common,flowvault -am verify -> 704 tests, 0 failures, BUILD SUCCESS. No japicmp impact - no signature changes, just added catch blocks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: fix the same exception leak in bulkInsert (sync), not just bulkInsertAsync processBulkInsertSync called insertBatchFutures (which invokes the caller's RequestInterceptor synchronously per batch) before its own try started, so an interceptor that threw leaked its raw exception type straight out of bulkInsert() - the sync twin of the bug this PR already fixed on the async side. Every sibling operation's equivalent helper (processBulkDetokenizeSync, processBulkDeleteTokensSync, processBulkTokenizeSync) already makes that call inside its own try; insertBatchFutures itself was never missing anything relative to its siblings - all four *BatchFutures helpers have zero catch blocks of their own, identically. The asymmetry was purely processBulkInsertSync's call-site placement. Moved the insertBatchFutures call inside the try, matching the other three helpers' structure exactly. Added a regression test mirroring testBulkInsertAsync_unexpectedExceptionWrappedAsSkyflowException for the sync path. mvn -pl common,flowvault -am verify -> 705 tests, 0 failures, BUILD SUCCESS. No japicmp impact - no signature changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-291cdd3c * SK-3061: add path to Token; fix stale samples pom.xml version (#413) * SK-3061: add path to Token, for tokens generated from nested/structured columns flowdb_dp_apis.proto's own literal example response for insert includes a "path" key on token entries for a structured column (e.g. an "address" object tokenized per nested field: {"path": "street", "token": "...", "tokenGroupName": "..."}) - present alongside token/tokenGroupName, absent for flat columns. Token.toToken()/parseTokens() silently dropped it. Added Token.getPath() (nullable) and a new 3-arg constructor (Token(token, tokenGroupName, path)); the existing 2-arg constructor delegates to it with path=null, so it stays fully backward compatible - this is purely additive, no existing signature changed. Confirmed via japicmp: BUILD SUCCESS, zero incompatibilities, no baseline regen needed. Token.parseTokens() now reads "path" from each raw entry when present. Token.toRawTokens() (getFields()'s deprecated rendering) only adds a "path" key when the Token actually has one, rather than unconditionally - so a path-less round trip stays exactly as lossless as it was before path existed (confirmed by the existing testToRawTokens_isTheInverseOfParseTokensForMapShapedInput test, which would otherwise have gained a spurious "path": null key it never had). Updated Token's Javadoc, README's Bulk Insert Token section, and added tests for: the new constructor, parsing path when present/absent, and toRawTokens() rendering path only when present. mvn -pl common,flowvault -am verify -> 707 tests, 0 failures, BUILD SUCCESS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: fix samples pom.xml's stale skyflow-flowvault-java dev version flowvault/samples/pom.xml pinned skyflow-flowvault-java to 3.0.0-beta.13-dev.18f8f1ba - a private dev-build version string carried over by mistake from the unrelated flowvault-release/26.8.1 branch when b3cf737 (SK-3002 release/26.8.1 #386) switched the sample's dependency from skyflow-java (v2) to skyflow-flowvault-java. Pinned to 1.0.1, the actual latest public GA release (main's [AUTOMATED] Public Release - 1.0.1). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Revert: bulkInsert (sync) structural exception-handling fix from #412 #412's second commit moved processBulkInsertSync's call to insertBatchFutures inside its own try, fixing a raw-exception leak via a throwing RequestInterceptor. Further investigation showed the same gap (missing SkyflowException-passthrough / generic Exception catch) exists identically in bulkDetokenize/bulkDeleteTokens/bulkTokenize's own outer catch lists too - not just bulkInsert's - so a narrower, operation-specific structural fix isn't the right shape for this. Reverting it here pending a uniform fix across all four sync methods. Removed testBulkInsert_unexpectedExceptionWrappedAsSkyflowException, which tested the now-reverted behavior. mvn -pl common,flowvault -am test -> 708 tests, 0 failures, BUILD SUCCESS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * chore: trigger internal-release workflow (re-run after commit-message false-skip) * chore: re-trigger internal-release workflow (#414) * [AUTOMATED] Private Release 1.0.1-dev-75ede823 * SK-3061 Flowvault release/26.8.13.1 (#416) * SK-3061 fix the unhandled exceptions * [AUTOMATED] Private Release 1.0.1-dev-2448f08e * SK-3061: fix bulkInsert(sync) interceptor-exception leak processBulkInsertSync called insertBatchFutures - which synchronously invokes the caller's RequestInterceptor - before entering its own try block, so a throwing interceptor escaped bulkInsert() as a raw, undeclared exception instead of the documented SkyflowException. processBulkDetokenizeSync/processBulkDeleteTokensSync/processBulkTokenizeSync already call their own *BatchFutures inside their own try/catch(Exception), which is why they were not exploitable the same way. This moves insertBatchFutures's call inside processBulkInsertSync's existing try, matching that same structure, instead of adding a new catch-all to the 4 public sync methods (the broader fix reverted here). Added a regression test asserting a throwing interceptor surfaces as SkyflowException from bulkInsert. Confirmed via TDD: failed before this change (raw IllegalStateException), passes after. mvn -pl common,flowvault -am test -> 709 (flowvault) + 106 (common/ValidationsTests) tests, 0 failures, BUILD SUCCESS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-7133bb23 * SK-3061: close PR #416 Codecov patch-coverage gaps Adds targeted unit tests for the new branches the Codecov patch report flagged as uncovered on PR #416 (82.52% patch coverage, 43 lines missing): - common/BearerToken: extractAccessToken() success branch (reflection). - flowvault/VaultClient: updateExecutorInHTTP wraps the retry interceptor's IllegalArgumentException (negative maxRetries) as SkyflowException. - flowvault/BulkInsertResponse & BulkDetokenizeResponse: buildSummary() with both records and originalPayload null (falls through the nested ternary's innermost 0 branch). - skyvault/VaultController: ApiClientException (network-error) catch in insert/detokenize/get/update/delete/query/tokenize/uploadFile, plus getFormattedBatchInsertRecord's "Body present but not a JSON object" branch. - skyvault/DetectController: ApiClientException catch in deidentifyText/reidentifyText/pollForResults/getDetectRun. - skyvault/Validations: validateGetRequest's orderBy != null false branch, unreachable through the public builder (which coalesces null to ORDER_ASCENDING) so forced via reflection. Two gaps were left uncovered on purpose because they are dead code, not missing tests (confirmed empirically, not just by inspection): - flowvault/VaultController's 4 new catch (ApiClientException e) blocks in bulkInsert/bulkDetokenize/bulkDeleteTokens/bulkTokenize (sync) are unreachable: each process*Sync helper already wraps everything in its own catch (Exception e) before anything can reach the outer catch. The existing testBulkInsert_throwingInterceptorWrappedAsSkyflowException regression test demonstrates this directly. - skyvault/Validations' validateTokensForInsertRequest has "tokensMap == null || valuesMap == null"; the valuesMap == null side can never be true when called from validateInsertRequest, since that method already rejects any null entry in `values` earlier in the same call. mvn -o test (common, flowvault, skyvault): 0 failures, 0 errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-f26c4df0 --------- Co-authored-by: skyflow-bharti <skyflow-bharti@users.noreply.github.com> Co-authored-by: Devesh Bhardwaj <devesh.bhardwaj@skyflow.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Devesh-Skyflow <Devesh-Skyflow@users.noreply.github.com> * chore: re-trigger internal-release workflow (#418) * chore: re-trigger internal-release workflow * Update pom.xml * [AUTOMATED] Private Release 1.0.1-dev-55bd5f19 * SK-3061: flatten Tokenize's response contract, generated type through public SDK shape (#419) * chore: trigger internal-release workflow rerun Empty commits don't trigger this workflow (paths-ignore treats a zero-file diff as vacuously all-ignored), so this touches pom.xml directly instead. * SK-3061: fix stale nested Tokenize response type to match real API contract V1FlowTokenizeResponseObject modeled a value + nested tokens[] array that the real API has never sent since a 2026-03-17 contract flattening (proto commit 7a656c0f, aligning Tokenize's shape with Detokenize's). The generated type was built from a stale spec snapshot and never corrected, so the SDK only produced correct output via a hand-written flatToken() workaround that scavenged token/tokenGroupName/error/httpCode out of Jackson's additionalProperties catch-all instead of reading real fields. Confirmed against the live proto (skyflowapi/common), a captured production response fixture already in this repo's tests, and the actual skyflow-fern-config/schemaless OpenAPI spec - all agree the wire shape is flat: token/value/tokenGroupName/error/httpCode as siblings, one row per (value, token group). Changes: - V1FlowTokenizeResponseObject: replaced value+tokens(nested) with the real flat fields (token, value, tokenGroupName, error, httpCode), matching the sibling V1FlowDetokenizeResponseObject's existing shape/conventions. - Deleted FlowTokenizeResponseObjectToken (the nested per-token type is no longer referenced anywhere). - Utils.java: buildTokenizeResponseTokens() now reads the flat fields directly; deleted flatToken() and the nested-vs-flat branching entirely. groupTokenizeRows()/acceptsRow()/valuesMatch() are untouched - the value-matching/folding logic that reconstructs per-record grouping from flat rows was already correct and needed no changes. - Updated the 6 tests that built the old nested shape directly to build the flat shape instead; replaced one test whose entire premise ("if the API ever returns the nested shape") is no longer a real code path with equivalent flat-shape coverage of the same folding behavior. Deliberately scoped to only the Tokenize response type + its consumer in Utils.java: Insert/Detokenize/DeleteToken and the FlowserviceClient/ VaultClient accessor structure are completely untouched, since they have no known contract issue and a full client regeneration (evaluated and set aside) would have meant unnecessary risk for operations that already work. Verified: mvn test (708/708, excluding one pre-existing unrelated failure - common's TokenTests.testExpiredTokenForIsExpiredToken depends on an uncommitted .env secret); mvn verify/japicmp shows zero public API changes (only diff present is an unrelated, already-merged Token.getPath() addition from #413). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * SK-3061: flatten public BulkTokenizeResponse to match the API's row shape BulkTokenizeResponse.getRecords() previously grouped rows back into a records[].tokens[] structure keyed by submitted value. The real API has never nested this way - it returns one flat row per (value, token group) pair. This drops the SDK-side grouping: BulkTokenizeResponseRecord and TokenizeResponseRecord are now flat (value, tokenGroupName, token, httpCode, error, requestId) with index kept on BulkTokenizeResponseRecord so rows from the same submitted value can still be correlated. Summary classification (totalTokenized/totalPartial/totalFailed) and getRecordsToRetry() are recomputed by grouping on index internally, including the case where a value gets zero rows back. TokenizeResponseToken is removed; its fields fold directly into TokenizeResponseRecord. Utils.java's row-to-index correlation (acceptsRow/valuesMatch/batch-splitting on duplicate values) is unchanged - only the final emission step is flat instead of nested. Samples and README updated to the flat shape. This is an intentional breaking change to BulkTokenizeResponse's public constructors; the japicmp baseline is regenerated accordingly. * chore: whitelist noextension in cspell word list DetectControllerTests.java's testGetBaseFileName_withoutExtensionReturnsWholeName (already merged on flowvault-release/26.8.13, ahead of this branch) uses the synthetic filename "noextension" as a test fixture, same pattern as the existing nocreds/nodir entries. Not otherwise related to this PR's Tokenize change; adding it here since it's what's blocking this PR's cspell check. * SK-3061: add BYOT-with-single-invalid-group coverage Closes the gap flagged earlier: every existing BYOT test only exercised naming too many groups (the 'should contain one token group' rejection). This covers a BYOT record naming exactly one group where that group itself is invalid - same error shape as the non-BYOT case, just on a BYOT record. Fixture verified against a live call (dev vault, single BYOT record naming one nonexistent group): the real API returns tokenGroupName=null, token=null, httpCode=400, and 'Tokenize failed. Token group X is invalid. Specify a valid token group.' - matching this test exactly. * SK-3061: cover partial tokenize failure with a retryable group through VaultController Closes the gap noted while discussing the retry path live: the only existing coverage of 'one value, one group succeeds, one group fails retryably (5xx)' was BulkResponseTests/BulkRetryAndSummaryTests exercising BulkTokenizeResponse directly. This runs the same scenario through VaultController.bulkTokenize() with a mocked raw client, so batching/index-assignment and getRecordsToRetry() are exercised together end to end, not just the summary math in isolation. A real 5xx can't be forced from the live API on demand (server-side failure, not something a crafted request triggers), so this is mocked, same as the existing apiErrorCapturedInErrors tests in this file. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * [AUTOMATED] Private Release 1.0.1-dev-34618022 * SK-3061: close tokenize patch-coverage gaps flagged on #420 (#421) * 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. * 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. * chore: whitelist unparseable in cspell word list * [AUTOMATED] Private Release 1.0.1-dev-490ffb14 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Devesh-Skyflow <Devesh-Skyflow@users.noreply.github.com> Co-authored-by: skyflow-bharti <118584001+skyflow-bharti@users.noreply.github.com> Co-authored-by: skyflow-bharti <skyflow-bharti@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bulk Insert's
getTokens()-typed README snippet (added in #408) referencedrecordwithout ever introducing it:recorddoesn't exist on its own — it only comes from iteratinginsertResponse.getRecords(). Copy-pasting this snippet as-is wouldn't compile. Bulk Tokenize and Bulk Delete Tokens had the opposite gap: no response-reading snippet at all, just a JSON sample and prose.Fix
Made all four bulk operations consistent — each section now has one code snippet that actually iterates
getRecords():Tokensnippet in the missing outer loop, plus a null check sincegetTokens()isnullon a failed record.tokens(it reports at two levels, per the paragraph above it).getError().Docs-only change — no source/test/baseline changes.
🤖 Generated with Claude Code