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

Support composite indexes (also primary) and foreign key associations #1485

Closed
ruifortes opened this Issue Mar 10, 2014 · 25 comments

Comments

@ruifortes
Contributor

ruifortes commented Mar 10, 2014

Not trivial to implement but surely one feature that would make sequelize greater and one big step closer to "production ready" "de facto ORM for Node" :)

Column order is important so defining it in table and not columns definition seams reasonable.
Starting by the end how about a "indexes" property in sequelize.define "option" argument. Something like:

indexes:[
    {
        type:"primary" // or "key", "unique", "fulltext",etc
        columns:["userId","classId"]
        name:"mypk" //optional, could default to generated name
    }
]

Of course...defining indexes in columns should also remain possible for single column indexes.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Mar 10, 2014

I agree, would be a great feature. But hardly a showstopper for being "production ready" since Sequelize is used in quite a few production environments :)

Option should probably be named 'indices' though, or i'm not sure, it's the proper but it's always felt off to me.

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 10, 2014

Something along the syntax you suggested looks good to me. Definitely name it indices though, in the spirit of technical correctness! :)

I don't think you should be able to specify primary keys this way though, since yoy can already to that on the model definition. I don't think column order is important in primary keys, is it?

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 10, 2014

http://dba.stackexchange.com/questions/15473/does-the-order-of-keys-in-compound-primary-key-make-any-difference
So it may actually have an effect. However, in that case I think I would define another index to target that case

@ruifortes

This comment has been minimized.

Contributor

ruifortes commented Mar 10, 2014

Since table definitions are not changing in runtime I guess it would be simpler to use only one indexes/indices array.
Internally a variable should be created to store PK as a result of evaluating column definitions and indexes array to choose the right PK (or throw an error if defined in both places).
Associations using "joinTableModel" should be checked since can also define pk on target table, right?
Eventually some kind of default logic could be applied. "Type" property in indexes could default to "key" and if no PK is defined first found "unique" and "key" would take its place.

About indexes/indices...they're both gramatically correct but DBA culture seams to choose first one...

http://federalist.wordpress.com/2006/09/28/grammar-indexes-vs-indices/
http://www.postgresql.org/message-id/8182.986230075@sss.pgh.pa.us

@mickhansen silly me...I meant "de facto ORM for Node" :)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Mar 10, 2014

I prefer 'indexes', and if there's actually a DBA precedent for it, hurray :)
join/through models will by default (moving forward) have a primary key id and a composite unique for the foreign keys.

Composite primary keys aren't really supported currently so defining primary keys outside of attributes would only have an affect on .sync().

@activestylus

This comment has been minimized.

activestylus commented Mar 13, 2014

+1 This would be a killer feature

@shkkmo

This comment has been minimized.

shkkmo commented Apr 20, 2014

+1 I can't use sequelize for my project without support for multi-column indices.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 20, 2014

@shkkmo time to provide a PR then ;) I must admit i get by just fine doing my indices manually.

@shkkmo

This comment has been minimized.

shkkmo commented Apr 20, 2014

@mickhansen I'll look into it once I get a little more familiar with node and sequelize.

In the meantime, I'll look into adding the multi-column indices via a manual addition to the migration files:
http://sequelizejs.com/docs/latest/migrations#functions

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 21, 2014

@shkkmo great, i'm around if you need any help. Not that we won't get to multi column indices eventually, but there are currently a few higher priorities.

@andreipet

This comment has been minimized.

andreipet commented May 20, 2014

We can use something like this when defining models:

Model = sequelize.define('Model',
{...},
{
  hooks: {
    afterCreate: function(model, fn) {
      var query = Model.QueryGenerator.addIndexQuery(Model.tableName, ['col1', 'col2']);
      return sequelize.query(query); 
    }
  }
});
@janmeier

This comment has been minimized.

Member

janmeier commented May 20, 2014

@andreipet The afterCreate hook is called after each time a Model.create call finished, not when the table is being created in the DB, so

@andreipet

This comment has been minimized.

andreipet commented May 20, 2014

Yes. My bad. I need a hook on model create not instance create :)

@mlegenhausen

This comment has been minimized.

Contributor

mlegenhausen commented May 21, 2014

This could be an idea for a new hook afterTableCreate or something like this?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 21, 2014

We have talked about beforeSync/afterSync - Although a sync doesn't necessarily garantuee a table create, but i suppose we could build that in.

@RichAyotte

This comment has been minimized.

RichAyotte commented May 21, 2014

AOP provides a generic way to hook to any function. Creating before* and after* function names for some or all functions will unnecessarily pollute the namespace with no added benefit.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 21, 2014

