STINGS MIGRATIONS SASHI and also add some to scope one#2
Conversation
📝 WalkthroughWalkthroughThis PR updates Luau database type signatures to require explicit ChangesDatabase Type Signatures
Stings Data Migration
Migration Orchestration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: rhit-sashiks <186237416+rhit-sashiks@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@luau/twshell/migrations/stings.luau`:
- Around line 39-43: The migration is skipping rows when userid is missing (see
the missing_userid increment and continue) but later unconditionally deletes all
rows in OLD_SCOPE (lines around the OLD_SCOPE deletion), causing data loss;
change the deletion logic so only rows that were actually migrated are
removed—e.g., accumulate migrated keys (or mark processed rows) when handling
key/owner_type/owner_id/scopes and then run DELETE only for those keys (or add
WHERE userid IS NOT NULL and match the processed key set) instead of deleting
all OLD_SCOPE rows; ensure references to missing_userid, key, owner_type,
owner_id, scopes, and OLD_SCOPE are used to identify which rows to keep vs
delete.
- Around line 25-29: The PgRow.get calls are using 0-based indexes but the API
is 1-based; update the row:get(...) calls for the migration tuple unpacking (the
locals owner_id, owner_type, key, scopes, value) from 0..4 to 1..5 (and do the
same for the second occurrence later around the other unpack block at lines
68-72) so each call uses the correct 1-based column index; search for any other
row:get(...) in this migration and adjust similarly to ensure all column
accesses use 1-based indices.
In `@src/migrations/mod.rs`:
- Around line 125-130: The Luau migrations pulled from MigrationsFolder::iter()
are not ordered, causing nondeterministic migration sequences; collect the
iterator results into a Vec, sort them deterministically (e.g.,
lexicographically by the embedded file name), then map them to
MigrationType::Luau and append to base_migrations (which already contains
RUST_MIGRATIONS_BEFORE_LUAU mapped to MigrationType::Rust) so that the Luau
entries are always applied in a stable, reproducible order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3397a90a-f481-4b17-bfd5-c076bf1fdf43
📒 Files selected for processing (4)
luau/twshell/common/db.luauluau/twshell/migrations/stings.luausrc/migrations/kv_scope_unnest.rssrc/migrations/mod.rs
| local owner_id = row:get(0, "string"):take() :: string | ||
| local owner_type = row:get(1, "string"):take() :: string | ||
| local key = row:get(2, "string"):take() :: string | ||
| local scopes = row:get(3, "{string}"):take() :: {string} | ||
| local value = row:get(4, "custom@khronosvalue"):take() :: any |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify API contract and call-site mismatch (read-only).
rg -n 'get:\s*\(self: PgRow, index: number' luau/twshell/common/db.luau
rg -n 'row:get\([0-9]+' luau/twshell/migrations/stings.luauRepository: Anti-Raid/template-worker
Length of output: 880
🏁 Script executed:
# Read context around the PgRow.get function definition in db.luau
cat -n luau/twshell/common/db.luau | sed -n '25,45p'Repository: Anti-Raid/template-worker
Length of output: 852
🏁 Script executed:
# Search for all usages of PgRow.get across the codebase to understand indexing patterns
rg -A 2 -B 2 'row:get\(' luau/twshell/ --type luaRepository: Anti-Raid/template-worker
Length of output: 51
PgRow.get indexes are off by one (0-based used, API is 1-based).
Lines 25-29 and 68-72 use row:get(0, ...) through row:get(4, ...), but PgRow.get is documented as 1-based. This will read from wrong columns during migration.
🔧 Proposed fix
- local owner_id = row:get(0, "string"):take() :: string
- local owner_type = row:get(1, "string"):take() :: string
- local key = row:get(2, "string"):take() :: string
- local scopes = row:get(3, "{string}"):take() :: {string}
- local value = row:get(4, "custom@khronosvalue"):take() :: any
+ local owner_id = row:get(1, "string"):take() :: string
+ local owner_type = row:get(2, "string"):take() :: string
+ local key = row:get(3, "string"):take() :: string
+ local scopes = row:get(4, "{string}"):take() :: {string}
+ local value = row:get(5, "custom@khronosvalue"):take() :: any
@@
- local id = row:get(0, "string"):take() :: string
- local owner_id = row:get(1, "string"):take() :: string
- local owner_type = row:get(2, "string"):take() :: string
- local key = row:get(3, "string"):take() :: string
- local value = row:get(4, "custom@khronosvalue"):take() :: any
+ local id = row:get(1, "string"):take() :: string
+ local owner_id = row:get(2, "string"):take() :: string
+ local owner_type = row:get(3, "string"):take() :: string
+ local key = row:get(4, "string"):take() :: string
+ local value = row:get(5, "custom@khronosvalue"):take() :: any📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local owner_id = row:get(0, "string"):take() :: string | |
| local owner_type = row:get(1, "string"):take() :: string | |
| local key = row:get(2, "string"):take() :: string | |
| local scopes = row:get(3, "{string}"):take() :: {string} | |
| local value = row:get(4, "custom@khronosvalue"):take() :: any | |
| local owner_id = row:get(1, "string"):take() :: string | |
| local owner_type = row:get(2, "string"):take() :: string | |
| local key = row:get(3, "string"):take() :: string | |
| local scopes = row:get(4, "{string}"):take() :: {string} | |
| local value = row:get(5, "custom@khronosvalue"):take() :: any |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@luau/twshell/migrations/stings.luau` around lines 25 - 29, The PgRow.get
calls are using 0-based indexes but the API is 1-based; update the row:get(...)
calls for the migration tuple unpacking (the locals owner_id, owner_type, key,
scopes, value) from 0..4 to 1..5 (and do the same for the second occurrence
later around the other unpack block at lines 68-72) so each call uses the
correct 1-based column index; search for any other row:get(...) in this
migration and adjust similarly to ensure all column accesses use 1-based
indices.
| if not userid then | ||
| print(string.format("[stings] sting key=%s owner=%s/%s has no userid in scopes %s; skipping", key, owner_type, owner_id, table.concat(scopes, ","))) | ||
| missing_userid += 1 | ||
| continue | ||
| end |
There was a problem hiding this comment.
Skipped rows are still deleted, causing data loss.
Line 42 skips rows with missing userid, but Line 138 deletes all OLD_SCOPE rows anyway. That drops records you explicitly did not migrate.
🔧 Proposed fix
- local deleted = tx:execute(
- "DELETE FROM tenant_kv WHERE $1 = ANY(scopes)",
- { db:cast(OLD_SCOPE, "string") }
- )
+ local deleted = tx:execute(
+ [[DELETE FROM tenant_kv
+ WHERE $1 = ANY(scopes)
+ AND EXISTS (
+ SELECT 1 FROM unnest(scopes) AS s WHERE s <> $1
+ )]],
+ { db:cast(OLD_SCOPE, "string") }
+ )Also applies to: 138-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@luau/twshell/migrations/stings.luau` around lines 39 - 43, The migration is
skipping rows when userid is missing (see the missing_userid increment and
continue) but later unconditionally deletes all rows in OLD_SCOPE (lines around
the OLD_SCOPE deletion), causing data loss; change the deletion logic so only
rows that were actually migrated are removed—e.g., accumulate migrated keys (or
mark processed rows) when handling key/owner_type/owner_id/scopes and then run
DELETE only for those keys (or add WHERE userid IS NOT NULL and match the
processed key set) instead of deleting all OLD_SCOPE rows; ensure references to
missing_userid, key, owner_type, owner_id, scopes, and OLD_SCOPE are used to
identify which rows to keep vs delete.
| let mut base_migrations = RUST_MIGRATIONS_BEFORE_LUAU.into_iter().map(MigrationType::Rust).collect::<Vec<_>>(); | ||
|
|
||
| for migration in RUST_MIGRATIONS { | ||
| base_migrations.push(MigrationType::Rust(migration)); | ||
| } | ||
|
|
||
| // Add luau migrations from MigrationsFolder after all base rust once have finished | ||
| // Add luau migrations from MigrationsFolder | ||
| for entry in MigrationsFolder::iter() { | ||
| base_migrations.push(MigrationType::Luau(entry)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does rust-embed (v8.x) guarantee sorted/deterministic order for RustEmbed::iter(), or is explicit sorting recommended for stable execution order?
💡 Result:
No. rust-embed (v8.x) does not explicitly guarantee a sorted/deterministic iteration order for RustEmbed::iter; it only documents that, depending on build mode, iter uses either a static array of relative file paths or lists files from the file system at runtime.[1][2] What can be inferred from the docs/source: - In release builds (or with the debug-embed feature), iter uses a “static array containing the list of relative file paths” and then iterates it via a slice iterator.[3] - In debug builds without debug-embed, iter “lists from the file system on each call,” so the order is whatever the underlying filesystem iteration returns for that platform/config.[1][2] - The public documentation you can rely on does not state that the embedded/compiled static array is sorted (e.g., lexicographically), nor does it promise deterministic ordering across builds/runs. Recommendation for stable execution order: - If you need stable ordering for any reason (tests, cache keys, reproducible outputs), explicitly sort the results yourself (e.g., collect into a Vec and sort by the path string) rather than relying on the order produced by iter. This is the safe approach given the documented “file system” case and the lack of a stated ordering guarantee for the static array.[1][2] Confidence: Medium (docs confirm dynamic vs static-array behavior, but do not explicitly state sorting/ordering guarantees for the static array; code inspection would be needed to confirm how the array is produced).
Citations:
- 1: https://lib.rs/crates/rust-embed
- 2: https://docs.rs/crate/rust-embed/latest
- 3: https://dev.to/xzdes/i-built-a-deep-learning-framework-in-rust-from-scratch-part-3-the-road-to-cratesio-jam
🏁 Script executed:
cd src/migrations && head -n 150 mod.rs | tail -n 30Repository: Anti-Raid/template-worker
Length of output: 1337
🏁 Script executed:
# Find MigrationsFolder definition
rg "MigrationsFolder" --type rust -B 2 -A 2Repository: Anti-Raid/template-worker
Length of output: 721
🏁 Script executed:
# Check how migrations are used downstream
rg "base_migrations" src/ --type rust -A 3 -B 1Repository: Anti-Raid/template-worker
Length of output: 1129
🏁 Script executed:
# Get more context around apply_migrations
rg "apply_migrations|for.*base_migrations" src/migrations/mod.rs -A 15 -B 2Repository: Anti-Raid/template-worker
Length of output: 659
🏁 Script executed:
# Get the full apply_migrations function to see how migrations are processed
sed -n '/^pub async fn apply_migrations/,/^}/p' src/migrations/mod.rs | head -n 80Repository: Anti-Raid/template-worker
Length of output: 2754
🏁 Script executed:
# Verify the exact location and syntax of the code being reviewed
sed -n '120,135p' src/migrations/mod.rsRepository: Anti-Raid/template-worker
Length of output: 754
🏁 Script executed:
# Check the type returned by MigrationsFolder::iter() and its traits
rg "impl.*Embed|impl.*Display" src/ -A 5 | grep -A 10 "MigrationsFolder\|DirEntry"Repository: Anti-Raid/template-worker
Length of output: 51
🏁 Script executed:
# Look for how Luau migrations (which are Cow<'static, str>) are used elsewhere
rg "MigrationType::Luau" src/ -B 2 -A 2Repository: Anti-Raid/template-worker
Length of output: 714
Sort Luau migrations for deterministic and reproducible execution order.
The before/after Rust migration split is sound, but Luau migrations should be sorted to ensure consistent ordering across builds. Since rust-embed::iter() provides no ordering guarantees (behaves differently between debug/release builds and filesystem-dependent during development), explicitly sorting prevents nondeterministic migration sequencing in databases where order matters.
🔧 Proposed fix
- // Add luau migrations from MigrationsFolder
- for entry in MigrationsFolder::iter() {
+ // Add luau migrations from MigrationsFolder (deterministic order)
+ let mut luau_migrations = MigrationsFolder::iter().collect::<Vec<_>>();
+ luau_migrations.sort();
+ for entry in luau_migrations {
base_migrations.push(MigrationType::Luau(entry));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut base_migrations = RUST_MIGRATIONS_BEFORE_LUAU.into_iter().map(MigrationType::Rust).collect::<Vec<_>>(); | |
| for migration in RUST_MIGRATIONS { | |
| base_migrations.push(MigrationType::Rust(migration)); | |
| } | |
| // Add luau migrations from MigrationsFolder after all base rust once have finished | |
| // Add luau migrations from MigrationsFolder | |
| for entry in MigrationsFolder::iter() { | |
| base_migrations.push(MigrationType::Luau(entry)); | |
| } | |
| let mut base_migrations = RUST_MIGRATIONS_BEFORE_LUAU.into_iter().map(MigrationType::Rust).collect::<Vec<_>>(); | |
| // Add luau migrations from MigrationsFolder (deterministic order) | |
| let mut luau_migrations = MigrationsFolder::iter().collect::<Vec<_>>(); | |
| luau_migrations.sort(); | |
| for entry in luau_migrations { | |
| base_migrations.push(MigrationType::Luau(entry)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/migrations/mod.rs` around lines 125 - 130, The Luau migrations pulled
from MigrationsFolder::iter() are not ordered, causing nondeterministic
migration sequences; collect the iterator results into a Vec, sort them
deterministically (e.g., lexicographically by the embedded file name), then map
them to MigrationType::Luau and append to base_migrations (which already
contains RUST_MIGRATIONS_BEFORE_LUAU mapped to MigrationType::Rust) so that the
Luau entries are always applied in a stable, reproducible order.
Resolved in commit |
|
@jack-e-tanner actually, can you fix the migration stuff to be deterministic order too with stings |
LGTM
Summary by CodeRabbit