Vincit / objection.js

An SQL-friendly ORM for Node.js
https://vincit.github.io/objection.js
MIT License
7.24k stars 635 forks source link

Insert succeeds with knex but not with Model subclass #369

Closed mitchellporter closed 7 years ago

mitchellporter commented 7 years ago

Hey, first off thanks for building this. I just got started using objection.js and I love it. The docs are great as well. I'm trying to insert new objects into Postgres and running into some problems.

I have two models, Team and User. A Team can have many members, and a User can only belong to one Team. So I have a one-to-many relationship, where team is the one and user is the many.

Here is the code for Team:

'use strict';

const Model = require('objection').Model;

class Team extends Model {
    // Table name is the only required property.
    static get tableName() {
        return 'Team';
    }
}

module.exports = Team;

Here is the code for User:

'use strict';

const Model = require('objection').Model;
const Team = require('../teams/team');

class User extends Model {
    // Table name is the only required property
    static get tableName() {
        return 'User';
    }

    // This object defines the relations to other models
    static get relationMappings() {
        return {
            team: {
                relation: Model.BelongsToOneRelation,
                modelClass: Team,
                join: {
                    from: 'User.team',
                    to: 'Team.id'
                }
            }
        };
    }
}

module.exports = User;

Here is the schema code for Team and User as well:

.createTable('Team', table => {
            table.increments('id').primary().notNullable();
            table.timestamps(true, true);
            table.string('name').notNullable();
})
.createTable('User', table => {
            table.increments('id').primary().notNullable();
            table.timestamps(true, true);
            table.string('email').notNullable();
            table.string('password').notNullable();
            table.string('name').notNullable();
            table.string('avatar_url').notNullable();
            table.string('position').notNullable();
            table.integer('team').unsigned();
            table.foreign('team').references('Team.id');
        })

All I am trying to do is insert a new instance of User with it's team column set to an id of a Team that I just finished creating. For some reason I can only do this using knex, but when I try to insert using my Model subclasses the insert fails.

Here are the various knex inserts that work:

const new_user = {
        name: 'John Smith',
        password: '1234',
        email: 'johnsmith@gmail.com',
        team: team.id
};
    knex('User')
    .returning('id')
    .insert(new_user)
    .then(new_user => {
        logger.silly(`created new user: ${util.inspect(new_user, false, null)}`);
    })
    .catch(handleSeedError);

    knex('User')
    .returning(['id', 'name', 'password'])
    .insert(new_user)
    .then(new_user => {
        logger.silly(`created new user: ${util.inspect(new_user[0], false, null)}`);
    })
    .catch(handleSeedError);

    knex('User')
    .returning('*')
    .insert(new_user)
    .then(new_user => {
        logger.silly(`created new user: ${util.inspect(new_user[0], false, null)}`);
    })
    .catch(handleSeedError);

Here is the Model subclass insert that always fails:

User
.query()
.insert(new_user)
.then(new_user => {
     logger.silly(`created new user: ${new_user.name}`);
})
.catch(handleSeedError);

Whenever I run the Model subclass insert, I get the following error:

Error: User.relationMappings.team: modelClass is not a subclass of Model or a file path to a module that exports one.

I have rechecked everything I can think of, including the exporting of my models. Nothing seems to work. Rather than create an issue for this specific error, I chose to keep it more high level and provide so much code because I'm assuming that because I've never used this lib before I could be making some mistakes at the schema or model subclass level.

Really appreciate the help thanks.

mitchellporter commented 7 years ago

Update: I have been staring at this code for too long so not sure what I did, but I now have the User inserts working, but the foreign key column for team is always null. I can view the inserted users in postgres but there is never a value in the team column. Am I not supposed to be inserting new objects with a foreign key column set to an id?

Ideally I'd be able to set the team column on User to not null so that it's required, because in our system a user cannot exist without a team.

