Skip to content

fix(sku): add DB CHECK backstop rejecting empty SKU ID#2300

Merged
ajf merged 2 commits into
NVIDIA:mainfrom
wminckler:sku-bug
Jun 10, 2026
Merged

fix(sku): add DB CHECK backstop rejecting empty SKU ID#2300
ajf merged 2 commits into
NVIDIA:mainfrom
wminckler:sku-bug

Conversation

@wminckler

@wminckler wminckler commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

The code-level guard in db::sku::create/replace rejects empty ids, but only on those two paths. The machine_skus PRIMARY KEY blocks NULL yet accepts the empty string, so an empty id could still reach the table via any unvalidated write path.

Add a CHECK (id <> '') constraint so the database enforces the invariant regardless of caller. Any pre-existing empty-id SKU is preserved: it is reassigned a generated id and referencing machines are repointed before the constraint is applied, so the migration is non-destructive.

Verified via the sqlx_test harness, which rebuilds the migrated template DB from scratch and runs test_sku_create_rejects_empty_id green.

Description

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

The code-level guard in db::sku::create/replace rejects empty ids, but
only on those two paths. The machine_skus PRIMARY KEY blocks NULL yet
accepts the empty string, so an empty id could still reach the table via
any unvalidated write path.

Add a CHECK (id <> '') constraint so the database enforces the invariant
regardless of caller. Any pre-existing empty-id SKU is preserved: it is
reassigned a generated id and referencing machines are repointed before
the constraint is applied, so the migration is non-destructive.

Verified via the sqlx_test harness, which rebuilds the migrated template
DB from scratch and runs test_sku_create_rejects_empty_id green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@wminckler wminckler requested a review from ajf June 8, 2026 16:48
@copy-pr-bot

copy-pr-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9400983b-e448-4681-a478-2ec12710b8df

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@wminckler

Copy link
Copy Markdown
Contributor Author

draft because I want to run the migration manually in my dev-env

@wminckler wminckler marked this pull request as ready for review June 8, 2026 17:00
@wminckler wminckler requested a review from a team as a code owner June 8, 2026 17:00
@wminckler

wminckler commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

draft because I want to run the migration manually in my dev-env

done. also verified that the check works by running the migration in an env that doesn't have the code fix in #2253

@ajf ajf merged commit 51827fe into NVIDIA:main Jun 10, 2026
53 checks passed
@ajf ajf added this to the v2.0 milestone Jun 10, 2026
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.

2 participants