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

Remove deprecated config.db.useMigrations, change keystone prisma behaviour #9090

Merged
merged 3 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/add-no-server.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@keystone-6/core': patch
---

Fix `--no-server` being ignored by `keystone start`
2 changes: 1 addition & 1 deletion .changeset/fix-frozen-prisma.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
'@keystone-6/core': major
---

Fix `keystone prisma` behaviour to respect `--frozen`, dont generate types or Prisma client when using `--frozen`
Change `keystone prisma` behaviour to first require `keystone build` (or `keystone dev`)
5 changes: 5 additions & 0 deletions .changeset/no-use-migrations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@keystone-6/core': major
---

Remove deprecated `config.db.useMigrations`, use `--with-migrations` process argument or `keystone prisma migrate [dev|deploy]` instead
1 change: 0 additions & 1 deletion packages/core/src/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ export async function getArtifacts (system: System) {
export async function generateArtifacts (cwd: string, system: System) {
const paths = getSystemPaths(cwd, system.config)
const artifacts = await getCommittedArtifacts(system.config, system.graphQLSchema)

await fs.writeFile(paths.schema.graphql, artifacts.graphql)
await fs.writeFile(paths.schema.prisma, artifacts.prisma)
return artifacts
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/lib/defaults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ export function resolveDefaults <TypeInfo extends BaseKeystoneTypeInfo> (config:
enableLogging: config.db.enableLogging === true ? ['query']
: config.db.enableLogging === false ? []
: config.db.enableLogging ?? [],
useMigrations: config.db.useMigrations ?? false
},
graphql: {
...config.graphql,
Expand Down Expand Up @@ -148,4 +147,4 @@ export function resolveDefaults <TypeInfo extends BaseKeystoneTypeInfo> (config:
publicPages:config.ui?.publicPages ?? [],
},
}
}
}
155 changes: 3 additions & 152 deletions packages/core/src/lib/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { createDatabase, uriToCredentials, type DatabaseCredentials } from '@pri
import { Migrate } from '@prisma/migrate'
import chalk from 'chalk'
import { ExitError } from '../scripts/utils'
import { confirmPrompt, textPrompt } from './prompts'
import { confirmPrompt } from './prompts'

// we don't want to pollute process.env.DATABASE_URL so we're
// setting the env variable _just_ long enough for Migrate to
Expand Down Expand Up @@ -154,9 +154,9 @@ export async function pushPrismaSchemaToDatabase (

if (!interactive) return
if (migration.warnings.length === 0 && migration.executedSteps === 0) {
console.info(`✨ The database is already in sync with the Prisma schema`)
console.info(`✨ Database unchanged`)
} else {
console.info(`✨ Your database is now in sync with your schema`)
console.info(`✨ Database synchronized with Prisma schema`)
}
}

Expand All @@ -174,155 +174,6 @@ function logWarnings (warnings: string[]) {
}
}

export async function deployMigrations (schemaPath: string, dbUrl: string) {
return withMigrate(schemaPath, async migrate => {
const migration = await runMigrateWithDbUrl(dbUrl, undefined, () => migrate.applyMigrations())
if (migration.appliedMigrationNames.length === 0) {
console.info(`✨ The database is already in sync with your migrations`)
} else {
console.info(`✨ Your database is now in sync with your migrations`)
}
})
}

