Devesh/sk 2814 java public interface cleanup#316
Open
Devesh-Skyflow wants to merge 90 commits into
Open
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>
…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
…acement - Skyflow.setLogLevel() is the new preferred method on the live client - updateLogLevel() retained as @deprecated(since="2.1",forRemoval=true) delegate; emits runtime warning via DEPRECATED_UPDATE_LOG_LEVEL log - InfoLogs: add DEPRECATED_UPDATE_LOG_LEVEL constant - SkyflowException: add JavaDoc documenting validation vs API error paths, null behaviour of requestId/grpcCode, and typical catch pattern - SkyflowTests: add setLogLevel test, deprecation-warning-fires test, and warning-suppressed-at-ERROR test for updateLogLevel() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…low_id form - UpdateExample.java: replace skyflow_id with skyflowId in both data maps - deprecated/UpdateExample.java: retain original skyflow_id pattern with @deprecated annotation and pointer to the current example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add sections for UpdateRequest skyflowId preference, updateLogLevel and getBYOT deprecations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes all reachable coverage gaps across 16 test files. Adds tests for detect response types (ReidentifyTextResponse, DeidentifyTextResponse, EntityInfo, TextIndex, TokenFormat), enum values (DetectOutputTranscriptions, DetectEntities, MaskingMethod, TokenType, DeidentifyFileStatus), InvokeConnectionResponse.getErrors(), DeidentifyFileRequest/Response constructors and builder null-handling, SkyflowException missing JSON branches, InsertRequest.continueOnError(false), and VaultConfig/ ConnectionConfig null-credentials fallback. Two residual JaCoCo false negatives remain: Optional.filter lambda in DetokenizeRecordResponse (unreachable null check) and dead ternary branches in Skyflow$SkyflowClientBuilder whose guards are always non-null after validation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tRecord getFormattedGetRecord and getFormattedQueryRecord both emit the deprecation warning when the API response contains the snake_case skyflow_id key. getFormattedBatchInsertRecord was inconsistent — it silently mapped skyflow_id to skyflowId with no warning. Also future-proofs the batch insert path to handle camelCase skyflowId if the API wire format ever migrates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents SkyflowException — its two categories (validation vs API errors), all six properties (httpCode, message, httpStatus, grpcCode, requestId, details), the recommended try/catch pattern, and a table distinguishing what is null/empty for validation errors versus API errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… table The subsection below the table already covers null/empty behaviour for validation errors; repeating it inline on every row was noise. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…814-java--public-interface-cleanup
…names table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…814-java--public-interface-cleanup
…recated UpdateExample DetokenizeExample demonstrates the deprecated downloadURL() builder method. UpdateExample now also demonstrates the deprecated updateLogLevel() client method alongside the existing skyflow_id key deprecation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… key Demonstrates reading the deprecated "skyflow_id" key alongside the preferred "skyflowId" key in the Get response map, retained for reference during the deprecation window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nController
Adds comprehensive unit tests across the three controller classes to
improve JaCoCo instruction coverage from ~25% to 87%/43%/87%:
- VaultController: full happy-path + API-error tests for insert (bulk
and batch), detokenize, get, update, delete, query, tokenize; also
unit tests for extractUpdateSkyflowId, getFormattedGetRecord,
getFormattedQueryRecord, downloadUrl accessors, and skyflowId
normalisation. All private helpers exercised via reflection.
- ConnectionController: happy-path tests for all HTTP methods (GET,
POST, PUT, DELETE) plus non-JSON response wrapping, requestId in
metadata, and all validation-failure paths (empty headers/params/
body, null header value, IOException, SkyflowException propagation).
- DetectController: happy-path + API-error tests for deidentifyText,
reidentifyText, and getDetectRun; drives extractBodyAsString to 100%.
- pom.xml: prefix surefire argLine with ${argLine} so JaCoCo's agent
is properly injected into the test JVM and coverage data is accurate.
Total test count: 462 → 514. Pre-existing 5 failures unchanged.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
VaultController: - getFormattedBatchInsertRecord: 77% → 100% (deprecated skyflow_id key path, error-body path, mixed success+error batch result) - detokenize: 95% → 100% (mixed success+error response, empty records list) - get: 97% → 100% (added redactionType to cover non-null ternary branch) - insert: 85% → 99% (batch all-error path, deprecated-key path, mixed result; remaining 2 instr are unreachable dead-code ternary branch) DetectController: - parseDeidentifyFileResponse: 66% → 95% (wordCharacterCount branch L272-273, processedFile decode+write branch L283-291; IOException catch at L290-291 requires mocking NIO and is left uncovered) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After the `if (insertedFields.isEmpty()) { return; }` guard, insertedFields
is guaranteed non-empty, making `insertedFields.isEmpty() ? null : insertedFields`
dead code. Simplify to just `insertedFields`, bringing insert to 100% coverage.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tests Adds 19 new test methods covering deidentifyFile validation, happy paths, IN_PROGRESS timeout, all processFileByType extension branches, and error paths. Also fixes null-dereference NPE in Validations.validateDeidentifyFileRequest when waitTime is null. Brings DetectController instruction coverage from 49% to 92%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Controller Adds three more tests: - processedFile present with no outputDirectory (covers line 146/168 branches) - IN_PROGRESS → SUCCESS with waitTime=2 (covers lines 218-229 if-branch retry) - IN_PROGRESS → SUCCESS with waitTime=3 (covers lines 225-226 else-branch retry) Brings DetectController to 95% instruction / 96% line / 86% branch coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ontroller Tests cover getFileForFileUpload via filePath, base64, and fileObject inputs, plus an API error path. Brings VaultClient instruction coverage from 89% to 91%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds 6 tests (txt/mp3/pdf/jpg/csv/dcm extensions with tokenFormat.entityUniqueCounter) to cover the entityUniqueCounter mapping branches across all deidentify request-builder methods. Brings VaultClient instruction coverage to 96%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds two tests with hand-crafted fake JWTs (no env var dependency): - 3-part token with expired payload (exp=1) → covers isExpired=true path - 3-part token with far-future payload (exp=9999999999) → covers isExpired=false path Brings Token instruction coverage from 58% to 95%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests appendRequestId with non-null, null, and empty requestId; also covers convertJsonToMap recursive path with nested JSON objects via sendRequest. Brings HttpUtility from 87% to 91% instruction coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntial fields are used Adds WARN logs in BearerToken and SignedDataTokens when the old all-caps field names (clientID, keyID, tokenURI) trigger the fallback path, so callers know to migrate to the camelCase forms (clientId, keyId, tokenUri). Also adds the three corresponding InfoLogs entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests across ConnectionClient, VaultClient, and Skyflow builder to
cover all previously missed code paths and reach 100% JaCoCo instruction
coverage on every public-facing package (com.skyflow, config, enums,
errors, vault.*).
New test files:
- ConnectionClientDotenvTests: PowerMock tests for dotenv credential path
- VaultClientDotenvTests: actual .env file approach (Java 21 compatible)
- BinAuditTests: GetBin/ListEvent constructor coverage
Extended tests:
- ConnectionClientTests: apiKey reuse, bearer token reuse, credential
change reset, and no-credentials (DotenvException) paths
- SkyflowTests: findAndUpdateVaultConfig/findAndUpdateConnectionConfig
null-fallback branches via anonymous subclasses and reflection
- VaultClientTests: file upload null return, masking method, entity
scores lambda, credential change reset, HTTP interceptor, and
getDeidentifyGenericFileRequest branch coverage
pom.xml: switch surefire argLine to @{argLine} (late-binding) so JaCoCo
agent arg is injected correctly; add --add-opens java.base/java.io for
PowerMock on Java 21.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes cspell failures in CI caused by abbreviations used in test string literals (nocreds, nodir, detok) and a hardcoded cluster ID in tracked sample files (qhdmceurtnlz). Also adds ngrok and obac which appear in sample config files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…apping When Dotenv loads successfully but SKYFLOW_CREDENTIALS is absent, SkyflowException thrown inside the try block was being caught by the generic catch(Exception e) handler and wrapped in RuntimeException. Add an explicit catch(SkyflowException e) before the generic handler so it propagates directly, matching the declared throws signature. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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
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.
Start with a concise summary of the PR. The first three sections are required. The questions present in each section is there to help you guide you what to add. They are meant to be overwritten by your comments.
Why
Goal
Testing
Tech debt