feat(sequelize): Add Sequelize adapter #237

luke-j · 2021-09-19T08:09:59Z

Reasoning 💡

This follows on from #73 (although doesn't share much code) and creates an adapter for Sequelize. Appreciate any feedback!

Checklist 🧢

Documentation
Tests
Ready to be merged

I still have to test this more thoroughly locally, but would appreciate any feedback in the meantime.

balazsorban44 · 2021-09-19T10:42:37Z

Looks promising, but running the tests from https://github.com/nextauthjs/adapters/blob/next/basic-tests.ts would be crucial for this to be merged. Keep up the good work, I'll review when you manage to run all tests successfully 😉 Feel free to ask for help if you are stuck somewhere.

luke-j · 2021-09-19T11:09:41Z

No problems @balazsorban44

running the tests from https://github.com/nextauthjs/adapters/blob/next/basic-tests.ts would be crucial for this to be merged.

Could you clarify? I've added these tests and they're passing for me locally, looks like the GitHub action requires maintainer approval to run.

balazsorban44 · 2021-09-19T11:20:05Z

My bad, I skimmed through https://github.com/nextauthjs/adapters/blob/748d7dc8fb4d8188d81b5e171a21943369ada642/packages/sequelize/tests/index.test.ts 😅 You are right about GitHub action requiring a maintainer.

balazsorban44

Very nice work! I only had a few suggestions! It looks to me that Sequelize is very nice to work with. 👌

I could see it as a replacement in our suggestions for the TypeORM adapter IMO.

packages/sequelize/README.md

packages/sequelize/package.json

balazsorban44 · 2021-09-19T12:01:18Z

packages/sequelize/src/index.ts

