Skip to content
This repository has been archived by the owner on Aug 5, 2021. It is now read-only.

Model.create() should return the created instance #43

Closed
SampsonCrowley opened this issue Jul 11, 2017 · 15 comments
Closed

Model.create() should return the created instance #43

SampsonCrowley opened this issue Jul 11, 2017 · 15 comments

Comments

@SampsonCrowley
Copy link
Contributor

SampsonCrowley commented Jul 11, 2017

given the following code:

Account.create({email: 'email@email.com', password: 'password'}))
.then(function(account){
  console.log(account) //true, should be {id: NEW_ID, email: ..., etc})
})

the returned account is true instead of the object that was created

this will create race conditions if I need that newly created record as relying on 'last insert' is not thread safe. postgres has the option RETURNING in Insert calls. that should be used to return the id of the object at least

@PhilWaldmann
Copy link
Owner

What you get as a boolean value is the success flag. It will be false if e.g. some validation failed.
If you need the id of the newly created record, just use this.id:

Account.create({email: 'email@email.com', password: 'password'})
.then(function(success){
  console.log(this.id) // `this` is the record
})

is the same as

Account.create({email: 'email@email.com', password: 'password'}, function(success){
  console.log(this.id) // `this` is the record
})

or

var record = Account.new({email: 'email@email.com', password: 'password'})
record.save(function(success){
  console.log(this.id == record.id) // this === record
})

@PhilWaldmann
Copy link
Owner

But this is something I would like to change in the future. So that validation errors could be catched via .catch(ValidationError, error => {...})

@SampsonCrowley
Copy link
Contributor Author

the problem with using this for getting the record instead of proper reject resolves is:

1: using await; the only response you can get is true unless you pass a callback function

2: bound functions; I like to have all my promises in a nice one layer chain instead of nested. often this has been bound to the parent function so this will not return the model, it will return the parent class instance

But this is something I would like to change in the future. So that validation errors could be catched via .catch(ValidationError, error => {...})

I think that is definitely something that should be in the roadmap as other than this particular functionality, most of this library works properly for promise chaining instead of function => callback;

overall this is a pretty awesome module, but the non standard create functionality is a temporary dealbreaker

@PhilWaldmann
Copy link
Owner

If you want to change the create/save behavior now, you could use the simple plugin I've just added to the test. see cced36b promise-plugin.js

@SampsonCrowley
Copy link
Contributor Author

lovin the quick responses here. I'm having an issue where Inserts are not running in production.

the database connection is there, because it's correctly building the attributes for each model. but when i run save() it just hangs indefinitely

@SampsonCrowley
Copy link
Contributor Author

it's not a permissions issue, the same connection string works for pg-native pool.

@SampsonCrowley
Copy link
Contributor Author

i did change it to use

save(function(result){
console.log(result) //never called
})

@PhilWaldmann
Copy link
Owner

Please provide more code!

@SampsonCrowley
Copy link
Contributor Author

SampsonCrowley commented Jul 12, 2017

Store Setup:

const params = url.parse(databaseUrl);
const auth = params.auth.split(':');
const config = {
  user: auth[0],
  password: auth[1] || null,
  host: params.hostname,
  port: params.port,
  database: params.pathname.split('/')[1],
  max: 20,
  ssl: false,
  idleTimeoutMillis: 10000
};
const pool = new pg.Pool(config);

const store = new OpenRecord(Object.assign({type: 'postgres', models: PATH.resolve(__dirname, 'models/*.js')}, config));
var storeReady = false;

Models:

//Account.js
module.exports = function (){
  this.filteredChanges = function filteredChanges(changes){
    return Object.keys(changes)
    .filter(key => key.toLowerCase() !== 'password')
    .reduce((obj, key) => {
      obj[key] = changes[key];
      return obj;
    }, {})
  }

  this.forCookie = function forCookie(){
    return {id: this.id, email: this.email}
  }

  this
  .validatesUniquenessOf(['email', 'athlete_id'])
  .validatesFormatOf('email', /^[^@\s\;]+@[^@\s\;]+\.[^@\s\;]+$/)
  .belongsTo('athlete')
  .hasMany('accounts_users')
  .hasMany('users', {through: 'accounts_users'})
  .hasMany('infokits')
  .hasMany('account_audits')
  .beforeSave(function(){
    let action;
    if(!this.__exists){
      action = 'create';
    } else if(this.hasChanges()){
      action = 'update';
    }

// THIS IS BEING LOGGED. beforeSave is successfully being called in both dev and prod

    if(action) console.log(this.account_audits.new({action: action, audited_changes: this.filteredChanges(this.changes), remote_address: this.context.current_ip || '127.0.0.1'}))
    return true
  })
}

