PhilWaldmann / openrecord

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

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

Closed SampsonCrowley closed 7 years ago

SampsonCrowley commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

SampsonCrowley commented 7 years ago

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 commented 7 years ago

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

SampsonCrowley commented 7 years ago

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 commented 7 years ago

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

SampsonCrowley commented 7 years ago

i did change it to use

save(function(result){
console.log(result) //never called
})
PhilWaldmann commented 7 years ago

Please provide more code!

SampsonCrowley commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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