Skip to content
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

Add User table to postgres #4554

Closed
3 tasks
mattkrick opened this issue Jan 19, 2021 · 7 comments
Closed
3 tasks

Add User table to postgres #4554

mattkrick opened this issue Jan 19, 2021 · 7 comments
Assignees

Comments

@mattkrick
Copy link
Member

let's start to move to postgres. see #4530

AC

  • Every time a User is created, they are also created in postgres
  • Every time a User is updated, the update is propagated to postgres
  • a postdeploy hook is written to copy all Users to postgres

Estimate: 16 hours

@jordanh jordanh added this to the Chores milestone Jan 26, 2021
@tiffanyhan
Copy link
Contributor

Ran into the following problems with pgtyped:

As an experiment, i tried installing zapatos to achieve this task. What I found:

  • custom domain types are supported
  • custom types are supported as a first-class citizen, with accessible hooks for user defined transformations of types
  • addresses the json casting problem, by setting castArrayParamsToJson optional runtime config to true

Other differences:

  • zapatos uses a sql template tag and doesn't seem to support .sql files. i think this is fine, since we don't already have a bunch of .sql files we have to work with.
  • zapatos generates ALL crud input params and output types into one file from the database; not dependent on written code. pgtyped generates relevant input params and output types based on the database and the written queries in the codebase; generated types go in a separate file corresponding to the src query file. i appreciate the simplicity of the zapatos generation and the fact that we don't have to watch the code to generate the types.
  • zapatos comes with helpers for basic crud queries. the helpers reduce a lot of boilerplate and are basically what i would write myself if using pgtyped.
  • zapatos supports transactions, personally untested.
  • zapatos supports joins returned as nested json. untested, but the nice interface with json gives me more confidence that this would offer a more convenient API for much of our json structured data.

@tiffanyhan tiffanyhan mentioned this issue Jan 31, 2021
7 tasks
@mattkrick
Copy link
Member Author

no support for postgres domains, ran into this while trying to implement a custom domain for email address field

citext is the right answer. emails don't always have tlds, they just need an at sign. custom domains can often do more harm than good

inconsistent casting of json types.

defaulting to native arrays over JSON arrays is probably the right answer here, can always explicitly stringify if we absolutely need a JSON array

incomplete support for postgres types and no hook provided for custom type mappings

totally fair! could you give an example where this might hurt us in the future?

After reviewing your PR, I think zapatos is too young to adopt:

  • stringifying dates is just wrong. it has access to the schema, it should use it. requiring us to iterate over every object prop looking for datestrings is going to result in some nasty bugs.
  • it uses namespaces instead of modules. this will break all attempt at code-driven refactoring because the absolute import statements are indistinguishable from node modules. it also makes for some pretty verbose code
  • most calls require typed generics instead of providing us with a runtime function with those types built in. await db.sql<s.authors.SQL, s.authors.Selectable[]> has more moving pieces than await insertOrgUserAuditQuery.run

let's stick with pgtyped for now

@tiffanyhan
Copy link
Contributor

when deleting a user then creating a new account, locally created accounts get recreated with a new row and id. but when using google auth, the newly created account reuses the same db row and id and the email stays "DELETED".

@mattkrick
Copy link
Member Author

oh that's interesting! probably because the userId that's generated isn't random, so it'll find that record. if they rejoin, we could still re-use the same User object, but just update the email.

@tiffanyhan
Copy link
Contributor

SEND TO SENTRY error: ON CONFLICT DO UPDATE command cannot affect row a second time

we need to merge users with duplicate emails

@tiffanyhan
Copy link
Contributor

r
.db('actionProduction')
.table('User')
.group(r.row('email').downcase(), {index: 'email'})
.count()
.ungroup()
.filter((row) => row('reduction').gt(1))('group')
.filter((row) => row.ne('DELETED'))
.run({arrayLimit: 200000})

@tiffanyhan
Copy link
Contributor

implemented in #4787

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants