Set precedence to DATABASE_URL over POSTHOG_DB_* fields #441
Conversation
Twixes
left a comment
There was a problem hiding this comment.
This should not be needed for any setup though 🤔 POSTHOG_DB_NAME – AFAIK – has to be explicitly set by the user, in which case we can assume it's more deliberate than DATABASE_URL – which practically always has a value, because it has a default.
src/utils/db/hub.ts
Outdated
| ) | ||
|
|
||
| const postgres = createPostgresPool(serverConfig) | ||
| const configOrDatabaseUrl = serverConfig.DATABASE_URL ? serverConfig.DATABASE_URL : serverConfig |
There was a problem hiding this comment.
This will always result in DATABASE_URL being used, because it has a non-empty default value (while POSTHOG_DB_NAME is null by default).
There was a problem hiding this comment.
Oh, right, this is annoying :$ - looks like it's not required on main PostHog: https://github.com/PostHog/posthog/blob/master/posthog/settings.py#L369-L376 - and hence the precedence order there.
If we use POSTHOG_DB_NAME, while PostHog/posthog uses DATABASE_URL, that's.. not good.
I wonder if we can get around this by removing the default value for DATABASE_URL ? Does that break things / workflows elsewhere? (I'm guessing probably yes)
.. or maybe it's a documentation thing? We explicitly mention to use one or the other? Although, I'd prefer to fix this ourselves so things are consistent.
There was a problem hiding this comment.
Yeah, def not good to have a discrepancy. I suppose the fix would then be removing the default value of DATABASE_URL here in the non-test non-dev case (should be OK), and adjusting createPostgresPool for same control flow as in the main repo.
src/utils/utils.ts
Outdated
| connectionString: configOrDatabaseUrl.DATABASE_URL, | ||
| } | ||
| : { | ||
| database: configOrDatabaseUrl.POSTHOG_DB_NAME || undefined, |
There was a problem hiding this comment.
Is there a better way to convert null to undefined for types?
There was a problem hiding this comment.
You can also use ?? which is the nullish coalescing operator – where || returns right side when left side is falsy, ?? returns right side only when left side is null or undefined. So depends on whether we want the plugin server to treat '' as a valid (if empty) string value (→ ??), or as a lack of value i.e. undefined (→ ||).
There was a problem hiding this comment.
Will do what PostHog main does: null is not ok, falsy is ok.
Twixes
left a comment
There was a problem hiding this comment.
Nice, discrepancy is no more. A few extra edge cases
…ds (PostHog/plugin-server#441) * set precedence to DATABASE_URL * make DATABASE_URL non mandatory, check env var configuration * fix tests * address comments * check what went wrong * Remove extra cat 😾 Co-authored-by: Michael Matloka <dev@twixes.com>
Changes
This reconciles precedence order to be the same as on PostHog/posthog. This is important, because otherwise cases are possible where the Django app connects to postgres without issues, but the plugin server doesn't: so events are shown on the UI, but aren't ingested properly.
Specially, have seen this happen with SSL enabled DB urls, like in this case: https://posthogusers.slack.com/archives/C01GLBKHKQT/p1621416865003200
Checklist