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

problem with db.sync() #283

Open
ralyodio opened this issue Jul 21, 2021 · 18 comments
Open

problem with db.sync() #283

ralyodio opened this issue Jul 21, 2021 · 18 comments

Comments

@ralyodio
Copy link

error: Uncaught (in promise) SqliteError: index email already exists

I have a subscriptions table with email as a field.

Using db.sync() causes this error. I'm not sure if I need it. It appears to work without it, but I'm not sure.

Can someone clarify the point of db.sync()

@ralyodio
Copy link
Author

import { Model, DataTypes } from "https://deno.land/x/denodb/mod.ts";
import { db } from '../db.ts';

class Subscriptions extends Model {
  static table = 'subscriptions';
  static timestamps = true;

  static fields = {
    id: { primaryKey: true, default: crypto.randomUUID() },
    email: {
      type: DataTypes.STRING,
      unique: true,
    },
  };
}

db.link([Subscriptions]);
db.sync();

export default Subscriptions

This is the code casuing the crash:

$ deno run  --allow-all index.ts
Check file:///home/ettinger/www/iptvfish/iptvfish-api/index.ts
error: Uncaught (in promise) TypeError: table[type] is not a function
      instruction = table[type](...fieldNameArgs);
                               ^
    at addFieldToSchema (https://deno.land/x/[email protected]/lib/helpers/fields.ts:81:32)
    at TableBuilder._fn (https://deno.land/x/[email protected]/lib/translators/sql-translator.ts:152:15)
    at TableBuilder.toSQL (https://raw.githubusercontent.com/aghussb/dex/master/lib/schema/tablebuilder.js:45:12)
    at SchemaCompiler_SQLite3.createTableIfNotExists (https://raw.githubusercontent.com/aghussb/dex/master/lib/schema/compiler.js:89:25)
    at SchemaCompiler_SQLite3.toSQL (https://raw.githubusercontent.com/aghussb/dex/master/lib/schema/compiler.js:72:26)
    at SchemaBuilder.toSQL (https://raw.githubusercontent.com/aghussb/dex/master/lib/schema/builder.js:81:43)
    at SchemaBuilder.Target.toQuery (https://raw.githubusercontent.com/aghussb/dex/master/lib/interface.js:9:21)
    at SchemaBuilder.toString (https://raw.githubusercontent.com/aghussb/dex/master/lib/schema/builder.js:77:15)
    at SQLTranslator.translateToQuery (https://deno.land/x/[email protected]/lib/translators/sql-translator.ts:225:25)
    at SQLite3Connector.query (https://deno.land/x/[email protected]/lib/connectors/sqlite3-connector.ts:54:36)

@chovyprognos
Copy link

bump

@schuelermine
Copy link

schuelermine commented Jul 29, 2021

This is likely because you have not specified a type for id. Try this instead:

/* … */
    id: {
      ...DataTypes.string(36),
      primaryKey: true
    },
/* … */

@schuelermine
Copy link

schuelermine commented Jul 29, 2021

That code uses ES spread syntax. If you don’t want to use that, do this:

/* … */
    id: {
      type: DataTypes.STRING,
      length: 36,
      primaryKey: true
    },
/* … */

@schuelermine
Copy link

As far as I can tell, the documentation also seems to describe putting defaults in their own static field, like this:

/* … */
  static defaults = {
    id: "00000000-0000-0000-0000-000000000000"
  }
/* … */

@schuelermine
Copy link

Note that it is likely not a good idea to specify a default ID. While your code may look like it generates a random UUID for every entry, it actually generates one UUID when the model is defined, which is used throughout. It may even be invalid to provide a default primary key.

@schuelermine
Copy link

schuelermine commented Jul 29, 2021

bump

@chovyprognos

That’s not how GH issues work

@ralyodio
Copy link
Author

ralyodio commented Sep 5, 2021

is there any kind of "beforeCreate" hook to populate an id field? or should I do that in my controller.

@ralyodio
Copy link
Author

ralyodio commented Sep 5, 2021

I also still get an error with await db.sync() -- this feature doesn't seem to work on existing tables...but if i add a table how can I run it?

error: SqliteError: index email already exists
    at DB._error (file://$deno$/bundle.js:53139:16)
    at DB.query (file://$deno$/bundle.js:53027:24)
    at file://$deno$/bundle.js:67011:44
    at Array.map (<anonymous>)
    at SQLite3Connector.query (file://$deno$/bundle.js:67010:36)
    at Database1.query (file://$deno$/bundle.js:67301:51)
    at Function.createTable (file://$deno$/bundle.js:65925:38)
    at Database1.sync (file://$deno$/bundle.js:67285:25)
    at file://$deno$/bundle.js:67422:11

@slim-hmidi
Copy link

I have the same issue, db.sync() throws an error when drop set to false.

@DomThePorcupine
Copy link

What is the error you get when you run sync?

@slim-hmidi
Copy link

I have 3 models User, Product and Review.
I don't want to erase the tables when I start the server because drop set to true removes everything.
with drop: false I got relation username is already exists.

username is a column from users table and it contains a constraint to guarantee that is unique.
With all model fields with unique set to true I got this issue.

@slim-hmidi
Copy link

I think sync() should verify first if the table exists or not then creates it.
Currently without any options it creates the models automatically.

@DomThePorcupine
Copy link

I think that's what it's doing under the hood (CREATE IF NOT EXISTS):

database.ts

async sync(options: SyncOptions = {}) {
    if (options.drop) {
      for (let i = this._models.length - 1; i >= 0; i--) {
        await this._models[i].drop();
      }
    }

    ...
    
    for (const model of this._models) {
      await model.createTable();
    }
  }

model.ts

static async createTable() {
    ....
    const createQuery = this._options.queryBuilder
      .queryForSchema(this)
      .table(this.table)
      .createTable(
        this.formatFieldToDatabase(this.fields) as ModelFields,
        this.formatFieldToDatabase(this.defaults) as ModelDefaults,
        {
          withTimestamps: this.timestamps,
          ifNotExists: true,
        },
      )
      .toDescription();

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

   ....
  }

I'd be interested to see how I could reproduce the error you're seeing, if you'd care to share some code snippets

@slim-hmidi
Copy link

slim-hmidi commented Jan 8, 2022

I took a look before at that function.
To reproduce the issue you can create those models:

import { DataTypes, Model } from "https://deno.land/x/[email protected]/mod.ts";
import {
  Database,
  PostgresConnector,
} from "https://deno.land/x/[email protected]/mod.ts";


class UserModel extends Model {
  static table = "users";
  static timestamps = true;

  static fields = {
    id: { primaryKey: true, autoIncrement: true, type: DataTypes.INTEGER },
    username: { type: DataTypes.STRING, unique: true },
    email: DataTypes.STRING,
    password: DataTypes.STRING,
  };
  }
  
  class ProductModel extends Model {
  static table = "products";
  static timestamps = true;

  static fields = {
    id: { primaryKey: true, autoIncrement: true, type: DataTypes.INTEGER },
    name: { type: DataTypes.STRING, unique: true },
    quantity: DataTypes.INTEGER
  };
}




const connector = new PostgresConnector({
  database: "db",
  host:  "127.0.0.1",
  username: "postgres",
  password: "postgres",
  port:  5432,
});

const db = new Database(connector);


const syncDB = async () => {
  db.link([UserModel, ProductModel])
  await db.sync()
}

First time, it will work correctly but if you restart the server or reload it with denon you will get an issue.

This is the error I get:

NOTICE: relation "users" already exists, skipping
error: Uncaught (in promise) PostgresError: relation "username" already exists
          error = new PostgresError(parseNoticeMessage(current_message));
                  ^
    at Connection.#simpleQuery (https://deno.land/x/[email protected]/connection/connection.ts:626:19)
    at async Connection.query (https://deno.land/x/[email protected]/connection/connection.ts:875:16)
    at async PostgresConnector.query (https://deno.land/x/[email protected]/lib/connectors/postgres-connector.ts:76:22)
    at async Database.query (https://deno.land/x/[email protected]/lib/database.ts:240:21)
    at async Function.createTable (https://deno.land/x/[email protected]/lib/model.ts:172:5)
    at async Database.sync (https://deno.land/x/[email protected]/lib/database.ts:210:7)
  

@DomThePorcupine
Copy link

Ah yeah, it looks like the table creation generates two commands:

create table if not exists "users" ("id" serial primary key, "username" varchar(255) not null, "email" varchar(255), "password" varchar(255), "created_at" timestamptz not null default CURRENT_TIMESTAMP, "updated_at" timestamptz not null default CURRENT_TIMESTAMP);

and

alter table "users" add constraint "username" unique ("username")

The second one is the one that's failing. Seems like we could check if the constraint already exists before adding it to the query - but I'd guess it'd be easier for denodb to implement some form of migrations instead of building up the sync method...

@slim-hmidi
Copy link

Thanks for that, so I need to do a workaround to accomplish that until it will be taken into consideration in the future.

@slim-hmidi
Copy link

slim-hmidi commented Jan 9, 2022

I created a migration file to accomplish my task (it's not clean but it does the work) and I think that the sync method is overengineering and contains a lot of logic that makes things not easy to do.
It is better to rethink the design of this function to not contain a lot of details in the background.
And the migrations can be simple enough by creating a cli like knex, one for up and the other for down, and user will trigger them manually whenever he needs to update the database.

Yarilo added a commit to Yarilo/azar that referenced this issue Jul 15, 2022
(Fine, I'll do it myself...)

This commit represents a branch to replace the denodb orm with deno-postgres and
a small db provider to access it.

Current DenoDB project had many issues and specially had some troubles sync the db
without dropping the info: eveningkid/denodb#283, eveningkid/denodb#258

So everytime the server restarted, the db was wiped out and therefore it was unusable for production.

After this change, the db is now "persistent" (and the code is arguably cleaner/straightforward)
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

No branches or pull requests

5 participants