//AccountAudit.js
module.exports = function (){
}

//Infokit.js
module.exports = function (){
  this.belongsTo('account')
}

Model Retrieval Function:

orm(model, request){
    const context = { headers: { 'x-forwarded-for': '127.0.0.1' } };
    if(storeReady){
      return Promise.resolve(store.Model(model).setContext(context));
    }
    return new Promise((resolve, reject) => {
      store.ready(function(){
        storeReady = true;
        resolve(store.Model(model).setContext(context))
      })
    })
  }

Calling function:

createWithInfokit(request, response) {
    let newAccount, infokit;
    const {account, address, athlete, contact, guardian} = request.body;
    request.current_ip = request.headers['x-forwarded-for'] || request.connection.remoteAddress || request.ip;

    if(!account || !address || !athlete || !contact || !guardian){
      return super.errorHandler("Missing Required Fields", "Missing Required Fields", response);
    }

    return this.credentials(account, true)
    .then(() => this.conn.orm('Account', request))
    .then(function(Account){
      newAccount = Account.new({email: account.email.toLowerCase(), password: account.password})
      infokit = newAccount.infokits.new({account_id: account.id, address: address, athlete: athlete, contact: contact, guardian: guardian})

      return newAccount.save()
    })
    .then(() => {
// NEVER MAKES IT HERE IN PRODUCTION
      console.log(newAccount)
      axios.get(this.railsUrl + '/node_mailers/accounts/' + newAccount.id + '/infokit')
    })
    .then(function(result){
      request.session.account_id = newAccount.id;
      request.session.save();
      const responseValues = newAccount.forCookie();
      response.cookie('user', JSON.stringify(responseValues));
      return response.json({message: 'ok', result: 'success', account: responseValues});
    })
    .catch(err => super.errorHandler(err, "error creating account", response));
  }

@SampsonCrowley
Copy link
Contributor Author

SampsonCrowley commented Jul 12, 2017

if i change Calling function:

createWithInfokit(request, response) {
    let newAccount, infokit;
    const {account, address, athlete, contact, guardian} = request.body;
    request.current_ip = request.headers['x-forwarded-for'] || request.connection.remoteAddress || request.ip;

    if(!account || !address || !athlete || !contact || !guardian){
      return super.errorHandler("Missing Required Fields", "Missing Required Fields", response);
    }

    return this.credentials(account, true)
    .then(() => this.conn.orm('Account', request))
    .then(function(Account){
      newAccount = Account.new({email: account.email.toLowerCase(), password: account.password})
      infokit = newAccount.infokits.new({account_id: account.id, address: address, athlete: athlete, contact: contact, guardian: guardian})
      return new Promise(function(resolve, reject){
        return newAccount.save(function(result){
          console.log(result) //Never Called

          if(result) return resolve(this)
          return reject(new Error(this.errors)
        })
      })
    })
    .then(() => {
// NEVER MAKES IT HERE IN PRODUCTION
      console.log(newAccount)
      axios.get(this.railsUrl + '/node_mailers/accounts/' + newAccount.id + '/infokit')
    })
    .then(function(result){
      request.session.account_id = newAccount.id;
      request.session.save();
      const responseValues = newAccount.forCookie();
      response.cookie('user', JSON.stringify(responseValues));
      return response.json({message: 'ok', result: 'success', account: responseValues});
    })
    .catch(err => super.errorHandler(err, "error creating account", response));
  }

it doesn't make a difference. save still hangs forever

@PhilWaldmann
Copy link
Owner

so the problem occurs only on prod? What's the difference between your production and development system?

Just a note: newAccount.infokits.new({account_id: account.id, ... -> account_id will be overwritten after the INSERT of the account record (with the new id!)

@SampsonCrowley
Copy link
Contributor Author

yeah I noticed that was there and took it out, thanks.

the difference between dev and production is production is on a server cluster with a remote database. local dev is using a localhost pg database. I know the connection string is valid and the remote DB isn't blocking the user (raw pg.Pool connection works fine for both retrieval and insert)

the difference with the pg.Pool vs OpenRecord is that I'm using pg-native with the raw sql
that shouldn't really matter though since pg and pg native use the same API

@SampsonCrowley
Copy link
Contributor Author

The only real problem I can think might be possible is that the dual pool is reserving too many connections. I'm gonna try diving full on into using OpenRecord and see if that solves it

@SampsonCrowley
Copy link
Contributor Author

SampsonCrowley commented Jul 13, 2017

After removing the native pool code from my app and using this library as the sole DB connection, it still times out on most queries. how is the database pool handled?

@SampsonCrowley
Copy link
Contributor Author

Problem was with postgres database. there were connections that never closed taking up the available connection spaces. Closing

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

No branches or pull requests

2 participants