Skip to content

SK-2954: common/v2/flowvault split + flowvault insert support#269

Open
saileshwar-skyflow wants to merge 11 commits into
mainfrom
saileshwar/SK-2954-flowdb-vaut-apis-support
Open

SK-2954: common/v2/flowvault split + flowvault insert support#269
saileshwar-skyflow wants to merge 11 commits into
mainfrom
saileshwar/SK-2954-flowdb-vaut-apis-support

Conversation

@saileshwar-skyflow

@saileshwar-skyflow saileshwar-skyflow commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

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 from v3/) — 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 as skyflow-flowvault, importable as skyflow_flowvault, starting at 1.0.0.

common/ — interfaces and shared logic

  • ISkyflow/IVaultController/IVaultClient: pure interfaces (ABC + @abstractmethod, no implementation) declaring the contract each variant must satisfy. BaseSkyflow/BaseVaultController/BaseVaultClient implement them with the logic shared by every variant — protected helpers like _validate_table_name_if_present/_validate_field_values live 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 concrete Skyflow class from injected collaborators (VaultClient, VaultController, its messages catalog, validators). Each variant's own client/skyflow.py is now a handful of lines calling this factory with its own classes — no variant-specific wiring logic lives in common, and common never imports v2 or flowvault.
  • Unified validation/logging: validate_vault_config, validate_update_vault_config, validate_log_level, validate_credentials, LogLevel, Logger were confirmed field-for-field identical between v2 and flowvault and are now single implementations in common, using a messages= 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/values are 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 own InsertRequest/InsertResponse subclass declares its exact type.
  • BaseSkyflow and the interface classes guard against direct instantiation via a SkyflowError (not a bare NotImplementedError), matching how every other SDK error is raised.

v2/

  • Structural relocation, zero behavior change to the public contract — same class names, signatures, import paths. Vault remains a backward-compatible alias for the internal VaultController name.
  • InsertRequest now extends BaseInsertRequest; every controller method (insert, get, update, delete, query, detokenize, tokenize, upload_file) has full request/response type hints.
  • Fixed a real bug surfaced during this work: v2's own Env enum was failing cross-class comparisons against common's Env because they were two structurally-identical-but-distinct enum classes — v2's is now a re-export of common's.

flowvault/ — insert support

  • Request shape: InsertRequest(values, table=None, upsert=None)values is a plain list of dicts (each shaped {"values": {...}, "table": ..., "upsert": ...}), matching PDB's own naming/shape convention rather than a custom InsertRecord class. upsert (at either the request or per-record level) is a plain dict, typed via a TypedDict (Upsert) rather than a class — type(upsert) is exactly dict at runtime, matching how records are handled.
  • InsertResponse shape was iterated against a real vault until it matched PDB's own shape exactly: no redundant data/table keys, tokens flattened directly onto each result dict (no nested tokens key), errors=None (not []) when there are none, each entry tagged with request_index (stable position in the original values list).
  • Validation ported from Java's v3 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.
  • Error handling: parses the backend's structured per-record error body into individual entries, propagating x-request-id onto each. All records for a request go out in a single API call (no batching this round).
  • Fixed a real, live-vault-confirmed bug: SDK_VERSION was hardcoded/stale (0.1.0) instead of matching setup.py's real version (1.0.0).
  • Skyflow/VaultController/VaultClient are built via make_skyflow_class/extend common's base classes — no flowvault-specific wiring logic beyond supplying its own collaborators.

CI/CD

  • Every workflow (main, ci, beta-release, internal-release, release, and the two shared reusable workflows) now correctly targets flowvault/ (the workflows were still pointing at the pre-rename v3/ directory, which meant every flowvault CI job failed outright — ci-scripts/bump_version.sh and the shared workflows now also take a package-name input, since they'd hardcoded the skyflow package name too and would've silently bumped/measured the wrong files for flowvault).
  • test-common now actually runs coverage and uploads to Codecov, matching v2/flowvault's own pattern — it previously ran unittest discover with no coverage tracking at all, making common/'s substantial growth this round invisible to Codecov's patch/project checks.
  • Fixed .semgreprules/customRule.yml's check-sensitive-info regex: an optional quote-capture group let its own backreference match an empty string, so the rule fired on any keyword: value regardless of quoting — flagging things like token=OPTIONAL_TOKEN or CredentialField.TOKEN as "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 a run: shell block instead of being routed through env: first.

Testing

  • common/tests, v2/tests, flowvault/tests, tests/contract/ all pass locally with no regressions from this round's changes.
  • Manually verified end-to-end against a real flowdb/flowvault vault multiple times through this round (SSL, auth, vault-URL resolution, exact response/error shapes all confirmed live, not just mocked).

Known follow-ups (intentionally out of scope here)

  • flowvault's get/update/delete/query/detokenize are stubs (NotImplementedError) — tracked in SK-2972, one operation at a time, same pattern as this round's insert.
  • CI: JFrog Artifactory publish target is reused as-is for both variants; worth a careful look before the first real flowvault- tag/branch push since some of the release automation couldn't be dry-run outside GitHub's own infrastructure.

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
saileshwar-skyflow force-pushed the saileshwar/SK-2954-flowdb-vaut-apis-support branch from 57c72e0 to cadf5d4 Compare July 7, 2026 11:01
saileshwar-skyflow and others added 2 commits July 8, 2026 00:34
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>
@saileshwar-skyflow saileshwar-skyflow changed the title SK-2954: common/v2/v3 split + v3 (flowvault) insert support SK-2954: common/v2/v3 split + flowvault insert support Jul 8, 2026
…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>
@saileshwar-skyflow saileshwar-skyflow changed the title SK-2954: common/v2/v3 split + flowvault insert support SK-2954: common/v2/flowvault split + flowvault insert support Jul 9, 2026
saileshwar-skyflow and others added 4 commits July 9, 2026 21:24
…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>
Comment thread common/service_account/client/auth_client.py Fixed
Comment thread common/utils/_utils.py Fixed
Comment thread common/utils/validations/_validations.py Fixed
Comment thread common/utils/validations/_validations.py Fixed
Comment thread common/utils/validations/_validations.py Fixed
Comment thread v2/skyflow/utils/constants.py Fixed
Comment thread v2/skyflow/utils/constants.py Fixed
Comment thread v2/skyflow/utils/constants.py Fixed
Comment thread v2/skyflow/utils/enums/detect_entities.py Fixed
Comment thread v2/skyflow/vault/client/client.py Fixed
…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>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread common/client/base_skyflow.py Outdated
raise NotImplementedError


class BaseSkyflow(ISkyflow):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need base class here? if so ISkyflow interface and BaseSkyflow doesnt match, both have to base or without base, not mix

Comment thread common/client/base_skyflow.py Outdated
return cls.Builder()

def add_vault_config(self, config):
self.__builder._Builder__add_vault_config(config)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants