SK-2814: java public interface cleanup#314
Merged
Devesh-Skyflow merged 65 commits intoMay 21, 2026
Merged
Conversation
Captures the design for public interface renames per the server-side SDK nomenclature changes spec: credential field fallbacks (clientId/keyId/tokenUri), skyflow_id→skyflowId in Get/Query responses, and QueryResponse errors/tokenizedData field additions. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds explanation for why tokenizedData change is valid despite the Query API currently not returning tokens — based on V1FieldRecords schema support and cross-SDK consistency requirement. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds detailed rationale to each design section: why the naming convention matters, why the fallback strategy was chosen over a hard cut, why skyflow_id normalization is inconsistent today, the tokenizedData API schema vs docs discrepancy, and why getErrors() is missing only from QueryResponse. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Clarifies that ignoring record.getTokens() in getFormattedQueryRecord is intentional (Query API cannot return tokens), and that the fix is to promote the toString() hack into a real always-empty field rather than reading from the API response. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Query API cannot return tokens; the toString() inconsistency is not worth fixing since callers have no reason to access tokenizedData programmatically on query results. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
5-task TDD plan covering credential field renames with fallback, skyflow_id normalisation in Get/Query responses, and QueryResponse getErrors() accessor. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…old form Add fallback lookup logic so getBearerTokenFromCredentials tries new camelCase keys (clientId, keyId, tokenUri) first and falls back to the legacy all-caps forms (clientID, keyID, tokenURI) for backward compatibility during migration. Add testBearerTokenWithNewFormCredentialKeys to verify the new key form is recognized end-to-end. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…form Add fallback logic to GenerateSignedTokensFromCredentials so both new-form keys (clientId/keyId) and legacy all-caps keys (clientID/keyID) are accepted during migration. Mirrors the pattern already applied to BearerToken.java. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Insert and Update responses already used camelCase skyflowId; Get and Query were passing through the raw wire-format snake_case key. Add the rename in getFormattedGetRecord and getFormattedQueryRecord, and add reflection-based unit tests to cover both formatters. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds a private final errors field (always null) and its public accessor
to QueryResponse, matching the pattern in GetResponse and InsertResponse.
Removes the hardcoded responseObject.add("errors", null) from toString()
since serializeNulls on the declared field handles it automatically.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adapted from skyflow-node PR #305. Includes: - CLAUDE.md with project overview, structure, naming conventions, build commands - .claude/settings.json with PostToolUse compile+checkstyle hooks, PreToolUse generated-code guard, Stop notification; paths are relative (no hardcoded user dirs) - .claude/commands/: code-review, code-security, sdk-sample, test slash commands Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- CLAUDE.md: add vault/bin/ package, all 5 controllers, pre-existing test failure baseline - settings.json: fix checkstyle hook to print violations (was silently swallowing output with capture_output=True) - sdk-sample.md: fix InsertOptions (doesn't exist), correct sample package structure, correct credential type per feature - code-review.md: fix validation location (controller not build()), fix HashMap rule (SDK pattern is raw HashMaps) - test.md: document pre-existing failures, note checkstyle failsOnError Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
6-task TDD plan: restore skyflow_id key alongside skyflowId in Get/Query responses, add WARN deprecation logs for old credential fields, Javadoc on affected response methods. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…y standard Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ecation Non-technical overview of credential field renames and skyflow_id response key deprecation — covers customer impact, deprecation warnings, migration guide, and what is NOT changing. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Deprecation plan: add Task 6 for GetRequest + DetokenizeRequest with @deprecated annotation approach (compile-time signal vs runtime log) - PM doc: add section 3 for downloadURL rename with migration example Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…nd Update The Skyflow API accepts additionalProperties of Any type including null and empty strings. SDK should not add validation on top of BE — pass through and let BE decide. Removed: - value == null/isEmpty check in validateInsertRequest - value == null/isEmpty check in validateUpdateRequest - value == null/isEmpty check in validateTokensMapWithTokenStrict - values.isEmpty() check in validateInsertRequest (no minItems in API spec) Kept: - values == null check (NPE guard — cannot iterate null array) - key == null/isEmpty check (null keys cannot be JSON-serialized) Deleted 6 tests that asserted on the removed behaviour. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- PM doc: new section explaining how @deprecated(forRemoval) shows in IntelliJ/VS Code autocomplete (strikethrough, orange underline, tooltip with clickable link to new method) vs runtime WARN log for map keys - Deprecation plan: update downloadURL tasks to use @deprecated(since="2.1", forRemoval=true) + {link} for stronger IDE signal Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…mmand Splits old Section 6 into: - Section 6: Code quality (actionable correctness checks) - Section 7: Code smells (structural signals, flagged at Smell severity) Code smell catalogue covers: long methods/classes, business logic in data classes, toString() with logic, deep nesting, magic numbers, raw HashMap chains, dead code, stale comments, temporary fields. Severity table clarified: Critical/Bug/Edge Case/Quality = fix before merge; Smell = flag and track. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…mpat Both skyflow_id (deprecated) and skyflowId (new form) are now present in response maps simultaneously. WARN log emitted per record.
…d DetokenizeRequest Old downloadURL() methods kept as @deprecated(forRemoval=true) delegates. Runtime WARN log emitted on old form usage. 100% test coverage: new form, deprecated form, default value for both classes. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…pported Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… tests Both old (downloadURL) and new (downloadUrl) builder methods tested: - GetTests: 2 new tests for downloadUrl() with cross-assertion on deprecated getDownloadURL() returning same value - DetokenizeTests: 1 new test same pattern - VaultClientTests: 1 new integration test for DetokenizeRequest.downloadUrl() Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Extract 260-line migration section from README.md to docs/migrate_to_v2.md following the pattern established in skyflow-node PR #258 - README now links to docs/migrate_to_v2.md instead of inline content - docs/migrate_to_v2.md adds v2.1+ sections for credential field renames and skyflow_id deprecation (new content) - CHANGELOG.md: add v2.0.4 release notes covering nomenclature changes, backward compat deprecations, and validation removal Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ore paths Added words: serialise/d/s, normalise/d/s/Normalises, behaviour/s/Behaviour, sanitisation, recognised, unrecognised, prioritised Added regex: /-D[A-Za-z][A-Za-z0-9.]*/g to ignore Maven -D flags Added ignorePaths: RUNNING_SAMPLES.md, docs/superpowers/** Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Real 2048-bit RSA key replaced with fake base64 value. Assertion updated from InvalidTokenUri to InvalidKeySpec — still proves all credential fields were resolved (failure is at RSA parsing, not field lookup). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Calling .downloadUrl(null) previously stored null in the field, creating an NPE risk for callers who read getDownloadUrl() back without a null check. Now null -> false (matching the default), consistent with the continueOnError(null) guard in the same builder. Added test: testDetokenizeRequestDownloadUrlNullTreatedAsFalse Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
EmptyValues, EmptyValueInValues, EmptyValueInTokens (ErrorMessage) and EMPTY_VALUES, EMPTY_OR_NULL_VALUE_IN_VALUES, EMPTY_OR_NULL_VALUE_IN_TOKENS (ErrorLogs) are unreachable since the SDK-level null/empty field validation was removed. Deleted to prevent accidental re-wiring. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This reverts commit a802668.
Three new tests assert that previously-blocked inputs now pass SDK validation (SDK defers to BE per API spec additionalProperties: Any type): - Empty values array [] passes - Null field value passes - Empty string field value passes Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…e request ID - Add EMPTY_STRING, QUOTE, HTTPS_PROTOCOL, CURLY_PLACEHOLDER and HttpUtilityExtra (RAW_BODY_KEY, SDK_GENERATED_PREFIX) to Constants - HttpUtility: conditional content-type header, __raw_body__ passthrough, UUID fallback when server omits x-request-id, URL-encoded form params - Utils: URL-encode path and query params with graceful fallback - Validations: accept String request bodies for non-JSON content types - ConnectionController: wrap String bodies in __raw_body__ for non-JSON content types; fall back to raw string when response is not JSON - InfoLogs: "Bearer token is expired" → "Bearer token is invalid or expired" - HttpUtilityTests: add raw body, no content-type, null request ID and special-character form-encoding tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…etectController - Remove dead statement `response.getEntities().get(0).getFile()` that discarded its result and threw IndexOutOfBoundsException/NPE when entities was null or empty (C1); the guarded block below already handles entity processing correctly - Replace unguarded `response.getOutput().get()` in getFirstOutput() with `.orElse(null)` so an absent output Optional returns null instead of throwing NoSuchElementException (C2) - Replace unguarded `firstOutput.getProcessedFileExtension().get().toString()` with `.map(Object::toString).orElse(UNKNOWN)`, reusing the already-computed Optional and matching the safe pattern used for processedFileType one line above (C3) - Add 4 unit tests covering all three fixes via reflection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rm-encode - VaultController.delete: replace unguarded getRecordIdResponse().get() with .orElse(Collections.emptyList()) so an absent field in the API response returns an empty list instead of throwing NoSuchElementException (M2) - BearerToken, SignedDataTokens: wrap FileReader in try/finally to guarantee close() is called even when JsonParser throws JsonSyntaxException; close IOException is intentionally swallowed since the parse already completed (M3) - HttpUtility.formatJsonToFormEncodedString: guard against empty entry set so substring(0, -1) is not called on an empty StringBuilder (M4) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace deprecated credential key names in examples: clientID→clientId, keyID→keyId, TokenURI→tokenUri - Update Insert, Get, and Query response examples to use skyflowId (the SDK has always returned skyflowId for Insert; Get/Query now return skyflowId as the primary key) - Add deprecation note after each affected response block: skyflow_id is deprecated and will be removed in an upcoming release Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d header When the response carries no x-request-id header, set requestID to null instead of fabricating a UUID with an SDK-Generated- prefix. A null value is the honest signal to callers that no server request ID is available. Also removes the now-unused UUID import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace per-record printWarningLog calls for DEPRECATED_SKYFLOW_ID_KEY with printWarningLogOnce so the deprecation notice fires at most once per JVM session regardless of result set size. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The SDK no longer generates a fallback UUID when the server omits x-request-id; getRequestID() now returns null. Update the assertion to match the new behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The SDK puts camelCase requestIndex in response maps; the README examples incorrectly showed snake_case request_index. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…n warnings per-request - UpdateRequest.data now accepts 'skyflowId' (preferred) alongside deprecated 'skyflow_id' - extractUpdateSkyflowId() prefers camelCase key, emits WARN-level deprecation on snake_case fallback - Remove printWarningLogOnce — all deprecation warnings now fire on every request via printWarningLog - Add DEPRECATED_SKYFLOW_ID_REQUEST_KEY log entry for request-side key deprecation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion warning When both keys are present, skyflowId is used and a warning is emitted. Adds unit tests for all four cases: camelCase only, snake_case only, both keys (preference), and both keys (map cleanup). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…elegate - TokenMode.getByot() is the new preferred accessor; getBYOT() retained as @deprecated(since="2.1",forRemoval=true) delegate for back-compat - VaultClient updated to call getByot() at all three insert/update sites - UpdateTests: 5 new tests covering camelCase skyflowId key in UpdateRequest - LogUtilLevelTests: 5 new tests verifying WARN log fires at DEBUG/INFO/WARN levels and is suppressed at ERROR (matches Skyflow default) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gate, and toString() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- InfoLogs: add DEPRECATED_GET_BYOT constant - TokenMode.getBYOT(): call LogUtil.printWarningLog() so callers see a runtime warning (consistent with downloadURL() and other deprecated methods) - TokenModeTest: replace @SuppressWarnings-only tests with assertions that the warning fires at INFO level and is suppressed at ERROR level Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…est deprecation warnings - InfoLogs: keep DEPRECATED_SKYFLOW_ID_REQUEST_KEY, DEPRECATED_SKYFLOW_ID_BOTH_KEYS, DEPRECATED_GET_BYOT added on this branch (superset of release branch) - VaultController: resolve printWarningLogOnce vs printWarningLog conflict in favour of printWarningLog (fires on every request, not once per JVM) - LogUtil: drop printWarningLogOnce and its WARNED_ONCE infrastructure re-introduced by the auto-merge; method is unused after conflict resolution - VaultControllerTests / UpdateTests: keep all new tests from this branch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…KYFLOW_ID_REQUEST_KEY Remove DEPRECATED_SKYFLOW_ID_BOTH_KEYS; both the snake_case-only and both-keys-present paths in extractUpdateSkyflowId now emit the same DEPRECATED_SKYFLOW_ID_REQUEST_KEY message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…public-interface-cleanup
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
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.
No description provided.