SK-2954: common/v2/flowvault split + flowvault insert support#269
Open
saileshwar-skyflow wants to merge 11 commits into
Open
SK-2954: common/v2/flowvault split + flowvault insert support#269saileshwar-skyflow wants to merge 11 commits into
saileshwar-skyflow wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…lowdb (v3) insert support Restructures the repo into three build variants sharing a bundled common/ module (SK-2938 Option C): v2 (today's SDK, behavior-preserving) and a new v3 built on the flowservice/flowdb API, insert-only this round. common/ - Shared credential resolution, vault-URL resolution, and bearer-token fetch/cache/expiry logic (VaultController, BaseVaultClient), enums, errors, service_account, and generic validators. - VaultController declares insert/get/update/delete/query/detokenize as abstract methods (Java-interface-style); v2 and v3 each provide their own concrete/stub implementations. v2 - Relocated from the repo root via git mv; public API unchanged (same class names, signatures, import paths). Vault is now a backward-compatible alias for the internal PdbVaultController class. - Fixed a latent bug where v2's own Env enum failed cross-class comparisons against common's Env; both now share one definition. v3 (skyflow-flowvault, starting at 1.0.0) - New InsertRequest/InsertRecord/Upsert/InsertResponse types, FlowVaultController, and VaultClient targeting the flowservice REST API. - Insert validation ported from Java's v3 Validations.java: table/upsert must live in exactly one place (request-level or per-record, matching in both), 10k record cap, empty key/value checks. - InsertResponse mirrors Java's v3 shape (summary/success/errors) as plain dicts, each result tagged with its index in the original record list (stable across batch boundaries). - Structured per-record error parsing from the backend's actual error body, plus x-request-id propagation onto error entries. - Batching via INSERT_BATCH_SIZE (default 50, max 1000), sequential, isolate-and-continue on a failing batch. - Vault URL resolution uses v3's own skyvault.skyflowapis.* domain for all four envs (DEV/SANDBOX/STAGE/PROD), confirmed to differ from v2's vault.skyflowapis.* domain. CI/CD - shared-tests.yml and shared-build-and-deploy.yml now take a `variant` input and scope every step to v2/ or v3/ via working-directory. - main.yml, ci.yml, beta-release.yml, internal-release.yml, and release.yml matrix over both variants. v3 releases are distinguished from v2's via a flowvault- tag/branch prefix (flowvault-1.0.0, flowvault-release/*) so a release trigger is never ambiguous between the two independently-versioned packages; v2's existing bare-semver tags are untouched. - Fixed ruff.toml/.codespellrc still excluding a pre-split "skyflow/generated" path that no longer existed after the relocation. - Fixed a bump_version.sh sed collision with a comment that happened to contain the literal text "__version__ = ...". - Added a common/ test job to main.yml/ci.yml. Note: v3/samples/ and the root samples/ folder are deliberately excluded from this branch/commit -- local working copies there contain credentials used for live testing against a real vault and must not be pushed. Tests: common 36, v3 65, v2 426 (2 pre-existing unrelated fixture failures), tests/contract passing for both variants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
saileshwar-skyflow
force-pushed
the
saileshwar/SK-2954-flowdb-vaut-apis-support
branch
from
July 7, 2026 11:01
57c72e0 to
cadf5d4
Compare
Distribution name was already skyflow-flowvault (setup.py) but the importable package stayed skyflow, colliding with v2's skyflow import name if both are ever installed in the same environment. Rename the v3 directory to flowvault and its package to skyflow_flowvault to match the distribution name and remove the collision. generated/ content is left untouched (Fern-owned).
…d base insert response Consolidates duplicated logic between v2 (PDB) and flowvault per architecture review: shared validation (vault config, credentials, log level), LogLevel/Logger, and insert field/table validation now live in common with per-variant message injection; adds BaseInsertResponse alongside BaseInsertRequest so each variant's InsertRequest/InsertResponse can extend a common base while keeping its own shape. Also fixes flowvault's insert() response shape (drop redundant 'data'/'table', flatten tokens, errors=None when empty) and a stale SDK_VERSION drift bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…insert() Splits BaseSkyflow, BaseVaultController, and BaseVaultClient into a pure interface (ISkyflow/IVaultController/IVaultClient, declaring the contract via ABC + abstractmethod) plus a base class implementing the shared logic, so future variant-specific overrides have a clear contract to satisfy. Also adds type hints to insert() at every layer (BaseInsertRequest/BaseInsertResponse in the abstract method, each variant's own InsertRequest/InsertResponse in their concrete override), and renames base_vault.py to base_vault_controller.py to match its class name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…shape, add types BaseInsertRequest's shared field is now named values (matching PDB's terminology) instead of records, and table/values are required rather than defaulted; each variant's InsertRequest forwards them explicitly. Removes flowvault's Upsert class in favor of a plain dict (now typed via a TypedDict) to match the rest of flowvault's dict-based request shape, and adds return type hints to every method on v2's VaultController to match insert's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BaseSkyflow implements every ISkyflow abstract method, so ABC alone doesn't block instantiating it directly -- only make_skyflow_class-produced subclasses should be constructed. Raises SkyflowError with a new SkyflowMessages entry instead of a raw NotImplementedError, matching how every other SDK error is raised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI workflows (ci.yml, main.yml, release.yml, beta-release.yml, internal-release.yml) still referenced the pre-rename v3/ directory, so every flowvault CI job failed outright. Threads a package-name (skyflow vs skyflow_flowvault) through shared-tests.yml/shared-build-and-deploy.yml/ bump_version.sh, since those hardcoded the skyflow package name too -- a real release would've bumped the wrong version file. Also fixes common/setup.py's missing python-dotenv dependency (test-common CI job installs only this file's declared deps), and removes the empty-value insert tests now that empty/null field values are explicitly allowed rather than rejected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… noise
flowvault/requirements.txt was missing coverage, so the v3 test job's
`python -m coverage run` step failed outright with "No module named
coverage" once the workflow correctly pointed at flowvault. Also excludes
**/generated/** (Fern-owned) from semgrep, since the generated REST clients
trip its secret-detection heuristics on parameter names like `token`, and
fixes a real semgrep finding: shared-build-and-deploy.yml interpolated
${{ }} context values directly into a run: shell block instead of routing
them through env: first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e positives test-common never ran coverage or uploaded to Codecov, so common/ (which grew substantially this session) was invisible to Codecov's patch/project checks. Adds a coverage run + Codecov upload step matching v2/flowvault's pattern. Also fixes .semgreprules/customRule.yml's check-sensitive-info regex: an optional quote-capture group let its own backreference match empty string, so any `keyword: value` matched regardless of quoting -- tightened to require an actual quoted literal and exclude self-referential values (e.g. TOKEN = 'token'), which eliminates 28 false positives without any inline suppressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Devesh-Skyflow
approved these changes
Jul 13, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| raise NotImplementedError | ||
|
|
||
|
|
||
| class BaseSkyflow(ISkyflow): |
Collaborator
There was a problem hiding this comment.
do we need base class here? if so ISkyflow interface and BaseSkyflow doesnt match, both have to base or without base, not mix
| return cls.Builder() | ||
|
|
||
| def add_vault_config(self, config): | ||
| self.__builder._Builder__add_vault_config(config) |
Collaborator
There was a problem hiding this comment.
why is the first builder is b and second is B
…tion/detect structurally absent when unsupported Renames ISkyflow -> BaseSkyflow (pure interface) and the old BaseSkyflow -> BaseSkyflowImpl (concrete). Moves connection/detect support into ConnectionCapable/DetectCapable interfaces + ConnectionMixin/DetectMixin in a new common/client/utils/_utils.py, conditionally composed into a variant's Skyflow class by make_skyflow_class() so unsupported variants (e.g. flowvault) genuinely lack .connection()/.detect() (AttributeError) instead of raising NotImplementedError from a present-but-guarded method. Also fixes two review-flagged bugs: adding a vault/connection config with a duplicate id to an already-built client now raises SkyflowError instead of silently overwriting the existing entry, and update_connection_config no longer risks a bare KeyError on a missing connection_id. Extracts the Builder's raw NotImplementedError string literals into named constants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
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.
Why
Splits skyflow-python from a single-variant SDK into three build variants sharing a bundled
common/module:common/— shared credential resolution, vault-URL resolution, bearer-token fetch/cache/expiry, validation, logging, and client-composition logic. Never published independently; bundled into each variant's wheel at build time.v2/— today's SDK, relocated from the repo root. Public API unchanged — same class names, signatures, import paths.flowvault/(renamed fromv3/) — new, built on the flowservice/flowdb API. Insert-only this round by design; every other operation is deliberately out of scope until a follow-up round (tracked in SK-2972). Publishes asskyflow-flowvault, importable asskyflow_flowvault, starting at1.0.0.common/— interfaces and shared logicISkyflow/IVaultController/IVaultClient: pure interfaces (ABC+@abstractmethod, no implementation) declaring the contract each variant must satisfy.BaseSkyflow/BaseVaultController/BaseVaultClientimplement them with the logic shared by every variant — protected helpers like_validate_table_name_if_present/_validate_field_valueslive on the base, per-variant behavior (insert/get/... ,resolve_vault_url/initialize_api_client) stays abstract for concrete subclasses to fill in.make_skyflow_class(...): a factory function that builds each variant's concreteSkyflowclass from injected collaborators (VaultClient,VaultController, its messages catalog, validators). Each variant's ownclient/skyflow.pyis now a handful of lines calling this factory with its own classes — no variant-specific wiring logic lives incommon, andcommonnever imports v2 or flowvault.validate_vault_config,validate_update_vault_config,validate_log_level,validate_credentials,LogLevel,Loggerwere confirmed field-for-field identical between v2 and flowvault and are now single implementations incommon, using amessages=dependency-injection parameter so each variant's own error text/SDK version still renders correctly (common has no SDK version of its own).BaseInsertRequest/BaseInsertResponse: shared base types for insert's request/response shape.table/valuesare required (no silent default) on the base;upsert's concrete type is left generic on the base (Union[str, dict]) since it's a plain string for v2 and a dict for flowvault — each variant's ownInsertRequest/InsertResponsesubclass declares its exact type.BaseSkyflowand the interface classes guard against direct instantiation via aSkyflowError(not a bareNotImplementedError), matching how every other SDK error is raised.v2/Vaultremains a backward-compatible alias for the internalVaultControllername.InsertRequestnow extendsBaseInsertRequest; every controller method (insert,get,update,delete,query,detokenize,tokenize,upload_file) has full request/response type hints.Envenum was failing cross-class comparisons againstcommon'sEnvbecause they were two structurally-identical-but-distinct enum classes — v2's is now a re-export ofcommon's.flowvault/— insert supportInsertRequest(values, table=None, upsert=None)—valuesis a plain list of dicts (each shaped{"values": {...}, "table": ..., "upsert": ...}), matching PDB's own naming/shape convention rather than a customInsertRecordclass.upsert(at either the request or per-record level) is a plain dict, typed via aTypedDict(Upsert) rather than a class —type(upsert)is exactlydictat runtime, matching how records are handled.InsertResponseshape was iterated against a real vault until it matched PDB's own shape exactly: no redundantdata/tablekeys, tokens flattened directly onto each result dict (no nestedtokenskey),errors=None(not[]) when there are none, each entry tagged withrequest_index(stable position in the originalvalueslist).Validations.java: table/upsert must live in exactly one place (request-level applying to every record, or per-record — never mixed, never both, confirmed against a real vault's 400 response), 10,000-record cap, and (post-review) empty/null field values are explicitly allowed — only empty/null keys are rejected.x-request-idonto each. All records for a request go out in a single API call (no batching this round).SDK_VERSIONwas hardcoded/stale (0.1.0) instead of matchingsetup.py's real version (1.0.0).Skyflow/VaultController/VaultClientare built viamake_skyflow_class/extendcommon's base classes — no flowvault-specific wiring logic beyond supplying its own collaborators.CI/CD
main,ci,beta-release,internal-release,release, and the two shared reusable workflows) now correctly targetsflowvault/(the workflows were still pointing at the pre-renamev3/directory, which meant every flowvault CI job failed outright —ci-scripts/bump_version.shand the shared workflows now also take apackage-nameinput, since they'd hardcoded theskyflowpackage name too and would've silently bumped/measured the wrong files for flowvault).test-commonnow actually runs coverage and uploads to Codecov, matching v2/flowvault's own pattern — it previously ranunittest discoverwith no coverage tracking at all, makingcommon/'s substantial growth this round invisible to Codecov's patch/project checks..semgreprules/customRule.yml'scheck-sensitive-inforegex: an optional quote-capture group let its own backreference match an empty string, so the rule fired on anykeyword: valueregardless of quoting — flagging things liketoken=OPTIONAL_TOKENorCredentialField.TOKENas "sensitive info". Tightened to require an actual quoted literal and exclude self-referential values (TOKEN = 'token'), eliminating 28 false positives with no inline suppressions. Also excludes**/generated/**(Fern-owned code) from semgrep, and fixed a real finding — a${{ }}GitHub Actions context value was interpolated directly into arun:shell block instead of being routed throughenv:first.Testing
common/tests,v2/tests,flowvault/tests,tests/contract/all pass locally with no regressions from this round's changes.Known follow-ups (intentionally out of scope here)
get/update/delete/query/detokenizeare stubs (NotImplementedError) — tracked in SK-2972, one operation at a time, same pattern as this round's insert.flowvault-tag/branch push since some of the release automation couldn't be dry-run outside GitHub's own infrastructure.