One thing that's interesting is if I log the json param in a $beforeValidate for the User model, the json never contains the team field even though I'm setting it. I can set any other random property like teamz or lol_team and when I log the json it's there, but team is never there.

Thanks again.

newhouse commented 7 years ago

I'm on the move now so can't dive all the way in, but the problem with your second issue is probably because your models do not have any JSON schemas defined. I believe the query builders will strip out any properties it doesn't recognize from the object you're inserting. You need to add a JSON schema that defines any properties you want to use the Model as mediator to the DB for. Hope that makes sense. Schema def examples are in the docs I'm pretty sure. Good luck!

mitchellporter commented 7 years ago

@newhouse Appreciate the fast reply! I added the following json schema code to my User model but the team field still isn't present when logging in $beforeValidate and doesn't have a value in Postgres either.

static get jsonSchema () {
    return {
        type: 'object',
        required: ['id', 'email', 'password', 'name', 'avatar_url', 'position', 'team'],
        properties: {
            id: { type: 'integer' },
            email: { type: 'string', minLength: 1, maxLength: 255 },
            password: { type: 'string', minLength: 1, maxLength: 255 },
            name: { type: 'string', minLength: 1, maxLength: 255 },
            avatar_url: { type: 'string', minLength: 1, maxLength: 255 },
            position: { type: 'string', minLength: 1, maxLength: 255 },
            team: { type: 'integer' }
        }
    };
}
mitchellporter commented 7 years ago

Can't test till morning but per https://github.com/Vincit/objection.js/issues/333 I think this is because the relationship name of team is conflicting with the actual team column name in my User table.

koskimas commented 7 years ago

jsonSchema is optional. The properties are stripped only if you have defined a schema. That behaviour can be changed by pickJsonSchemaProperties and starting from 0.8 the default value for that will change to false.

The problem is exactly what you just figured out yourself. The relation name cannot be the same as the foreign key.

mitchellporter commented 7 years ago

@koskimas Yep, it fixed it. I ended up renaming the column name to team_id in the User table. I do have another question though. I wanted the User model to have a team property for the relation mappings, and this works fine, but one thing I noticed is that when incoming json comes in to create a new user, the json field needs to be team_id instead of team.

I had no problem renaming all of my column names to include id eg. user_id, team_id, etc, but I'm used to using API's where if you wanted to create a new user for a team your HTTP body would take a team field, not a team_id field. Same goes for json responses, whether it's just the id of an object, or the full object itself, the field is usually very convenient and just team not team_id.

I'm curious how you and others have dealt with this as I'm sure I can't be the only one trying to achieve this. Should I be using a pre-save hook or something to convert a json field like team into team_id? Or is there a better way that doesn't require me to manually check for and convert certain fields?

I realize all of this could be avoided by going back to db column names of just team and changing my User model's property from team to a different name but that doesn't feel right either because then it might feel weird working with the model in code.

koskimas commented 7 years ago

$parseDatabaseJson and $formatDatabaseJson methods are used to map the object from/to the database format. Similarily $parseJson and $formatJson methods are used to map from/to the "outside world representation". Whenever a model is created explicitly/implicitly for example in insert, patch etc. methods the data gets passed through $parseJson. Whenever the object is serialized to json it gets passed through $formatJson. You could probably do something like this:

class User extends Model {
  $parseJson(json, opt) {
    if (json.team) {
      json.team_id = json.team;
      delete json.team;
    }

    return super.$parseJson(json, opt);
  }

  $formatJson(json) {
    json = super.$formatJson(json);

    if (json.team_id) {
      json.team = json.team_id;
      delete json.team_id;
    }

    return json;
  }
}
mitchellporter commented 7 years ago

@koskimas I read your response yesterday and was excited to get in this morning and get it working. However, I'm still running into problems. When $parseJson is called, the json param never has the team property set on it. If I rename it to anything other than team (which is the same name as the relation mapping property), then it exists in $parseJson and I can do whatever I want with it.

