chore: db migrations and types for functions configs - #6960
Conversation
There was a problem hiding this comment.
3 issues found across 5 files
Confidence score: 2/5
- In
packages/database/lib/migrations/20260729120000_create_function_tables.cjs, independent foreign keys allow a function instance to mix a connection and function config from different environments, which can trigger cross-environment execution and isolation breaches—add a shared-environment constraint (e.g., composite FK/check) to enforce environment consistency at the DB level. - In
packages/types/lib/functions/config.ts, scheduled function configs can still persistperConnection: 'max'even though schedules are meant to run with concurrency1, creating a runtime/typing mismatch that can allow unintended parallel runs—tighten the persisted schedule config type to preserve the schedule-specific1constraint. - In
packages/types/lib/functions/config.ts, function config declarations now exist in two places with diverging semantics, increasing regression risk as changes land in only one source—consolidate shared types in@nangohq/types(or complete that migration here) so SDK and persisted configs stay aligned.
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/types/lib/functions/config.ts">
<violation number="1" location="packages/types/lib/functions/config.ts:8">
P2: Function config types now have two sources of truth, and their semantics already diverge. Move the shared declarations to `@nangohq/types` and have runner-sdk import them, or complete that migration in this change before deployment paths depend on both.</violation>
<violation number="2" location="packages/types/lib/functions/config.ts:39">
P2: Scheduled function configs can persist `perConnection: 'max'`, although SDK declarations explicitly prohibit concurrent schedule runs. Preserve the schedule-specific `1` constraint in the persisted config type so deployment cannot bypass it.</violation>
</file>
<file name="packages/database/lib/migrations/20260729120000_create_function_tables.cjs">
<violation number="1" location="packages/database/lib/migrations/20260729120000_create_function_tables.cjs:77">
P1: Function instances can pair a connection with a function config from another environment because these independent foreign keys do not enforce a shared environment. This permits cross-environment function execution if a caller supplies mismatched IDs; store `environment_id` and enforce composite foreign keys (or otherwise constrain both references to the same environment).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| CREATE TABLE IF NOT EXISTS function_instances ( | ||
| id UUID PRIMARY KEY, | ||
| nango_connection_id INTEGER NOT NULL REFERENCES _nango_connections(id) ON DELETE CASCADE, | ||
| function_config_id INTEGER NOT NULL REFERENCES function_configs(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
P1: Function instances can pair a connection with a function config from another environment because these independent foreign keys do not enforce a shared environment. This permits cross-environment function execution if a caller supplies mismatched IDs; store environment_id and enforce composite foreign keys (or otherwise constrain both references to the same environment).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/database/lib/migrations/20260729120000_create_function_tables.cjs, line 77:
<comment>Function instances can pair a connection with a function config from another environment because these independent foreign keys do not enforce a shared environment. This permits cross-environment function execution if a caller supplies mismatched IDs; store `environment_id` and enforce composite foreign keys (or otherwise constrain both references to the same environment).</comment>
<file context>
@@ -0,0 +1,139 @@
+ CREATE TABLE IF NOT EXISTS function_instances (
+ id UUID PRIMARY KEY,
+ nango_connection_id INTEGER NOT NULL REFERENCES _nango_connections(id) ON DELETE CASCADE,
+ function_config_id INTEGER NOT NULL REFERENCES function_configs(id) ON DELETE CASCADE,
+ name TEXT NOT NULL,
+ variant VARCHAR(255) NOT NULL DEFAULT 'base',
</file context>
There was a problem hiding this comment.
on purpose, to be enforced app-side by the data access layer. Adding composite keys over several tables including existing connection table isn't worth it imho. let me know if you disagree
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning.
| * - `{ body: '$.portalId' }`: dot notation path into the body | ||
| * - `{ header: 'x-goog-resource-id' }`: flat, case-insensitive header lookup | ||
| */ | ||
| export type DebounceKeySource = { body: string } | { header: string }; |
There was a problem hiding this comment.
P2: Function config types now have two sources of truth, and their semantics already diverge. Move the shared declarations to @nangohq/types and have runner-sdk import them, or complete that migration in this change before deployment paths depend on both.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/types/lib/functions/config.ts, line 8:
<comment>Function config types now have two sources of truth, and their semantics already diverge. Move the shared declarations to `@nangohq/types` and have runner-sdk import them, or complete that migration in this change before deployment paths depend on both.</comment>
<file context>
@@ -0,0 +1,43 @@
+ * - `{ body: '$.portalId' }`: dot notation path into the body
+ * - `{ header: 'x-goog-resource-id' }`: flat, case-insensitive header lookup
+ */
+export type DebounceKeySource = { body: string } | { header: string };
+
+/** Coalesces a burst of inbound HTTP requests into a single function run within a sliding window. */
</file context>
There was a problem hiding this comment.
as I said next PR will align types across runner-sdk and db. I want to keep the PR small
|
|
||
| export interface FunctionLimits { | ||
| concurrency?: { | ||
| perConnection: FunctionConcurrencyLimit; |
There was a problem hiding this comment.
P2: Scheduled function configs can persist perConnection: 'max', although SDK declarations explicitly prohibit concurrent schedule runs. Preserve the schedule-specific 1 constraint in the persisted config type so deployment cannot bypass it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/types/lib/functions/config.ts, line 39:
<comment>Scheduled function configs can persist `perConnection: 'max'`, although SDK declarations explicitly prohibit concurrent schedule runs. Preserve the schedule-specific `1` constraint in the persisted config type so deployment cannot bypass it.</comment>
<file context>
@@ -0,0 +1,43 @@
+
+export interface FunctionLimits {
+ concurrency?: {
+ perConnection: FunctionConcurrencyLimit;
+ };
+}
</file context>
There was a problem hiding this comment.
will be enforced at deploy time
kaposke
left a comment
There was a problem hiding this comment.
I have questions about the decisions - not really challenging them, just wanted to understand how some where taken. I might have just missed some meeting/docs?
But looks like a solid structure nonetheless.
| variant VARCHAR(255) NOT NULL DEFAULT 'base', | ||
| last_run_date TIMESTAMPTZ, | ||
| last_fetched_at TIMESTAMPTZ, | ||
| frequency TEXT, |
There was a problem hiding this comment.
What's the frequency in this new model?
There was a problem hiding this comment.
the same as it is right now, with one caveat: it can be null now.
There is no plan to change how frequency works (other than support for cron syntax but from a data pov it is still some sort of string)
There was a problem hiding this comment.
Yeah but it's in the trigger now, right? I'm guessing this is here in function_instances to adapt to the scheduler.
| deleted: boolean; | ||
| } | ||
| export interface DeletedAt { | ||
| deleted_at: Date | null; |
There was a problem hiding this comment.
No redundant boolean??????
Not absolutely required since it tracks execution status which is already done by orchestrator and records aggregated count tracking which could be done by the runner itself
…ion_instances table
Add db tables and types to support
function.Tables are similar to current
sync_configs,syncsandsync_jobswith the following caveat:functions_configsandfunctions_config_versionto clearly mark the difference between a function and its different version (note that the plan is to have version managed internally based on definition and/or code change, instead of having an entry every time customer deploys).None of the tables and types are used yet. I will align the types in the runner SDK and CLI in next PR. Then I will work on function deployment.