adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.08k stars 196 forks source link

withSchema method doesn't work #282

Closed yariksav closed 6 years ago

yariksav commented 6 years ago

Environment Knex version: knex@0.14.2 adonis-lucid version: lucid@4.1.2

Hello, seems I have found major bug, withSchema method doesn't work correctly:

how I've used migration:

  up () {
    this.withSchema('processing').create('protocols', (table) => {
      table.increments()
      table.string('name', 64)
      table.string('classname', 128)
      table.string('config')
      table.timestamps()
      table.boolean('active').notNullable().defaultTo(1)
    })
  }

this always generate sql without schema: create table "protocols" ("id" serial primary key, "name" varchar(64), "classname" varchar(128), "config" varchar(255), "created_at" timestamptz, "updated_at" timestamptz, "active" boolean not null default '1') Clarification:

https://github.com/adonisjs/adonis-lucid/blob/fd225d82f89f7c6de750afa2b2cc5cfed77ef7fc/src/Schema/index.js#L74-L77

https://github.com/adonisjs/adonis-lucid/blob/fd225d82f89f7c6de750afa2b2cc5cfed77ef7fc/src/Schema/index.js#L50-L52

and when calling this.db.schema it return always NEW INSTANCE of schemabuilder

https://github.com/adonisjs/adonis-lucid/blob/fd225d82f89f7c6de750afa2b2cc5cfed77ef7fc/src/Database/index.js#L137-L139

according to logic of KNEX:

https://github.com/tgriesser/knex/blob/9f8d2eddbf4f42fe8c9fd9f2b2162a98d8cf2c12/src/util/make-knex.js#L99-L103

    schema: {
      get() {
        return client.schemaBuilder()
      }
    },

and

https://github.com/tgriesser/knex/blob/9f8d2eddbf4f42fe8c9fd9f2b2162a98d8cf2c12/src/client.js#L77-L79

  schemaBuilder() {
    return new SchemaBuilder(this)
  },

therefore after calling next time this.schema in builder - always create new object and erase options

yariksav commented 6 years ago

I've found solution of problem: in https://github.com/adonisjs/adonis-lucid/blob/fd225d82f89f7c6de750afa2b2cc5cfed77ef7fc/src/Schema/index.js#L50-L52

change to

  get schema () {
    return this._schemaInstance = this._schemaInstance || this.db.schema
  }
yariksav commented 6 years ago

Oh, message above work's only for toSql, but If i start real migration - bau repeated again.. It because new transaction trx creates new schema, where options with schema is undefigned

const trx = await this.db.beginTransaction()
   .....
    await trx.schema[action.name](...action.args)

https://github.com/adonisjs/adonis-lucid/blob/fd225d82f89f7c6de750afa2b2cc5cfed77ef7fc/src/Schema/index.js#L380-L391

thetutlage commented 6 years ago

Want to give it a try using the develop branch from Github