Uh oh!
There was an error while loading. Please reload this page.
SK-3061: revert fields→tokens rename, restore data field on bulkInsert response - #408
Merged
Merged
Conversation
…rt 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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## flowvault-release/26.8.13 #408 +/- ##
===============================================================
+ Coverage 91.30% 91.36% +0.06% - Complexity 0 475 +475
===============================================================
Files 157 158 +1 Lines 6392 6440 +48 Branches 850 859 +9 ===============================================================
+ Hits 5836 5884 +48 + Misses 364 362 -2 - Partials 192 194 +2
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:
|
…dd 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>
- 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>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>
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>
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>
…n 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>
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>
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>
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>
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>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>
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>
…en>>), 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>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>
Contract baseline change detected ( |
| Status | Type | Serialization | Compatibility Changes |
|---|---|---|---|
| Modified | com.skyflow.vault.data.BulkInsertResponseRecord |  |   |
| Modified | com.skyflow.vault.data.InsertResponseRecord | |   |
| Added | com.skyflow.vault.data.Token | | |
Expand for details.
com.skyflow.vault.data.BulkInsertResponseRecord
- Binary-compatible
- Source-compatible
- Serialization-compatible
| Status | Modifiers | Type | Name | Extends | JDK | Serialization | Compatibility Changes |
|---|---|---|---|---|---|---|---|
| Modified | public | Class | BulkInsertResponseRecord | InsertResponseRecord | JDK 8 | |  |
Constructors
| Status | Modifiers | Generics | Constructor | Annotations | Throws | Compatibility Changes |
|---|---|---|---|---|---|---|
| Source-incompatible | public | BulkInsertResponseRecord(int, String, String, Map<String, Object>Map<String, List<Token>>, Map<String, Object>, int, String, String) | Deprecated: forRemoval=true, since="1.0.2" | | ||
| Added | public | BulkInsertResponseRecord(int, String, String, Map<String, List<Token>>, Map<String, Object>, Map<String, Object>, int, String, String) | |
com.skyflow.vault.data.InsertResponseRecord
- Binary-compatible
- Source-compatible
- Serialization-compatible
| Status | Modifiers | Type | Name | Extends | JDK | Serialization | Compatibility Changes |
|---|---|---|---|---|---|---|---|
| Modified | public | Class | InsertResponseRecord | Object | JDK 8 | | |
Constructors
| Status | Modifiers | Generics | Constructor | Annotations | Throws | Compatibility Changes |
|---|---|---|---|---|---|---|
| Source-incompatible | public | InsertResponseRecord(String, String, Map<String, Object>Map<String, List<Token>>, Map<String, Object>, int, String) | Deprecated: forRemoval=true, since="1.0.2" | | ||
| Added | public | InsertResponseRecord(String, String, Map<String, List<Token>>, Map<String, Object>, Map<String, Object>, int, String) | |
Methods
| Status | Modifiers | Generics | Type | Method | Annotations | Throws | Compatibility Changes |
|---|---|---|---|---|---|---|---|
| Added | public | Map<String, Object> | getData() | | |||
| Source-incompatible | public | Map<String, Object>Map<String, List<Token>> | getFields() | Deprecated: forRemoval=true, since="1.0.2" | | ||
| Added | public | Map<String, List<Token>> | getTokens() | |
com.skyflow.vault.data.Token
- Binary-compatible
- Source-compatible
- Serialization-compatible
| Status | Modifiers | Type | Name | Extends | JDK | Serialization | Compatibility Changes |
|---|---|---|---|---|---|---|---|
| Added | public | Class | Token | Object | JDK 8 | | |
Constructors
| Status | Modifiers | Generics | Constructor | Annotations | Throws | Compatibility Changes |
|---|---|---|---|---|---|---|
| Added | public | Token(String, String) | |
Methods
| Status | Modifiers | Generics | Type | Method | Annotations | Throws | Compatibility Changes |
|---|---|---|---|---|---|---|---|
| Added | public | String | getToken() | | |||
| Added | public | String | getTokenGroupName() | | |||
| Added | staticpublic | Map<String, List<Token>> | parseTokens(Map<String, Object>) | | |||
| Added | public | String | toString() | |
Warning
All missing classes, i.e. superclasses and interfaces that could not be found on the classpath were ignored.
Hence changes caused by these superclasses and interfaces are not reflected in the output.
Generated on: 2026-08-13 15:21:49.670+0000.
Devesh-Skyflow
merged commit Aug 13, 2026
b17c441
into
flowvault-release/26.8.13
27 of 28 checks passed
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Fixes the deviations from the FlowVault API contract flagged in this Slack thread, plus a documentation/samples cleanup pass that grew out of it.
1. Response contract fix
fields→tokens: the SDK had renamed the API'stokensresponse key tofields, so callers neededgetFields()instead of the API-matchinggetTokens(). Reverted the rename;getFields()stays as a@Deprecatedalias that logs a warning and delegates togetTokens()— existing callers keep working unchanged,detokenize/deleteTokensuntouched throughout.datafield restored: the API'sdatafield was silently dropped from the bulk insert response. It's wired back through fromV1RecordResponseObject.getData(), which already carried it — nothing inUtils.formatBulkInsertResponsewas reading it.Compatibility note:
flowvault/pom.xmlruns ajapicmpbinary/source-compatibility check againstflowvault/api-report/*.baseline.jarforcom.skyflow.vault.data. Rather than changing the existingInsertResponseRecord/BulkInsertResponseRecordconstructor signatures (a breaking change under that gate), the old (data-less) constructor overloads are kept as@Deprecatedpass-throughs, anddatais added via new overloads. No baseline regeneration should be required.2.
flowvault/README.mdaccuracy passWent through the README against the actual source rather than just fixing the two items above:
1.0.0; pom.xml is1.0.1.CustomHeaderKeysample used names (SkyflowAccountId, etc.) that don't exist — the real enum isSKYFLOW_ACCOUNT_ID/SKYFLOW_ACCOUNT_NAME/REQUEST_ID_HEADER. The README's own code block didn't compile.vault()takes no arguments, use one client per vault" was false —vault(String vaultId)exists and is tested for multi-vault use on one client. Documented it.updateTypeacceptsUPDATE(the default)" overstated the SDK's behavior — it omits the field when unset rather than sending"UPDATE"itself; reworded.getHttpStatus()example used"BAD_REQUEST"; the actual hardcoded validation-error string is"Bad Request".tokensJSON example showed a flat string per column; the real shape is always a list of{token, tokenGroupName}entries per column (one per token group), per a dedicated regression test (testBulkInsert_successWithListOfMapsTokenShape). Fixed the example and added a snippet showing how to read it (there's no typed accessor yet — same gap raised in the Slack thread).bulkInsert= structured,bulkTokenize/bulkDeleteTokens= schemaless,bulkDetokenize= both) — flagged in the thread as a known doc gap, confirmed against git history (SK-2646, which shipped tokenize/delete-tokens specifically as "Schemaless vault apis") since there's no code-level enforcement of it. Repeated as a one-line note on each of the four operation sections so it's visible without reading top-to-bottom.metadataJSON example initially backed out a claim about its typical content (onlyskyflowIdwas grounded in code;tableNamewas just a Javadoc description with nothing behind it), then re-grounded it againstflowdb_dp_apis.proto's own literal example value for the field,{"table": "table1", "skyflowID": "4524524534623"}— stronger evidence than the description text, which is itself misleading (it says "tableName", the actual wire key istable;Utils.javaonly renamesskyflowID→skyflowId,tablepasses through unrenamed). Updated the JSON example to match.3. Samples cleanup (
flowvault/samples/)BearerTokenExpiryExample.java— despite its name, it never touchesBearerToken/Token.isExpired(); it's a generic "retry once on 401" wrapper, redundant withBearerTokenGenerationExample's real expiry pattern and the README's own retry guidance.samples/README.md— it referencedDetokenizeExample.java,InsertExample.java,GetByIdExample.java, etc., none of which exist in this module (stale boilerplate from a different layout). Replaced with an accurate index of the real samples and correct Maven run instructions.BulkInsertSync/Async,BulkMultiTableInsertSync/Async,BulkDetokenizeSync/Async, andCustomHeaderExample— these previously only didSystem.out.println(response).BulkTokenizeSync/AsyncandBulkDeleteTokensSync/Asyncalready had this pattern.4. Testing
ResponseComponentTests,BulkResponseTests,UtilsTests,VaultControllerTestsfor the rename/restore, including a dedicated test for the deprecated constructor +getFields()alias.InsertResponseRecord.java(its own deprecated constructor — never exercised directly, sinceBulkInsertResponseRecord's deprecated constructor bypasses it). Added a direct test for it.mvn -pl common,flowvault -am test→ 692 tests, 0 failures.mvn -pl common,flowvault -am verify(full reactor,japicmpincluded) → BUILD SUCCESS against the regenerated baseline from item 6.common'sTokenTests.testExpiredTokenForIsExpiredTokenreads aTEST_EXPIRED_TOKENvalue from a.envfile that isn't committed to the repo (a CI-only secret) — nothing to do with this PR,com.skyflow.serviceaccount.util.Tokenis a pre-existing, unrelated class (JWT expiry check) that this PR never touches.5. Bug fix:
Skyflow.getVaultConfig()crashed on an empty vault listFound while double-checking the
vault()/vault(String)claim above — a different, unrelated method with a real bug:build()never validates that at least one vault was registered, so a client built with zeroaddVaultConfig(...)calls is a valid, reachable state — and callinggetVaultConfig()on one crashed with an uncheckedArrayIndexOutOfBoundsExceptionrather than failing predictably. Traced every call site in the codebase first: all of them are onVaultController(a different, already-safe method with the same name — it just returns its own single stored config, no lookup involved), never onSkyflowdirectly. So this had zero usages and zero test coverage anywhere in the suite.Fixed by mirroring the sibling method's already-correct convention —
BaseSkyflow.getVaultConfig(String)is a plainmap.get(vaultId)that returnsnullwhen absent, no exception, no signature change:Deliberately did not make it throw
SkyflowExceptionlikevault()/vault(String)do — that would diverge from its own sibling method's contract and require adding a checked exception to the signature, a source-incompatible change under this module'sjapicmpgate, for no real benefit. This fix is implementation-only (same signature), so no baseline update needed. Added 7 tests inSkyflowTestscovering the regression case plus the existing untested gaps on the by-id overload.6.
InsertResponseRecord.getTokens()is now typed (Map<String, List<Token>>)The generic
tokens: Map<String, Object>(item 2 above) still required casting/iterating by hand to reachtoken/tokenGroupName— the exact usability complaint in the Slack thread. First pass at fixing this added a separate typed accessor (getTokenDetails()) alongside the untouched genericgetTokens(), keeping both per the thread's "generic for flexibility, typed for UX" clarification. Per explicit direction, went further:getTokens()itself is now the typed one —Map<String, List<Token>>— matching the shape this field had before insert was reworked (flowvault's own pre-rework "v3"Success.tokenswasMap<String, List<Token>>;getTokenDetails()no longer exists).This is the one genuinely breaking change in this PR — confirmed against the actual
japicmpdiff once a JDK/Maven became available (not just reasoned about in the abstract):getTokens()itself was never breaking. It's reported as a brand-new method against the committed baseline — the pre-SK-3061 codebase only ever hadgetFields(), so any shapegetTokens()returns is additive. My initial writeup here overstated this, assuming a same-arity generic-erasure conflict that doesn't actually apply to a method with zero parameters (erasure conflicts are a parameter-list problem, not a return-type one — a method can't be overloaded by return type at all, regardless of arity, so this was never really in question).com.skyflow.vault.data:InsertResponseRecord's deprecated 6-arg constructor andBulkInsertResponseRecord's deprecated 8-arg constructor (both had theirtokensparameter generics change), andgetFields()(return type generics changed, since it just delegates togetTokens()).getFields()could reverse-map back to the original shape instead of delegating directly. Flagged this and asked; declined in favor of just regenerating the baseline (see the warning banner above and the regeneratedflowvault/api-report/skyflow-flowvault-java.baseline.jarin this PR).Object. Accepted as the tradeoff for matching the requested typed shape.Token(getToken()/getTokenGroupName()) isn't a new invention — it's a straight reintroduction of a class flowvault itself had before insert was reworked (deleted in685a82ff), withfinalfields and atoString()added to matchTokenizeResponseToken's established convention in this package.The actual parsing (a column's raw wire value can be a list of
{token, tokenGroupName}entries, a single such entry, or a bare value) now lives in a new static utility,Token.parseTokens(Map<String, Object>), called once byUtils.formatBulkInsertResponsewhen constructing each record — not recomputed on everygetTokens()call like the first pass'sgetTokenDetails()was.Found and fixed a real landmine while adding the
Tokenclass:ResponseComponentTests.javahad a dangling{@link Token}Javadoc reference left over from the class's original removal, which would have silently started resolving to this new class (while the surrounding comment still said Token "was removed") had I not updated it.Updated every test that constructed a record with a raw
Map<String,Object>tokens value or asserted on the old generic return type,BulkInsertSync/Async,BulkMultiTableInsertSync/Async, andCustomHeaderExampleto usegetTokens()directly (matchingBulkTokenizeSync.java's existing typed per-token loop), and closed every branch in the new parsing logic with tests (a per-columnnullvalue, anullelement inside a token-group list, a Map entry missing one of its two expected keys).VaultControllerTests.testBulkInsert_successWithListOfMapsTokenShapesimplified nicely as a result — no more manual casting needed to assert on it.Also updated outside this repo
The FlowVault 1.0.0 migration Confluence doc (linked from the Slack thread) has been corrected to match the response-contract change and flags the
Object-typing gap (now closed by item 6, but the doc itself is unchanged since this PR).Not in this PR (flagged as follow-ups)
datahad here).🤖 Generated with Claude Code