PhilWaldmann / openrecord

Make ORMs great again!
https://openrecord.js.org
MIT License
486 stars 38 forks source link

Integer keys #113

Closed nibarulabs closed 4 years ago

nibarulabs commented 4 years ago

Hello. I love the project, thank you. I've played with many Node ORMs and this is the only one I think actually works as I expect.

I have noticed an issue however. Id's are marked as integer, this includes foreign keys. I use Postgres and while integer will work, I think bigint is the industry approach. Granted, you will need over 2bln rows for this to be an issue, but I have had friends create successful projects where they forgot this (tsk tsk on them) limitation and ran into issues.

A work around for my migrations is do to something like

this.type('bigint', 'user_id');

which, luckily I can do. But I can't use the stampable plugin because it is hardcoded with integers. I dug through the code and did not see a bigint datatype, nor did I see integer doing a magic conversion to the 64-bit variant.

I just wanted to raise this issue for discussion - maybe I'm missing something?

Thanks.

PhilWaldmann commented 4 years ago

Hi @nibarulabs,

you are totally right to use bigint for relations in Postgres. At the moment there is shared code for migrations wich works in SQLite, MySQL, Postgres and Oracle. The easiest way to get started was to use integer and it seems that I totally forgot to add database specific rules to the migration layer.

I think what we need is a this.references('field_name, 'table_name', {opts...}) method to define relations. Which should also add foreign key constrains to the table.

What do you think?

nibarulabs commented 4 years ago

Hey, @PhilWaldmann thx for quick reply. :)

Totally understandable approach to getting it done and out there. I have run into a few minor issues regarding all this.

That's not a ton of work, but it's still a list. I have found work arounds for all of these, so not the end of the world. This is a typical migration for me:

    this.createTable('users', function () {
        this.uuid('uuid');
        this.string('email', {unique: true});
        this.string('password');
        this.string('first_name');
        this.string('last_name');
        this.string('confirm_email_token');
        this.type('bigint', 'activated_ts');

        this.stampable();
    });

So, you can see where I force bigints (I use epoch timetamps quite a bit as well which have to be bigger than integer). I'm guessing an option for bigint to be used as the pk above would work. Somewhere. I'm kinda riffing now, but I'm sure you get the gist.

My project will be going live at the end of the month. This lib was honestly make or break and it's helped me get things over the finish line. Thanks again. 👍

PhilWaldmann commented 4 years ago

I've published a new version. this.id('name') will now use bigint. this.userstamp() will use id('creator_id') and id('updater_id') where is now a dedicated bigint type

regarding references: I forgot that this feature is already implemented. See references option

And thanks for the nice feedback =)

nibarulabs commented 4 years ago

Wow, awesome! Thanks so much for all the work you've done.