export async function devMigrations (
Copy link
Member Author

@dcousens dcousens Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use keystone prisma migrate dev instead

dbUrl: string,
shadowDbUrl: string | undefined,
prismaSchema: string,
schemaPath: string,
resetDb: boolean
) {
const created = await createDatabase(dbUrl, path.dirname(schemaPath))
if (created) {
const credentials = uriToCredentials(dbUrl)
console.log(
`✨ ${credentials.type} database "${credentials.database}" created at ${getDbLocation(
credentials
)}`
)
}

return withMigrate(schemaPath, async migrate => {
if (!migrate.migrationsDirectoryPath) {
console.error('No migrations directory path')
throw new ExitError(1)
}

const { migrationsDirectoryPath } = migrate

if (resetDb) {
await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () => migrate.reset())
console.log('✨ Your database has been reset')
} else {
// see if we need to reset the database
// note that the other action devDiagnostic can return is createMigration
// that doesn't necessarily mean that we need to create a migration
// it only means that we don't need to reset the database
const devDiagnostic = await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () =>
migrate.devDiagnostic()
)

// when the action is reset, the database is somehow inconsistent with the migrations so we need to reset it
// (not just some migrations need to be applied but there's some inconsistency)
if (devDiagnostic.action.tag === 'reset') {
const credentials = uriToCredentials(dbUrl)
console.log(`${devDiagnostic.action.reason}

We need to reset the ${credentials.type} database "${credentials.database}" at ${getDbLocation(
credentials
)}.`)
const confirmedReset = await confirmPrompt(
`Do you want to continue? ${chalk.red('All data will be lost')}`
)
console.info() // empty line

if (!confirmedReset) {
console.error('Reset cancelled')
throw new ExitError(0)
}

// do the reset
await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () => migrate.reset())
}
}
const { appliedMigrationNames } = await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () => migrate.applyMigrations())

// inform user about applied migrations now
if (appliedMigrationNames.length) {
console.info(`✨ The following migration(s) have been applied:`)
for (const id of appliedMigrationNames) {
console.info(` - ${chalk.cyan.bold(id)}`)
}
}

// evaluateDataLoss basically means "try to create a migration but don't write it"
// so we can tell the user whether it can be executed and if there will be data loss
const evaluateDataLossResult = await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () =>
migrate.evaluateDataLoss()
)

// if there are no steps, there was no change to the prisma schema so we don't need to create a migration
if (evaluateDataLossResult.migrationSteps) {
console.log('✨ There has been a change to your Keystone schema that requires a migration')
const migrationCanBeApplied = !evaluateDataLossResult.unexecutableSteps.length

// see the link below for what "unexecutable steps" are
// https://github.com/prisma/prisma-engines/blob/c65d20050f139a7917ef2efc47a977338070ea61/migration-engine/connectors/sql-migration-connector/src/sql_destructive_change_checker/unexecutable_step_check.rs
// the tl;dr is "making things non null when there are nulls in the db"
if (!migrationCanBeApplied) {
logUnexecutableSteps(evaluateDataLossResult.unexecutableSteps.map(x => x.message))
}
// warnings mean "if the migration was applied to the database you're connected to, you will lose x data"
// note that if you have a field where all of the values are null on your local db and you've removed it, you won't get a warning here.
// there will be a warning in a comment in the generated migration though.
if (evaluateDataLossResult.warnings.length) {
logWarnings(evaluateDataLossResult.warnings.map(x => x.message))
}

console.log() // for an empty line
const migrationNameInput = await textPrompt('Name of migration')

// 200 characters is the limit from Prisma
// see https://github.com/prisma/prisma/blob/c6995ebb6f23996d3b48dfdd1b841e0b5cf549b3/packages/migrate/src/utils/promptForMigrationName.ts#L12
const migrationName = migrationNameInput.replace(/[^A-Za-z0-9_]/g, '_').slice(0, 200)

// note this only creates the migration, it does not apply it
const { generatedMigrationName } = await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () =>
migrate.createMigration({
migrationsDirectoryPath,
// https://github.com/prisma/prisma-engines/blob/11dfcc85d7f9b55235e31630cd87da7da3aed8cc/migration-engine/core/src/commands/create_migration.rs#L16-L17
// draft means "create an empty migration even if there are no changes rather than exiting"
// because this whole thing only happens when there are changes to the schema, this can be false
// (we should also ofc have a way to create an empty migration but that's a separate thing)
draft: false,
prismaSchema,
migrationName,
})
)