+import { Sequelize, Model, ModelCtor } from "sequelize"
+import {
+  accountSchema,
+  userSchema,
+  sessionSchema,
+  verificationTokenSchema,
+} from "./schema"
+
+// @see https://sequelize.org/master/manual/typescript.html
+interface AccountInstance
+  extends Model<ApadterAccount, Partial<ApadterAccount>>,
+    ApadterAccount {}
+interface UserInstance
+  extends Model<AdapterUser, Partial<AdapterUser>>,
+    AdapterUser {}
+interface SessionInstance
+  extends Model<AdapterSession, Partial<AdapterSession>>,
+    AdapterSession {}
+interface VerificationTokenInstance
+  extends Model<VerificationToken, Partial<VerificationToken>>,
+    VerificationToken {}
+
+export { accountSchema, userSchema, sessionSchema, verificationTokenSchema }
+
+export default function SequelizeAdapter(client: Sequelize): Adapter {
+  const tableOptions = { underscored: true, timestamps: false }
+
+  const Account =
+    (client.models.Account as ModelCtor<any>) ||
+    client.define<AccountInstance>("account", accountSchema, tableOptions)
+  const User =
+    (client.models.User as ModelCtor<any>) ||
+    client.define<UserInstance>("user", userSchema, tableOptions)
+  const Session =
+    (client.models.Session as ModelCtor<any>) ||
+    client.define<SessionInstance>("session", sessionSchema, tableOptions)
+  const VerificationToken =
+    (client.models.VerificationToken as ModelCtor<any>) ||
+    client.define<VerificationTokenInstance>(
+      "verificationToken",
+      verificationTokenSchema,
+      tableOptions
+    )


Suggested change

import { Sequelize, Model, ModelCtor } from "sequelize"

import {

accountSchema,

userSchema,

sessionSchema,

verificationTokenSchema,

} from "./schema"

// @see https://sequelize.org/master/manual/typescript.html

interface AccountInstance

extends Model<ApadterAccount, Partial<ApadterAccount>>,

ApadterAccount {}

interface UserInstance

extends Model<AdapterUser, Partial<AdapterUser>>,

AdapterUser {}

interface SessionInstance

extends Model<AdapterSession, Partial<AdapterSession>>,

AdapterSession {}

interface VerificationTokenInstance

extends Model<VerificationToken, Partial<VerificationToken>>,

VerificationToken {}

export { accountSchema, userSchema, sessionSchema, verificationTokenSchema }

export default function SequelizeAdapter(client: Sequelize): Adapter {

const tableOptions = { underscored: true, timestamps: false }

const Account =

(client.models.Account as ModelCtor<any>) ||

client.define<AccountInstance>("account", accountSchema, tableOptions)

const User =

(client.models.User as ModelCtor<any>) ||

client.define<UserInstance>("user", userSchema, tableOptions)

const Session =

(client.models.Session as ModelCtor<any>) ||

client.define<SessionInstance>("session", sessionSchema, tableOptions)

const VerificationToken =

(client.models.VerificationToken as ModelCtor<any>) ||

client.define<VerificationTokenInstance>(

"verificationToken",

verificationTokenSchema,

tableOptions

)

import type { Sequelize, Model, ModelCtor, ModelOptions } from "sequelize"

import * as defaultModels from "./schema"

export { defaultModels as models }

// @see https://sequelize.org/master/manual/typescript.html

interface AccountInstance

extends Model<ApadterAccount, Partial<ApadterAccount>>,

ApadterAccount {}

interface UserInstance

extends Model<AdapterUser, Partial<AdapterUser>>,

AdapterUser {}

interface SessionInstance

extends Model<AdapterSession, Partial<AdapterSession>>,

AdapterSession {}

interface VerificationTokenInstance

extends Model<VerificationToken, Partial<VerificationToken>>,

VerificationToken {}

interface SequelizeAdapterOptions {

models?: Partial<{

User: ModelCtor<UserInstance>

Account: ModelCtor<AccountInstance>

Session: ModelCtor<SessionInstance>

VerificationToken: ModelCtor<VerificationTokenInstance>

}>

}

export default function SequelizeAdapter(

client: Sequelize,

options?: SequelizeAdapterOptions

): Adapter {

const { models } = options ?? {}

const defaultModelOptions: ModelOptions = {

underscored: true,

timestamps: false,

}

const { User, Account, Session, VerificationToken } = {

User:

models?.User ??

client.define<UserInstance>(

"user",

defaultModels.User,

defaultModelOptions

),

Account:

models?.Account ??

client.define<AccountInstance>(

"account",

defaultModels.Account,

defaultModelOptions

),

Session:

models?.Session ??

client.define<SessionInstance>(

"session",

defaultModels.Session,

defaultModelOptions

),

VerificationToken:

models?.VerificationToken ??

client.define<VerificationTokenInstance>(

"verificationToken",

defaultModels.VerificationToken,

defaultModelOptions

),

}

I would refactor this section in a way that makes it easier to configure by the user.

With my suggestions, the user would override User like this:

SequelizeAdapter(client, { schemas: { User: client.define("user", MyUserModel) } })

What do you think?

Also for consistent naming, I think we should just stick to models/model instead of schemas, as the TS types suggest that anyway. (could you rename the schema.ts file to models.ts as well?)

Yep I think that's a fair change, I've refactored that in f6b9518 I've also added docs on how to do this in cd44414

balazsorban44 · 2021-09-19T12:04:31Z

packages/sequelize/src/index.ts

+  Account.belongsTo(User, { onDelete: "cascade" })
+  Session.belongsTo(User, { onDelete: "cascade" })


Is this necessarily done runtime? I am not familiar with Sequelize, but usually, you would do some kind of seeding/setup step to define the database schema, right? Does Sequelize have anything similar to TypeORM's syncrhonize: true (https://github.com/typeorm/typeorm/blob/master/docs/faq.md#how-do-i-update-a-database-schema)?

We have to be careful not to update things in production to prevent data loss. In development though, auto-syncing model changes might be useful. #224 how we handled this for the TypeORM adapter recently.

Sequelize has the sequelize.sync() function similar to TypeORM's synchronize option. Defining these associations at runtime is recommended and doesn't produce any SQL unless sync() is called.

It would be best practice for the user to define the tables as migrations, but sequelize will automatically create the necessary tables/FKs/indexes with sync like so:

const sequelize = new Sequelize('sqlite:memory:') const adapter = SequelizeAdapter(sequelize) sequelize.sync() export default NextAuth({ ... adapter ... })

I've added a section in the README about how to create tables like this with a mention that best practice is still migrations in e0b1dae.

I think we should default to synchronizing if we detect process.env.NODE_ENV !== "production", but make it so that users can opt-out even then, with a config option like synchronize: false.

We could reuse this warning by some rewording: https://next-auth.js.org/warnings#adapter_typeorm_updating_entities

I would add client.sync() in an if statement with the warning similar to this c2559b7 (#224) in the body of the Adapter.

Otherwise, we should create explain how to migrate manually: https://sequelize.org/master/manual/migrations.html

@balazsorban44 I'm thinking about how we might achieve this. I remembered that sync actually returns a promise, in which case we'd have to make the adapter async which would mess with the user-facing API.

eg.

if (process.env.NODE_ENV !== 'production' && synchronize) { await Promise.all([ User.sync(), Account.sync(), Session.sync(), VerificationToken.sync() ]) }

I'm wondering if including sync, which is potentially dangerous or unexpected from the user's perspective, is worth it in this regard? There's not a huge advantage over just leaving this to user land code, right?

I see. Ultimately, this is why we had to get a connection manager in each adapter method with TypeORM, because getting a manager was an async operation. 😅

https://github.com/nextauthjs/adapters/blob/next/packages/typeorm-legacy/src/index.ts

I don't want the adapter to be async as that would complicate the initialization.

I see there is an all-in-one .sync() method as well.

https://sequelize.org/master/class/lib/sequelize.js~Sequelize.html#instance-method-sync

My idea is that we should create a wrapper for it, and add a sync: boolean | SyncOptions to the adapter options, and do something similar to TypeORM, and call it in each method first.

I feel like we should stretch here, as the DX would be better. Especially useful if you would need to debug, I think.

I am thinking of something like this:

function SequelizeAdapter(client, options) { // Make sure we only sync once per invocation let _synced = false const sync = async () => { if ( process.env.NODE_ENV === "production" || _synced || options.sync === false ) return const syncOptions = typeof options.sync === "object" ? options.sync : { // ... some default options, if we need it } await client.sync(syncOptions) _synced = true } return { async createUser() { await sync() // call this first in each method //... }, } }

Yeah I think if we want to do this this is probably the best option. I've done this in 48961d1 along with some tests. Unfortunately it's not the prettiest solution.

If this is occurring in multiple adapters I think it's reasonable to consider an optional connect or init function returned from an adapter which is then called by the main next-auth package prior to calling the other functions.

Most adapters seem to connect/disconnect automatically, actually. In my experience, it is only TypeORM that needs manual connection handling so far. 😕

packages/sequelize/src/schema.ts

balazsorban44 · 2021-09-19T12:12:26Z

packages/sequelize/tests/index.test.ts

+    verificationToken: async ({ identifier, token }) => {
+      const verificationToken =
+        await sequelize.models.verificationToken.findOne({
+          where: { identifier, token },
+        })


Suggested change

verificationToken: async ({ identifier, token }) => {

const verificationToken =

await sequelize.models.verificationToken.findOne({

where: { identifier, token },

})

verificationToken: async (where) => {

const verificationToken =

await sequelize.models.verificationToken.findOne({ where })

balazsorban44 · 2021-09-19T12:13:02Z

packages/sequelize/tests/index.test.ts

+    account: async ({ provider, providerAccountId }) => {
+      const account = await sequelize.models.account.findOne({
+        where: { provider, providerAccountId },
+      })


Suggested change

account: async ({ provider, providerAccountId }) => {

const account = await sequelize.models.account.findOne({

where: { provider, providerAccountId },

})

account: async (where) => {

const account = await sequelize.models.account.findOne({ where })

balazsorban44 · 2021-09-19T12:14:46Z

packages/sequelize/src/index.ts

+    async getUser(id) {
+      const userInstance = await User.findByPk(id)
+
+      return userInstance?.get({ plain: true }) ?? null


I love plain: true 😍! This reminds me of the Prisma Adapter, which is by far the easiest to understand IMO, since it can return plain JavaScript objects, instead of convoluted custom formats for dates and stuff.

All ORMs should have an option like this! 🙏

Agree! Having an instance returned can be useful but the option for a plain object is always great.

packages/sequelize/src/index.ts

Co-authored-by: Balázs Orbán <info@balazsorban.com>

luke-j · 2021-09-20T00:19:24Z

Thanks for the excellent feedback @balazsorban44.

I could see it as a replacement in our suggestions for the TypeORM adapter IMO.

Sequelize is certainly the most mature ORM available at the moment, I think. It's not as new and shiny as TypeORM but it's a solid choice IMO. I'd be happy to help out with any refactor 👍

balazsorban44 · 2021-09-21T08:32:56Z

packages/sequelize/README.md

+sequelize.sync()
+
+export default NextAuth({
+  ...
+  adapter
+  ...
+})


Suggested change

sequelize.sync()

export default NextAuth({

...

adapter

...

})

export default async function auth(req, res) {

await sequelize.sync()

return await NextAuth(req, res, {

...

adapter

...

})

}

As you pointed out, .sync() returns a Promise, so I guess this wouldn't work anyway. I leave a suggestion just to show you the alternative, but I don't think we should actually do it that way. See my comment here: https://github.com/nextauthjs/adapters/pull/237/files#r712815434

balazsorban44

Thanks, just a few more suggestions.

balazsorban44 · 2021-09-23T22:55:06Z

packages/sequelize/README.md

+
+export default NextAuth({
+  ...
+  adapter: SequelizeAdapter(sequelize, { synchronize: true })


Suggested change

adapter: SequelizeAdapter(sequelize, { synchronize: true })

adapter: SequelizeAdapter(sequelize, { synchronize: false })

As part of the rephrased section over.

balazsorban44 · 2021-09-23T22:55:40Z

packages/sequelize/README.md

+By default, the sequelize adapter will not create tables in your database. In production, best practice is to create the [required tables](https://next-auth.js.org/adapters/models) in your database via [migrations](https://sequelize.org/master/manual/migrations.html). In development, you are able to pass `{ synchronize: true }` as the second option to `SequelizeAdapter` to have sequelize create the necessary tables, foreign keys and indexes:
+


We should rephrase it in a way that says in development, we will synchronize by default, but in production, migrations are recommended instead to prevent data loss.

balazsorban44 · 2021-09-23T22:57:01Z

packages/sequelize/README.md

+const sequelize = new Sequelize("sqlite::memory:")
+const options = {
+  models: {
+    User: sequelize.define('user', { ...models.User, phoneNumber: DataTypes.STRING })
+  }
+}
+
+export default NextAuth({
+  ...
+  adapter: SequelizeAdapter(sequelize, options)
+  ...
+})


Suggested change

const sequelize = new Sequelize("sqlite::memory:")

const options = {

models: {

User: sequelize.define('user', { ...models.User, phoneNumber: DataTypes.STRING })

}

}

export default NextAuth({

...

adapter: SequelizeAdapter(sequelize, options)

...

})

const sequelize = new Sequelize("sqlite::memory:")

export default NextAuth({

...

adapter: SequelizeAdapter(sequelize, {

models: {

User: sequelize.define('user', { ...models.User, phoneNumber: DataTypes.STRING })

}

})

...

})

Let's do this for IDE hints/type suggestion.

balazsorban44 · 2021-09-23T22:59:36Z

packages/sequelize/src/index.ts

+  client: Sequelize,
+  options?: SequelizeAdapterOptions
+): Adapter {
+  const { models, synchronize } = options ?? {}


Suggested change

const { models, synchronize } = options ?? {}

const { models, synchronize = true } = options ?? {}

Let's default to synchronize: true, so the user doesn't have to set this to true manually in development.

I would assume the more rare case would be when they don't need syncing in development, in that case, they could set synchronize: false explicitly. Explain that in the README.md

rageshoo · 2021-10-04T15:40:09Z

@luke-j Hi Luke, thanks for working on this! Do you have any updates?

luke-j · 2021-10-05T00:54:37Z

@balazsorban44 I've defaulted synchronize to true in a03a37c & 564dc81. Let me know if you have any further feedback.

rodocontegni · 2021-11-17T16:56:02Z

@luke-j thank you so much for the hard work
is there anything holding up this PR? cc @balazsorban44
i'm happy to take a shot if there's anything pending

balazsorban44 · 2021-11-17T22:16:40Z

I'll have a look, sorry for the delay. I was getting the core repo ready for stable v4 in the past couple of weeks, but most of the important PRs have been merged, I'll focus on adapters and the docs in the coming weeks!

balazsorban44 · 2021-11-17T22:22:54Z

packages/sequelize/package.json

+    "access": "public"
+  },
+  "scripts": {
+    "test": "jest",


Don't need to run build on test, we will build before publishing anyway.

balazsorban44 · 2021-11-17T22:23:18Z

packages/sequelize/package.json

+    "dist"
+  ],
+  "peerDependencies": {
+    "next-auth": ">4 || 4.0.0-beta.1 - 4.0.0-beta.x",


Corrected this as per #274

balazsorban44

All tests pass locally, types look great, let's get this in! 👍

I will appreciate it if @luke-j and @rodocontegni could be around when v4 goes stable (in ~2 weeks) in case an issue arises! 🙏

Nice work and thank you so much for your contribution and patience! 💚

balazsorban44 · 2021-11-17T22:38:28Z

Enjoy @next-auth/sequelize-adapter

Sequelize adapter

748d7dc

github-actions bot added the tests Changes to testing label Sep 19, 2021

balazsorban44 self-requested a review September 19, 2021 10:43

balazsorban44 suggested changes Sep 19, 2021

View reviewed changes

luke-j and others added 8 commits September 20, 2021 09:16

Simplify README

bb0e792

Co-authored-by: Balázs Orbán <info@balazsorban.com>

Reference adapters repo in package.json

6d4f411

Co-authored-by: Balázs Orbán <info@balazsorban.com>

Add docs on updating database schema

e0b1dae

Use uuids over ints for ids

30363a1

Refactor schema to models, change user-facing API

f6b9518

Add docs on customizing models

cd44414

Simplify arguments

ddeba75

Assume user always exists in

3b2da67

luke-j mentioned this pull request Sep 20, 2021

feat: Add Sequelize adapter docs nextauthjs/docs#37

Merged

balazsorban44 reviewed Sep 21, 2021

View reviewed changes

Enable automatic sync

48961d1

balazsorban44 suggested changes Sep 23, 2021

View reviewed changes

luke-j added 2 commits October 5, 2021 11:48

Default synchronize to true

a03a37c

Slightly change wording

564dc81

fix: correct peer dep versioning, don't run build on test

4cd4fe8

balazsorban44 reviewed Nov 17, 2021

View reviewed changes

balazsorban44 approved these changes Nov 17, 2021

View reviewed changes

balazsorban44 merged commit 15cb846 into nextauthjs:next Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequelize): Add Sequelize adapter #237

feat(sequelize): Add Sequelize adapter #237

luke-j commented Sep 19, 2021

balazsorban44 commented Sep 19, 2021 •

edited

Loading

luke-j commented Sep 19, 2021

balazsorban44 commented Sep 19, 2021 •

edited

Loading

balazsorban44 left a comment

balazsorban44 Sep 19, 2021

luke-j Sep 20, 2021

balazsorban44 Sep 19, 2021

luke-j Sep 19, 2021

balazsorban44 Sep 20, 2021

luke-j Sep 21, 2021

balazsorban44 Sep 21, 2021 •

edited

Loading

luke-j Sep 23, 2021 •

edited

Loading

balazsorban44 Sep 23, 2021

balazsorban44 Sep 19, 2021

balazsorban44 Sep 19, 2021

balazsorban44 Sep 19, 2021

luke-j Sep 20, 2021

luke-j commented Sep 20, 2021

balazsorban44 Sep 21, 2021

balazsorban44 left a comment

balazsorban44 Sep 23, 2021

balazsorban44 Sep 23, 2021

balazsorban44 Sep 23, 2021

balazsorban44 Sep 23, 2021

rageshoo commented Oct 4, 2021

luke-j commented Oct 5, 2021

rodocontegni commented Nov 17, 2021

balazsorban44 commented Nov 17, 2021 •

edited

Loading

balazsorban44 Nov 17, 2021

balazsorban44 Nov 17, 2021

balazsorban44 left a comment

balazsorban44 commented Nov 17, 2021

		Account.belongsTo(User, { onDelete: "cascade" })
		Session.belongsTo(User, { onDelete: "cascade" })

-sequelize.sync()
-export default NextAuth({
-  ...
-  adapter
-  ...
-})
+export default async function auth(req, res) {
+    await sequelize.sync()
+	return await NextAuth(req, res, {
+	  ...
+      adapter
+      ...
+	})
+}

	adapter: SequelizeAdapter(sequelize, { synchronize: true })
	adapter: SequelizeAdapter(sequelize, { synchronize: false })

		By default, the sequelize adapter will not create tables in your database. In production, best practice is to create the [required tables](https://next-auth.js.org/adapters/models) in your database via [migrations](https://sequelize.org/master/manual/migrations.html). In development, you are able to pass `{ synchronize: true }` as the second option to `SequelizeAdapter` to have sequelize create the necessary tables, foreign keys and indexes:

	const { models, synchronize } = options ?? {}
	const { models, synchronize = true } = options ?? {}

feat(sequelize): Add Sequelize adapter #237

feat(sequelize): Add Sequelize adapter #237

Conversation

luke-j commented Sep 19, 2021

Reasoning 💡

Checklist 🧢

balazsorban44 commented Sep 19, 2021 • edited Loading

luke-j commented Sep 19, 2021

balazsorban44 commented Sep 19, 2021 • edited Loading

balazsorban44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazsorban44 Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

luke-j Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-j commented Sep 20, 2021

Choose a reason for hiding this comment

balazsorban44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rageshoo commented Oct 4, 2021

luke-j commented Oct 5, 2021

rodocontegni commented Nov 17, 2021

balazsorban44 commented Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazsorban44 left a comment

Choose a reason for hiding this comment

balazsorban44 commented Nov 17, 2021

balazsorban44 commented Sep 19, 2021 •

edited

Loading

balazsorban44 commented Sep 19, 2021 •

edited

Loading

balazsorban44 Sep 21, 2021 •

edited

Loading

luke-j Sep 23, 2021 •

edited

Loading

balazsorban44 commented Nov 17, 2021 •

edited

Loading