-
-
Notifications
You must be signed in to change notification settings - Fork 88
feat(sequelize): Add Sequelize adapter #237
Conversation
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. |
No problems @balazsorban44
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. |
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. |
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.
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/src/index.ts
Outdated
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 | ||
) |
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.
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?)
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.
Account.belongsTo(User, { onDelete: "cascade" }) | ||
Session.belongsTo(User, { onDelete: "cascade" }) |
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.
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.
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.
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.
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.
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
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.
@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?
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.
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
//...
},
}
}
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.
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.
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.
Most adapters seem to connect/disconnect automatically, actually. In my experience, it is only TypeORM that needs manual connection handling so far. 😕
verificationToken: async ({ identifier, token }) => { | ||
const verificationToken = | ||
await sequelize.models.verificationToken.findOne({ | ||
where: { identifier, token }, | ||
}) |
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.
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 }) |
account: async ({ provider, providerAccountId }) => { | ||
const account = await sequelize.models.account.findOne({ | ||
where: { provider, providerAccountId }, | ||
}) |
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.
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 }) |
async getUser(id) { | ||
const userInstance = await User.findByPk(id) | ||
|
||
return userInstance?.get({ plain: true }) ?? null |
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.
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! 🙏
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.
Agree! Having an instance returned can be useful but the option for a plain object is always great.
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Thanks for the excellent feedback @balazsorban44.
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 👍 |
packages/sequelize/README.md
Outdated
sequelize.sync() | ||
|
||
export default NextAuth({ | ||
... | ||
adapter | ||
... | ||
}) |
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.
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
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, just a few more suggestions.
packages/sequelize/README.md
Outdated
|
||
export default NextAuth({ | ||
... | ||
adapter: SequelizeAdapter(sequelize, { synchronize: true }) |
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.
adapter: SequelizeAdapter(sequelize, { synchronize: true }) | |
adapter: SequelizeAdapter(sequelize, { synchronize: false }) |
As part of the rephrased section over.
packages/sequelize/README.md
Outdated
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: | ||
|
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.
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.
packages/sequelize/README.md
Outdated
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) | ||
... | ||
}) |
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.
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.
packages/sequelize/src/index.ts
Outdated
client: Sequelize, | ||
options?: SequelizeAdapterOptions | ||
): Adapter { | ||
const { models, synchronize } = 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.
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
@luke-j Hi Luke, thanks for working on this! Do you have any updates? |
@balazsorban44 I've defaulted |
@luke-j thank you so much for the hard work |
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! |
"access": "public" | ||
}, | ||
"scripts": { | ||
"test": "jest", |
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.
Don't need to run build on test, we will build before publishing anyway.
"dist" | ||
], | ||
"peerDependencies": { | ||
"next-auth": ">4 || 4.0.0-beta.1 - 4.0.0-beta.x", |
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.
Corrected this as per #274
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.
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! 💚
Enjoy |
Reasoning 💡
This follows on from #73 (although doesn't share much code) and creates an adapter for Sequelize. Appreciate any feedback!
Checklist 🧢
I still have to test this more thoroughly locally, but would appreciate any feedback in the meantime.