console.log(`✨ A migration has been created at migrations/${generatedMigrationName}`)

const shouldApplyMigration =
migrationCanBeApplied &&
(await confirmPrompt('Would you like to apply this migration?', false))

if (shouldApplyMigration) {
await runMigrateWithDbUrl(dbUrl, shadowDbUrl, () => migrate.applyMigrations())
console.log('✅ The migration has been applied')
} else {
console.error('Please edit the migration and try again')
throw new ExitError(0)
}
} else {
if (appliedMigrationNames.length) {
console.log('✨ Your migrations are up to date, no new migrations need to be created')
} else {
console.log('✨ Your database is up to date, no migrations need to be created or applied')
}
}
})
}

function getDbLocation (credentials: DatabaseCredentials): string {
if (credentials.type === 'sqlite') {
return credentials.uri!
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/scripts/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export async function cli (cwd: string, argv: string[]) {
}

if (command === 'start') {
return start(cwd, defaultFlags(flags, { ui: true, withMigrations: false }))
return start(cwd, defaultFlags(flags, { server: true, ui: true, withMigrations: false }))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start --with-migrations --no-server might be redundant by comparison to keystone prisma migrate deploy, but, I think start --no-server being ignored is worse

}

if (command === 'prisma') {
Expand Down
18 changes: 4 additions & 14 deletions packages/core/src/scripts/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import express from 'express'
import { printSchema } from 'graphql'
import esbuild, { type BuildResult } from 'esbuild'
import { generateAdminUI } from '../admin-ui/system'
import { devMigrations, pushPrismaSchemaToDatabase } from '../lib/migrations'
import { pushPrismaSchemaToDatabase } from '../lib/migrations'
import {
createSystem,
getBuiltKeystoneConfiguration,
Expand Down Expand Up @@ -155,15 +155,7 @@ export async function dev (
await generateTypes(cwd, system)
await generatePrismaClient(cwd, system)

if (system.config.db.useMigrations) {
await devMigrations(
system.config.db.url,
system.config.db.shadowDatabaseUrl,
prismaSchema,
paths.schema.prisma,
false
)
} else if (dbPush) {
if (dbPush) {
await pushPrismaSchemaToDatabase(
system.config.db.url,
system.config.db.shadowDatabaseUrl,
Expand Down Expand Up @@ -270,10 +262,8 @@ export async function dev (
// we only need to test for the things which influence the prisma client creation
// and aren't written into the prisma schema since we check whether the prisma schema has changed above
if (
JSON.stringify(newSystem.config.db.enableLogging) !==
JSON.stringify(system.config.db.enableLogging) ||
newSystem.config.db.url !== system.config.db.url ||
newSystem.config.db.useMigrations !== system.config.db.useMigrations
JSON.stringify(newSystem.config.db.enableLogging) !== JSON.stringify(system.config.db.enableLogging)
|| newSystem.config.db.url !== system.config.db.url
) {
console.error('Your database configuration has changed, please restart Keystone')
return stop(null, true)
Expand Down
32 changes: 12 additions & 20 deletions packages/core/src/scripts/prisma.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,31 @@
import fs from 'node:fs/promises'
import { spawn } from 'node:child_process'
import esbuild from 'esbuild'
import {
createSystem,
getBuiltKeystoneConfiguration
getBuiltKeystoneConfigurationPath,
getBuiltKeystoneConfiguration,
} from '../lib/createSystem'
import {
generateArtifacts,
generatePrismaClient,
generateTypes,
validateArtifacts,
} from '../artifacts'
import { getEsbuildConfig } from '../lib/esbuild'
import { ExitError } from './utils'

export async function prisma (cwd: string, args: string[], frozen: boolean) {
if (frozen) {
args = args.filter(arg => arg !== '--frozen')
} else {
await esbuild.build(getEsbuildConfig(cwd))
// TODO: this cannot be changed for now, circular dependency with getSystemPaths, getEsbuildConfig
const builtConfigPath = getBuiltKeystoneConfigurationPath(cwd)

// this is the compiled version of the configuration which was generated during the build step
if (!(await fs.stat(builtConfigPath).catch(() => null))) {
console.error('🚨 keystone build must be run before running keystone prisma')
throw new ExitError(1)
}

// TODO: this cannot be changed for now, circular dependency with getSystemPaths, getEsbuildConfig
const config = getBuiltKeystoneConfiguration(cwd)
const system = createSystem(config)

if (frozen) {
await validateArtifacts(cwd, system)
console.log('✨ GraphQL and Prisma schemas are up to date')
} else {
await generateArtifacts(cwd, system)
console.log('✨ Generated GraphQL and Prisma schemas')

await generatePrismaClient(cwd, system)
await generateTypes(cwd, system)
}
await validateArtifacts(cwd, system)
console.log('✨ GraphQL and Prisma schemas are up to date')

return new Promise<void>((resolve, reject) => {
const p = spawn('node', [require.resolve('prisma'), ...args], {
Expand Down
12 changes: 8 additions & 4 deletions packages/core/src/scripts/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import {
} from '../lib/createSystem'
import { createExpressServer } from '../lib/createExpressServer'
import { createAdminUIMiddlewareWithNextApp } from '../lib/createAdminUIMiddleware'
import { deployMigrations } from '../lib/migrations'
import { withMigrate, runMigrateWithDbUrl } from '../lib/migrations'
import { ExitError } from './utils'
import type { Flags } from './cli'

export async function start (
cwd: string,
{ ui, withMigrations }: Pick<Flags, 'ui' | 'withMigrations'>
{ server, ui, withMigrations }: Pick<Flags, 'server' | 'ui' | 'withMigrations'>
) {
console.log('✨ Starting Keystone')

Expand All @@ -32,10 +32,14 @@ export async function start (
const keystone = system.getKeystone(prismaClient)

if (withMigrations) {
console.log('✨ Applying database migrations')
await deployMigrations(paths.schema.prisma, system.config.db.url)
console.log('✨ Applying any database migrations')
await withMigrate(paths.schema.prisma, async migrate => {
const { appliedMigrationNames } = await runMigrateWithDbUrl(system.config.db.url, undefined, () => migrate.applyMigrations())
console.log(appliedMigrationNames.length === 0 ? `✨ No database migrations to apply` : `✨ Database migrated`)
})
}

if (!server) return
console.log('✨ Connecting to the database')
await keystone.connect()

Expand Down
3 changes: 0 additions & 3 deletions packages/core/src/types/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ export type KeystoneConfig<TypeInfo extends BaseKeystoneTypeInfo = BaseKeystoneT
prismaSchemaPath?: string

extendPrismaSchema?: (schema: string) => string

/** @deprecated TODO: remove in breaking change */
useMigrations?: boolean
}

graphql?: {
Expand Down
8 changes: 5 additions & 3 deletions tests/cli-tests/__snapshots__/prisma.test.ts.snap
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`keystone prisma exits with the same code as the prisma child process exits with 1`] = `
"? Generated GraphQL and Prisma schemas
"? GraphQL and Prisma schemas are up to date

! Unknown command "bad-thing"

Expand Down Expand Up @@ -59,12 +59,14 @@ Examples

Display Prisma debug info
$ prisma debug

"
`;

exports[`keystone prisma uses the db url in the keystone config 1`] = `
"? Generated GraphQL and Prisma schemas
"? GraphQL and Prisma schemas are up to date
Prisma schema loaded from schema.prisma
Datasource "sqlite": SQLite database "app.db" at "file:./app.db"
Error: P1003: Database app.db does not exist at ./app.db"
Error: P1003: Database app.db does not exist at ./app.db
"
`;
Loading