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

experimentalAutoMigrate (draft) #259

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vmasdani
Copy link
Contributor

No description provided.

@vmasdani vmasdani changed the title experimental experimentalAutoMigrate (draft) May 11, 2021
Copy link
Owner

@eveningkid eveningkid left a comment

Choose a reason for hiding this comment

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

Hey,

I know there are many comments but I hope they can be helpful to you!

Have a great week :)



if (dialect === "mongo") {
throw ("Auto-migration only works on SQL.");
Copy link
Owner

Choose a reason for hiding this comment

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

The rest of the codebase uses Error usually:

throw new Error("Auto-migration only works on SQL.");

Comment on lines +179 to +196
static async createTableOnlyTable() {
if (this._isCreatedInDatabase) {
throw new Error("This model has already been initialized.");
}

const createQuery = this._options.queryBuilder
.queryForSchema(this)
.table(this.table)
.createTableOnlyTable({
withTimestamps: this.timestamps,
ifNotExists: true,
})
.toDescription();

await this._options.database.query(createQuery);

this._isCreatedInDatabase = true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be great to combine createTable and createTableOnlyTable together (I'm sure you thought of doing it!).

Here's an idea:

static async createTable(options = { withFields: true }) {
  const createQuery = this._options.queryBuilder
    .queryForSchema(this)
    .table(this.table);
  
  if (options.withFields) {
    // NOTE: the queryBuilder.createTable signature should be changed to match the changes
    createQuery.createTable({
      fields: this.formatFieldToDatabase(this.fields) as ModelFields,
      defaults: this.formatFieldToDatabase(this.defaults) as ModelDefaults,
      withTimestamps: this.timestamps,
      ifNotExists: true,
    });
  } else {
    createQuery.createTable({
      withTimestamps: this.timestamps,
      ifNotExists: true,
    });
  }
  
  await this._options.database.query(createQuery.toDescription());
  
  this._isCreatedInDatabase = true;
}

console.log(`[experimentalAutoMigrate] syncing table ${this.table}`);
console.log(`[experimentalAutoMigrate] fields:`, this.fields);

for (const key in this.fields) {
Copy link
Owner

Choose a reason for hiding this comment

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

Just a quick note: I think field would be more appropriated here

Comment on lines +227 to +232
const alterQuery = this._queryBuilder
.removeSelect()
.alterTable(this.table)
.addColumn(this.formatFieldToDatabase(key) as string)
.columnType(this.fields[key].toString())
.toDescription();
Copy link
Owner

Choose a reason for hiding this comment

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

This could be more compact (but is it possible?):

const alterQuery = this._queryBuilder
  .table(this.table)
  .addColumn(
    this.formatFieldToDatabase(key) as string,
    // vvv I wonder if this might not be an object sometimes? with autoIncrement, primary, etc?
    this.fields[key].toString()
  )
  .toDescription();

If I can give a piece of advice: when creating functions/methods/etc. I personally think of what I would ideally type first, then of the implementation.

That way, you keep your API simpler (because that's how you'd ideally have written it in the first place!)

console.log(`[experimentalAutoMigrate] syncing table ${this.table}`);
console.log(`[experimentalAutoMigrate] fields:`, this.fields);

for (const key in this.fields) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm just thinking of another approach: we could SELECT * FROM table and see what fields we get in return?

Because with the current implementation, we're making a lot of queries. If we could just make one query, see what fields we have and the ones missing would then be created with one ALTER query too.

// User { name, age, address }
// See how this could be created. This is just an example
const fieldsInDatabase = {
  name: true,
  age: true,
};

const missingFields = [];

for (const field of this.fields) {
  if (field in fieldsInDatabase) {
    continue;
  }
  
  const fieldType = ...;
  
  missingFields.push({ field, type: fieldType });
}

if (missingFields.length === 0) {
  return;
}

const addMissingFieldsQuery = this._queryBuilder
  .table(this.table)
  .addColumns(missingFields)
  .toDescription();

await this._options.database.query(addMissingFieldsQuery);

This I guess would run faster especially if there are many models, what do you think?

Comment on lines +139 to +152
createTableOnlyTable({
withTimestamps,
ifNotExists,
}: {
withTimestamps: boolean;
ifNotExists: boolean;
}) {
this._query.type = "create";
this._query.ifExists = ifNotExists ? false : true;
this._query.fields = {};
this._query.timestamps = withTimestamps;
return this;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Based on previous comment, could be combined with createTable

Comment on lines +169 to +173
removeSelect() {
delete this._query.select;
return this;
}

Copy link
Owner

Choose a reason for hiding this comment

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

I think this one isn't necessary, but I'm sure you had a problem somewhere when generating the query?

// Alter table for auto-migration
if (query.table && query.type === "alter table") {
// [autoMigrate] don't use queryBuilder but just use schema.alterTAble
const alterTableQuery = (SQLQueryBuilder as any)({
Copy link
Owner

Choose a reason for hiding this comment

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

Could queryBuilder be used there instead?

// break;
// }

table.text(query.addColumn);
Copy link
Owner

Choose a reason for hiding this comment

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

I remember you found a way to solve this with dex :D

for (const model of this._models) {
try {
console.log('[experimentalAutoMigrate] migrating table', model.table);
await model.createTableOnlyTable();
Copy link
Owner

Choose a reason for hiding this comment

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

So this could be:

await model.createTable({ withFields: false });

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

Successfully merging this pull request may close these issues.

2 participants