Do you have any idea why when my user submitted json contains a field of team, and the relation mappings for the model has a property of team, that the json param in $parseJson doesn't have the team property anymore? Is this expected behavior?

I have tried this with and without a jsonSchema as well.

mitchellporter commented 7 years ago

@koskimas I have tried hooking into the following methods but can never access a team field on the json: $beforeValidate, $setJson, $setDatabaseJson, $parseDatabaseJson, $formatDatabaseJson, $parseJson, $formatJson.

$parseDatabaseJson is always the first to fire and the json param looks like this the second time that it fires: {team_id: null}

If I edit the relationMappings in my User model and change team to be named anything else, then I can view the various json params and they all have the team field present.

Not really sure what to do at this point other than rolling my own method and making all my models inherit it and then remembering to call it every time before I do inserts or updates. Would definitely feel better hooking into an existing method in Objection. Hopefully I'm doing something wrong here.

mitchellporter commented 7 years ago

The only workaround I've found for this is to first create an instance of my User model class, and then query and insert off of that:

const user = User.fromJson(json);

user
.$query()
.insert(user)
.then(new_user => {
      // do stuff
})
.catch(handleSeedError);

When I do this $parseJson is called, and the team field has not been removed from the json so I can successfully perform my conversion there.

Hopefully there's another way though because this feels fairly redundant. Would be better to use the convenience of the static method and just passing in req.body like the examples show. Thanks for the help.

koskimas commented 7 years ago

@mitchellporter Did you remember to check for team before calling super in $parseJson?

mitchellporter commented 7 years ago

@koskimas Yeah I did. I went back and copy/pasted your code when it first failed just to make sure I didn't mistype something. Here's a quick recap.. If I run the following code, $parseJson is called, but the json param never has a team field so there's nothing for me to change or format anyways:

User
.query()
.insert(user_json)
.returning('*')
.then(user => {
     // do stuff
})
.catch(handleSeedError);

If I run this code instead, then when $parseJson is triggered, there is a team field on the json param and I can do what I need to do:

const user = User.fromJson(user_json);

User
.query()
.insert(user)
.returning('*')
.then(user => {
     // do stuff
})
.catch(handleSeedError);

I can insert the actual user instance, or the user_json, both succeed.

Here is a very simple example project I made to display everything that I just described. You can run the project using npm run seed, or run the seed config using VSCode's node.js debugger:

https://github.com/mitchellporter/objectionjs-example

You can comment/uncomment out the const user = User.fromJson(user_json); line in the seed.js file to reproduce all of this. It will work when uncommented and fail when its commented out and only relying on the insert.

koskimas commented 7 years ago

Thank you for the detailed description. I think this is a bug. I took a glance of the code and in the insert(user_json) case the relations are stripped away before fromJson is called. I can't remember a reason for that. I'll see if I can remove or alter the code so that what you do is possible.

mitchellporter commented 7 years ago

@koskimas Of course no worries and likewise I appreciate the help. I'm really looking forward to using objection on some fun projects and potentially contributing. Thanks.

nexdrew commented 6 years ago

Concerning this:

The relation name cannot be the same as the foreign key.

I couldn't find anything about this in the docs. If it's not mentioned, perhaps we should add it for the benefit of new users like me 😄? If this is a requirement, I would have expected to find it here: http://vincit.github.io/objection.js/#relationmappings

Just wanted to bring this up since I ran into this problem today. I'd be happy to submit a PR for docs.

Also, thanks for writing this awesome library! I love that it's built on top of Knex.js, uses a flexible Promise-based API, and is super easy to set up. Very nice!

newhouse commented 6 years ago

On the above note, is there any way to detect this naming collision automatically? I would think that (at least) in the case that a schema is defined that it would be something that Objection could sniff out: checking to see if any fields/properties in the jsonSchema match any of the relations/properties in the relationMappings. Just a thought...