Skip to content

Conversation

@oxtoacart
Copy link
Contributor

This allows built-in functions to be disabled.

Updates tailscale/corp#31396

@oxtoacart oxtoacart force-pushed the percy/disable-func branch 3 times, most recently from 354e841 to d6350c3 Compare August 22, 2025 14:11
Copy link
Member

@alexwlchan alexwlchan left a comment

Choose a reason for hiding this comment

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

🎉

One suggestion but it's a tiny one; happy for you to merge as-is if you prefer.

sqlite_test.go Outdated
}
defer conn.Close()

exec(t, conn, "CREATE TABLE t (c)")
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I would skip creating the table; SQLite allows you to run SQL functions without applying them to a table (SELECT LOWER('Hi') is a complete query).

This allows built-in functions to be disabled.

Updates tailscale/corp#31396

Signed-off-by: Percy Wegmann <percy@tailscale.com>
@oxtoacart oxtoacart merged commit 1673cdf into main Aug 22, 2025
2 checks passed
Comment on lines +63 to +66
// DisableFunction disables an existing function (including built-ins) using
// sqlite3_create_function. The name and numArgs must match the existing
// function's signature.
//
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit

Suggested change
// DisableFunction disables an existing function (including built-ins) using
// sqlite3_create_function. The name and numArgs must match the existing
// function's signature.
//
// DisableFunction disables an existing function (including built-ins) using
// sqlite3_create_function. The name and numArgs must match the existing
// function's signature.

It is probably also worth noting that we "disable" it by replacing that function name with a stub that does nothing, rather than making it report an error. That might be relevant to someone debugging.

}

// DisableFunction disables the named function on the given connection.
// numArgs must match the number of args of the function to be disabled.
Copy link
Member

Choose a reason for hiding this comment

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

As written, this will also let you "create" new previously non-existing no-op functions. That's harmless, but someone who misspells the function name they wanted to disable could easily be confused as there's no error to say "that doesn't exist".

Is it practical to look up the requested function first, and use that to report an error if someone tries to "disable" a function that isn't there? (E.g., we could query select name, narg from pragma_function_list where name = 'fname'). That would also have the incidental benefit that we wouldn't need to know the argument count a priori.

Not positive it's worth it, but this should be uncommon enough that it might be worth the extra step.

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.

3 participants