-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(checkpoint-postgres): Add support for providing a custom schema during initialization #838
feat(checkpoint-postgres): Add support for providing a custom schema during initialization #838
Conversation
Thanks for the contribution! I'll have a closer look tomorrow, but from a quick glance this looks really nice! |
Don't worry about the docs deploy - that's expected to fail on external PRs for the moment. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small change requested, please & thanks. Handy change overall, though!
*/ | ||
static fromConnString( | ||
connString: string, | ||
options: PostgresSaverOptions = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of a nit, but on first read through I thought that perhaps you were mistakenly causing default options to not be applied by specifying this default value.
It might be more readable/explicit if you have a module-scoped _defaultOptions
, kept the options
arg here and in the ctor optional, and then conditionally applied that at the start of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamincburns I've pushed a change to update the default options behavior, including moving it to a module-scoped _defaultOptions
variable.
Let me know if this works for you.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the contribution!
/** | ||
* Creates a new instance of PostgresSaver from a connection string. | ||
* | ||
* @param {string} connString - The connection string to connect to the Postgres database. | ||
* @param {PostgresSaverOptions} [options] - Optional configuration object. | ||
* @returns {PostgresSaver} A new instance of PostgresSaver. | ||
* | ||
* @example | ||
* const connString = "postgresql://user:password@localhost:5432/db"; | ||
* const checkpointer = PostgresSaver.fromConnString(connString, { | ||
* schema: "custom_schema" // defaults to "public" | ||
* }); | ||
* await checkpointer.setup(); | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the reference docs, btw!
const _ensureCompleteOptions = ( | ||
options?: Partial<PostgresSaverOptions> | ||
): PostgresSaverOptions => { | ||
return { | ||
..._defaultOptions, | ||
...options, | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. In the future you can also just in-line this, as it's a really common pattern for applying defaults. Having it be a well-named function call does express intent more clearly, however.
…being removed by mistake
… if a connection string env var is not found
… string for easier extensibility later on
…efined and ensured during init
92d5b0f
to
82e696d
Compare
This PR adds support for specifying a custom schema in the
checkpoint-postgres
module during initialization. Instead of relying on the defaultpublic
schema used by Postgres or modifying the connection string’s search path (which can be unreliable at best), SQL queries now explicitly reference the provided schema, ensuring compatibility across all PostgreSQL versions and configurations, while still allowing the original module to stay backward compatible with previous versions sincepublic
was already the default schema.Key Changes
options
parameter to specify a schema when creating aPostgresSaver
instance via thefromConnString
methodfromConnString
with usage example