Skip to content

Sync with InsForge-sdk-js v1.5.0#6

Open
Fermionic-Lyu wants to merge 1 commit into
mainfrom
sdk-sync/v1.5.0
Open

Sync with InsForge-sdk-js v1.5.0#6
Fermionic-Lyu wants to merge 1 commit into
mainfrom
sdk-sync/v1.5.0

Conversation

@Fermionic-Lyu

@Fermionic-Lyu Fermionic-Lyu commented Jul 21, 2026

Copy link
Copy Markdown
Member

Ports the storage changes from InsForge-sdk-js v1.5.0 (diff v1.4.5..v1.5.0), paired with the backend change in InsForge/InsForge#1760.

What was ported

  • upload(path, bytes) — standard PUT create-or-replace semantics. Uploading to a key that already exists now replaces the object in place; previously the server silently auto-renamed the key (photo.pngphoto (1).png). Method signature unchanged — runtime behavior change only. Doc comments updated to state the new contract.
  • uploadAutoKey(filename, bytes) — client-side key minting. Now generates a unique, collision-free key client-side (<sanitized-base>-<timestamp>-<random><ext>, mirroring the JS generateObjectKey exactly: invalid chars → -, base truncated to 32 chars, file fallback, extension preserved) and uploads through the standard upload() path via PUT, instead of POSTing to /objects for a server-generated key — the storage API no longer mints keys server-side.
  • Deprecated the now no-op upsert flag on upload, uploadAutoKey, and FileOptions (this repo's Dart-specific knob; the JS SDK never had it). Since PUT always replaces, the flag does nothing — the x-upsert header is no longer sent. Kept in the signatures for source compatibility with @Deprecated.
  • Tests: rewrote/extended file_upload_test.dart (PUT to client-minted key, sanitization, no-extension and empty-filename fallbacks, distinct keys for repeated uploads, upsert no-op) mirroring the new JS unit tests; updated the integration lifecycle test to assert replace-in-place on re-upload to the same key, and the uploadAutoKey integration test to expect a client-generated key.
  • Docs/versioning (repo already maintains them): bumped both packages 0.1.0 → 0.2.0 in lockstep via dart run tool/set_version.dart 0.2.0 (regenerates version.dart), updated root + per-package changelogs, updated the root README storage snippet, and bumped the internal insforge/insforge_flutter dep constraints in integration_tests/ and samples/twitter_app/ so the workspace resolves.

What was skipped and why

  • JS storage.test.ts blocks covering getPublicUrl encoding, createSignedUrl/createSignedUrls, and the legacy POST download-strategy fallback — pre-existing JS surface/tests unrelated to the v1.5.0 API change (this SDK has no signed-URL helpers to sync here).
  • .github/workflows/*, package.json/package-lock.json — JS repo plumbing.
  • SDK-REFERENCE.md one-liner — no equivalent reference doc here; the same note went into the Dart doc comments and README instead.

Notes

Test results

Ran the full CI sequence locally with Flutter 3.44.7 stable (downloaded into the workspace; no toolchain was preinstalled):

  • dart format --output=none --set-exit-if-changed . — clean
  • dart analyze . — no issues
  • dart run tool/set_version.dart --check — 0.2.0 in sync across both packages + version.dart
  • packages/insforge: dart test185/185 passed
  • packages/insforge_flutter: flutter analyze — no issues; flutter test5/5 passed
  • dart pub publish --dry-run on both packages — only the expected dirty-git-state warning pre-commit

Live integration tests (integration_tests/) were not run — they are opt-in and need a real project via INSFORGE_INTEGRATION_* env vars, which are not available here.

🤖 Generated with Claude Code


Summary by cubic

Syncs storage with InsForge JS SDK v1.5.0: upload now uses standard PUT replace semantics, and uploadAutoKey generates unique keys client‑side. Bumps insforge and insforge_flutter to 0.2.0 with updated docs and tests.

  • New Features

    • upload(path, bytes): re-uploads replace in place (no auto-rename).
    • uploadAutoKey(filename, bytes): mints a unique key client-side (<sanitized-base>-<timestamp>-<random><ext>) and PUTs to it.
    • Deprecated no-op upsert flag (header removed); signatures kept with @Deprecated.
    • Tests updated for replace semantics, client-key minting, sanitization, and no x-upsert.
  • Migration

    • Requires a backend that supports standard PUT storage behavior.
    • Stop relying on server auto-rename; re-uploads overwrite the same key.
    • Remove any use of upsert (it’s ignored).
    • Update dependencies to insforge 0.2.0 and insforge_flutter 0.2.0.

Written for commit 1232198. Summary will update on new commits.

Review in cubic

Ports the v1.4.5..v1.5.0 storage changes (paired with the backend change
in InsForge/InsForge#1760):

- upload(path, bytes): uploading to an existing key now replaces the
  object in place (standard PUT semantics) instead of the server
  auto-renaming the key. Signature unchanged.
- uploadAutoKey(filename, bytes): generates a unique, collision-free key
  client-side (sanitized base + timestamp + random suffix) and uploads
  through the standard upload() path — the backend no longer mints keys
  server-side via POST /objects.
- Deprecate the now no-op upsert flag on upload/uploadAutoKey/FileOptions;
  the x-upsert header is no longer sent.
- Update unit + integration tests, READMEs, changelogs; bump both
  packages to 0.2.0 in lockstep (tool/set_version.dart).

Requires an InsForge backend that includes the standard-PUT storage
change; do not run against a pre-change backend.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code review — Sync with InsForge-sdk-js v1.5.0

Summary: A clean, well-scoped port of the JS SDK v1.5.0 storage change (standard PUT create-or-replace + client-side key minting, deprecated upsert), with tests and versioning updated in lockstep — no blocking issues.

Requirements context

The repo keeps plans/specs under docs/superpowers/. The relevant artifacts (plans/2026-06-08-insforge-flutter-sdk-04-storage.md, specs/2026-06-08-insforge-flutter-sdk-design.md) describe the original v0.1.0 storage design (server-minted auto-key via POST /objects, x-upsert header). This PR deliberately supersedes that design to match the new backend contract, so there is no dedicated spec for this sync — I assessed against the PR description, the paired backend change (InsForge/InsForge#1760), and the upstream source. To verify the claimed exact parity I fetched the real generateObjectKey from InsForge-sdk-js at tag v1.5.0 (src/modules/storage.ts) and compared it line-by-line against the Dart port.

Findings

Critical

(none)

Suggestion

  • Functionality — uploadAutoKey forwards metadata into a sink (packages/insforge/lib/src/storage/storage_file_api.dart:112-130, :83-102): uploadAutoKey now passes metadata: metadata down to upload, but upload never serializes metadata into the request (it only builds the single file form part). So metadata is silently dropped for both entry points. This is pre-existing (the old upload already ignored it) and out of this PR's stated scope, but the new explicit forwarding makes the dead parameter more visible/misleading. Consider either wiring metadata through (e.g. as form fields the backend accepts) or removing it from the signatures in a follow-up.

Information

  • Software engineering — JS parity confirmed, with one benign deviation (storage_file_api.dart:36-54): the Dart _generateObjectKey matches JS generateObjectKey@v1.5.0 on all observable behavior — dotIndex = lastIndexOf('.'), hasExt = dotIndex > 0, extension preserved, sanitize [^a-zA-Z0-9_-] → '-', truncate-to-32-then-'file'-fallback ordering, ms timestamp, 6-char [0-9a-z] suffix. One deviation: JS derives the suffix from Math.random().toString(36).slice(2,8), which can occasionally yield fewer than 6 chars; the Dart version always emits exactly 6 (arguably more robust). Cross-SDK keys can never be byte-identical anyway (random), so this is fine — noting it because the PR body says "mirroring … exactly."
  • Security — key randomness (storage_file_api.dart:28): _keyRandom = Random() is non-cryptographic. That is correct here — object keys are collision-avoidance identifiers, not capability tokens (they're echoed by getPublicUrl), and it matches the JS Math.random() source. No change needed.
  • Housekeeping: the historical planning docs (docs/superpowers/plans/...-04-storage.md, design spec) still describe the old x-upsert/server-minted-POST behavior. These read as point-in-time planning artifacts rather than maintained reference docs, and the PR correctly updated the living surfaces (doc comments, README, changelogs). No action required, but a one-line "superseded by v0.2.0 sync" note in the plan could prevent future confusion.

Dimension coverage

  • Software engineering: Strong. Tests were rewritten to mirror the new JS unit tests — PUT to a client-minted key, filename sanitization, no-extension and empty-filename 'file' fallback, distinct keys across repeated uploads, and the upsert no-op (no x-upsert header). Integration test updated to assert replace-in-place and a client-generated key. @Deprecated annotations are applied consistently with matching // ignore: in same-package tests. Conventions (import order, dart:math addition, doc-comment style) match surrounding code.
  • Functionality: Solves the stated goal. upload now PUTs directly (create-or-replace); uploadAutoKey mints the key client-side and delegates to upload, so repeated uploads no longer collide/overwrite. upsert retained for source-compat as a documented no-op. Sanitized auto-keys contain only [a-zA-Z0-9_.-], so the unencoded .../objects/$path interpolation is safe for auto keys (the lack of encodeURIComponent on user-supplied upload paths is pre-existing and intentional — nested / keys).
  • Security: No new attack surface. No secrets/PII newly logged or returned, no auth/authorization changes, no new dependencies (only dart:math). See key-randomness note above.
  • Performance: No regressions — no new network round-trips (auto-key path drops the previous server POST and reuses upload), no loops, no large allocations. Key generation is O(filename).

Verdict

approved (informational — zero Critical findings; explicit GitHub approval remains a human action). Version bumps (0.2.0 across version.dart, both pubspec.yaml, dep constraints, samples, integration_tests) and the backend-coordination caveat are consistent and correctly documented.

🤖 Automated review by the InsForge review bot.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@cubic-dev-ai cubic-dev-ai Bot 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.

2 issues found across 14 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/insforge/lib/src/storage/storage_file_api.dart">

<violation number="1" location="packages/insforge/lib/src/storage/storage_file_api.dart:53">
P2: Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.</violation>
</file>

<file name="CHANGELOG.md">

<violation number="1" location="CHANGELOG.md:22">
P2: Repeated `uploadAutoKey` calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

6,
(_) => _keyAlphabet[_keyRandom.nextInt(_keyAlphabet.length)],
).join();
return '$sanitizedBase-$timestamp-$random$ext';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/insforge/lib/src/storage/storage_file_api.dart, line 53:

<comment>Filenames containing URL-significant characters can upload under a truncated or different key because the extension is inserted into the raw object URL without sanitization. Preserving normal extensions while replacing characters outside the URL-safe key alphabet would keep the generated key addressable.</comment>

<file context>
@@ -23,6 +24,35 @@ class StorageFileApi {
+      6,
+      (_) => _keyAlphabet[_keyRandom.nextInt(_keyAlphabet.length)],
+    ).join();
+    return '$sanitizedBase-$timestamp-$random$ext';
+  }
+
</file context>
Suggested change
return '$sanitizedBase-$timestamp-$random$ext';
return '$sanitizedBase-$timestamp-$random${ext.replaceAll(RegExp('[^a-zA-Z0-9._-]'), '-')}';

Comment thread CHANGELOG.md
method signature is unchanged; the friendly auto-rename UX now lives in the
InsForge dashboard rather than the API.
- **Storage `uploadAutoKey(filename, bytes)`** — now generates a unique,
collision-free key client-side (sanitized base + timestamp + random suffix)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Repeated uploadAutoKey calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 22:

<comment>Repeated `uploadAutoKey` calls are not guaranteed collision-free: a random/key collision will make the replacing PUT overwrite the existing object. The changelog should describe this as best-effort/low-collision unless key existence checking and retry are added.</comment>

<file context>
@@ -4,6 +4,33 @@ All notable changes to the InsForge Dart/Flutter SDK are documented here. This
+  method signature is unchanged; the friendly auto-rename UX now lives in the
+  InsForge dashboard rather than the API.
+- **Storage `uploadAutoKey(filename, bytes)`** — now generates a unique,
+  collision-free key client-side (sanitized base + timestamp + random suffix)
+  and uploads through the standard `upload` path, so repeated uploads of the
+  same file never overwrite each other. The backend no longer mints keys
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants