init computed deployment variable value #516
Conversation
WalkthroughThe schema for the Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/db/src/schema/deployment-variables.ts (2)
86-91: Strengthen thevalid_value_typeCHECK to forbid empty arrays and blank text.Current predicate allows:
INSERT … (value_type='reference', reference='', path='{}');Both
reference = ''andpath = '{}'are probably invalid.
Appending additional predicates keeps the intent tight:- (value_type = 'reference' AND value IS NULL AND reference IS NOT NULL AND path IS NOT NULL) + (value_type = 'reference' + AND value IS NULL + AND reference <> '' + AND path IS NOT NULL + AND cardinality(path) > 0)Likewise you may want
jsonb_typeof(value) IS NOT NULLwhenvalueType='direct'.
78-79: JSONB unique index may cause performance & operator-class issues.PostgreSQL requires a b-tree operator class for
jsonb; without one the index will fail to create.
If uniqueness of raw JSON is really required, consider hashing:CREATE UNIQUE INDEX … ON deployment_variable_value(variable_id, md5(value::text));Drizzle:
uniqueIndex().on(t.variableId, sqlmd5(${t.value}::text)).If strict JSON equality is not essential, rethink the approach (see previous comment).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/db/src/schema/deployment-variables.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/deployment-variables.ts
🧬 Code Graph Analysis (1)
packages/db/src/schema/deployment-variables.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
| valueType: text("value_type").notNull().default("direct"), // 'direct' | 'reference' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider promoting valueType to a Postgres enum for stronger guarantees.
text("value_type") works, but misses out on type–level safety in the DB and allows silent typos ("diret" etc.).
Replacing it with a dedicated pgEnum("value_type", ["direct", "reference"]) (or re-using a shared enum) will:
- eliminate invalid literals at the DB layer,
- simplify the CHECK constraint,
- improve schema self-documentation.
Diff sketch:
-import {
- boolean,
- foreignKey,
- jsonb,
- pgTable,
- text,
- uniqueIndex,
- uuid,
-} from "drizzle-orm/pg-core";
+import {
+ boolean,
+ foreignKey,
+ jsonb,
+ pgTable,
+ pgEnum,
+ text,
+ uniqueIndex,
+ uuid,
+} from "drizzle-orm/pg-core";
+const valueTypeEnum = pgEnum("deployment_variable_value_type", [
+ "direct",
+ "reference",
+]);
...
- valueType: text("value_type").notNull().default("direct"),
+ valueType: valueTypeEnum("value_type").notNull().default("direct"),| // Direct value fields | ||
| value: jsonb("value").$type<any>(), | ||
| sensitive: boolean("sensitive").notNull().default(false), | ||
|
|
||
| // Reference fields | ||
| reference: text("reference"), | ||
| path: text("path").array(), | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
value / reference uniqueness coverage is incomplete
The existing unique index uniq is (variable_id, value).
For rows whose valueType = 'reference', value is always NULL, so all reference rows for the same variable can coexist with identical reference + path – probably unintended.
Recommend expanding the uniqueness criteria:
- uniq: uniqueIndex().on(t.variableId, t.value),
+ uniq: uniqueIndex("deployment_variable_value_unique").on(
+ t.variableId,
+ sql`COALESCE(value, to_jsonb(reference || ':' || array_to_string(path, '.')))`,
+ ),or (cleaner) create two partial unique indexes:
CREATE UNIQUE INDEX ... ON deployment_variable_value(variable_id, value)
WHERE value_type = 'direct';
CREATE UNIQUE INDEX ... ON deployment_variable_value(variable_id, reference, path)
WHERE value_type = 'reference';Drizzle supports uniqueIndex().on(...).where(sql``) if you prefer all-TS.
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Locate the deployment-variables.ts file
FILE=$(fd 'deployment-variables.ts$' -t f | head -n1)
echo "Found file: $FILE"
echo
# Display the first 200 lines to inspect schema and indexes
echo "=== File Content (lines 1-200) ==="
sed -n '1,200p' "$FILE"
echo
# Search for uniqueIndex definitions
echo "=== uniqueIndex occurrences ==="
grep -n "uniqueIndex" "$FILE" || true
echo
# Search for valueType column definition
echo "=== valueType occurrences ==="
grep -n "valueType" "$FILE" || trueLength of output: 6469
Ensure uniqueness for both direct and reference values
The current uniq index on deployment_variable_value(variable_id, value) only applies to rows where value_type = 'direct'. For reference-type rows (value_type = 'reference), value is always NULL, so multiple rows with the same (variable_id, reference, path) can slip through.
Apply two partial unique indexes:
• In packages/db/src/schema/deployment-variables.ts, update the table definition for deploymentVariableValue:
(t) => ({
- uniq: uniqueIndex().on(t.variableId, t.value),
+ // Unique for direct values only
+ directValueUniq: uniqueIndex('deployment_variable_value_value_unique')
+ .on(t.variableId, t.value)
+ .where(sql`value_type = 'direct'`),
+
+ // Unique for reference values only
+ referenceValueUniq: uniqueIndex('deployment_variable_value_reference_unique')
+ .on(t.variableId, t.reference, t.path)
+ .where(sql`value_type = 'reference'`),
variableIdFk: foreignKey({ columns: [t.variableId], foreignColumns: [deploymentVariable.id] })
.onUpdate("restrict")
.onDelete("cascade"),
check: sql`CONSTRAINT valid_value_type CHECK (
(value_type = 'direct' AND value IS NOT NULL AND reference IS NULL AND path IS NULL) OR
(value_type = 'reference' AND value IS NULL AND reference IS NOT NULL AND path IS NOT NULL)
)`,
}),This enforces uniqueness separately for direct and reference entries.
📝 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.
| // Direct value fields | |
| value: jsonb("value").$type<any>(), | |
| sensitive: boolean("sensitive").notNull().default(false), | |
| // Reference fields | |
| reference: text("reference"), | |
| path: text("path").array(), | |
| }, | |
| // … earlier in deployment-variables.ts … | |
| export const deploymentVariableValue = mysqlTable( | |
| 'deployment_variable_value', | |
| { | |
| id: serial('id').primaryKey(), | |
| variableId: int('variable_id').notNull(), | |
| valueType: text('value_type').notNull(), | |
| // Direct value fields | |
| value: jsonb('value').$type<any>(), | |
| sensitive: boolean('sensitive').notNull().default(false), | |
| // Reference fields | |
| reference: text('reference'), | |
| path: text('path').array(), | |
| }, | |
| (t) => ({ | |
| // Unique for direct values only | |
| directValueUniq: uniqueIndex('deployment_variable_value_value_unique') | |
| .on(t.variableId, t.value) | |
| .where(sql`value_type = 'direct'`), | |
| // Unique for reference values only | |
| referenceValueUniq: uniqueIndex('deployment_variable_value_reference_unique') | |
| .on(t.variableId, t.reference, t.path) | |
| .where(sql`value_type = 'reference'`), | |
| variableIdFk: foreignKey({ | |
| columns: [t.variableId], | |
| foreignColumns: [deploymentVariable.id], | |
| }) | |
| .onUpdate('restrict') | |
| .onDelete('cascade'), | |
| check: sql`CONSTRAINT valid_value_type CHECK ( | |
| (value_type = 'direct' AND value IS NOT NULL AND reference IS NULL AND path IS NULL) OR | |
| (value_type = 'reference' AND value IS NULL AND reference IS NOT NULL AND path IS NOT NULL) | |
| )`, | |
| }), | |
| ); |
Summary by CodeRabbit
New Features
Bug Fixes