@RichAyotte AOP? Could you provide an example?
Hooks provide a ton of benefit, plugins/extensions for one. If you can suggest a better API than the current .hook('beforeafteretc') you're more than welcome to do so

@RichAyotte

This comment has been minimized.

RichAyotte commented May 21, 2014

You can do AOP manually but I like to use a library for convenience. Here's how it would look like with DCL.

var advise = require("dcl/advise");
var db = require(models).db;

advise.before(db, 'sync', function() {
    // Do stuff before sync
})

advise.after(db, 'sync', function() {
    // Do stuff after sync
})

http://www.dcljs.org/docs/advise_js/
https://github.com/cujojs/meld
http://dojotoolkit.org/reference-guide/1.9/dojo/aspect.html

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 21, 2014

Excuse my ignorance (haven't worked with this pattern before) but how would this pattern support async callbacks or promises?

@RichAyotte

This comment has been minimized.

RichAyotte commented May 21, 2014

Here's how you can pass the args and returned values around.

advise.after(db, 'sync', function(origArgs, promise) {
    return promise;
});

The most powerful is the around advise but I left it out for brevity. That's where you can invoke the original function and manipulate its args and return values.

advise.around(db, 'sync', function(origFunc) {
    return function(origArgs) {
        if(userPermissions === 'allowed') {
            return origFunc.call(this, origArgs);
        }
    };
});
@olive75

This comment has been minimized.

olive75 commented Jun 26, 2014

+1 this would be a killer feature

@horiuchi

This comment has been minimized.

horiuchi commented Jul 15, 2014

👍

@vmadman

This comment has been minimized.

vmadman commented Jul 15, 2014

I'm having trouble figuring out how to do non-unique indexes at all. I'm not using migrations because this project is a short-term solution to a problem... but the databases are HUGE, making precise indexing pretty important.

The only reason its needed at all is because Sequelize.sync builds and modifies the tables, etc.. so, we need Sequelize to understand our true intents so that it can create or update the table properly (right?).

I guess I'm not understanding why the simple indices option wouldn't work..

sequelize.define('MyModel, { .. }, {
    indices: [
        "my_index_one": {
            fields: ["field_1", "field_2"],
            primary: false,
            unique: false
        }
    ]
});

While unique (and I assume, primary) indexes are already addressed, they do not account for field order.. and would need to be relocated in this way.

I'm a bit ignorant so please excuse me if I am off base.

@RichAyotte Agreed, an AOP library could resolve the whole discussion on before/after anything and could allow Sequelize to remove the whole concept.. allowing users to select their AOP libs and apply them from the outside.

I'm still coming to terms with these patterns myself... trying to understand why some folks call this "AOP" and others call it "Decoration". Anyway..

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jul 15, 2014

Just haven't seen any good libs/design patterns for async AOP (pref. with promises) yet - So would have to bake our own. And in any case they'd have to be injected into each function anyways since we need to use the async results of before.

As for the indices/indexes option, that is definitely something that would make sense to have in the codebase.

@RichAyotte RichAyotte referenced this issue Sep 21, 2014

Closed

Hooks refactor + new hooks #1852

3 of 5 tasks complete
@dianaflorez

This comment has been minimized.

dianaflorez commented Aug 31, 2016

I did this and working for me... for postgres
(I declared id_pais and codigo_pais how primaryKey: true)

"use strict";

module.exports = function(sequelize, DataTypes) {

var Pais = sequelize.define('pais', {
    id_pais:{
        type: DataTypes.INTEGER,
        primaryKey: true,
        autoIncrement: true
    },
    codigo_pais: {
        type: DataTypes.STRING(3),
        primaryKey: true
    },
    nombre: {
        type: DataTypes.STRING(50),
        allowNull : false
    },
   "createdAt":{
        type: DataTypes.DATE,
        defaultValue : sequelize.fn('now') 
    },
    "updatedAt":{
        type: DataTypes.DATE,
        defaultValue : sequelize.fn('now') 
    }
});

return Pais;
};

//**********************************************
The result in postgres was

-- Table: pais

-- DROP TABLE pais;

CREATE TABLE pais
(
id_pais serial NOT NULL,
codigo_pais character varying(3) NOT NULL,
nombre character varying(50) NOT NULL,
"createdAt" timestamp with time zone DEFAULT now(),
"updatedAt" timestamp with time zone DEFAULT now(),
CONSTRAINT pais_pkey PRIMARY KEY (id_pais, codigo_pais)
)
WITH (
OIDS=FALSE
);
ALTER TABLE pais
OWNER TO diana;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment