Using pgp.helpers instead of sql

[ferlopezm94]

Hi!

I'm trying to enhance my app structure following this demo.

In repos/producs.js you return an object with one property being:
add: values => rep.one(sql.add, values, user => user.id)

I'm trying to use pgp.helpers instead, like:

const {pgp} = require('./../../../database');
const csUser = new pgp.helpers.ColumnSet([
      {name: 'email'}, 
      {name: 'first_name', prop: 'firstName', def: null}, 
      {name: 'last_name', prop: 'lastName', def: null}], {table: 'user'});

module.exports = (rep, pgp) => {
   return {
      add: values => rep.one(pgp.helpers.insert(values, csUser) + 'RETURNING id', [], user => user.id)
   }
}

I got an error because I was trying to use pgp before it was created so it was undefined. The solution I found was to simply create csUser inside module.exports:

module.exports = (rep, pgp) => {
   const csUser = new pgp.helpers.ColumnSet([...], {table: 'user'});

   return {
      add: values => rep.one(pgp.helpers.insert(values, csUser) + 'RETURNING id', [], user => user.id)
   }
}

And with this I'd like to ask you:

1. Is it valid to use pgp.helpers instead of sql approach like in this demo? I'm using pgp.helpers because of the flexibility that provides with ColumnSet and because I'm planning to do massive inserts.
2. Is it ok to define my columns set inside module.exports? I have doubt of doing this because you mention when we extend the database protocol that "Do not use 'require()' here, because this event occurs for every task and transaction being executed, which should be as fast as possible." so what I understand with this is what we place inside extend function is executed every time we make a task or tx, so creating my columns set this way is bad for performance.

Thanks!

[vitaly-t]

I got an error because I was trying to use pgp before it was created

Are you saying that your declaration of pgp is invalid? -

const {pgp} = require('./../../../database');

Perhaps then you should just fix that ;) Otherwise, you are not explaining the problem well there.

Is it valid to use pgp.helpers instead of sql approach like in this demo

If it wasn't valid, it wouldn't have worked, right? 😉

Is it ok to define my columns set inside module.exports?

If you do so, you needlessly re-create it, thus killing its performance optimization that's based on the object's internal caching.

I have doubt of doing this because you mention when we extend the database protocol that "Do not use 'require()' here...

Not sure if you are referring to the right context, it was a piece of demo documentation for event extend. When you do extend, do not use require inside, that'd be a bad idea, performance-wise.

so what I understand with this is what we place inside extend function is executed every time we make a task or tx

Yes.

so creating my columns set this way is bad for performance.

Yes.

---

In the demo we initialize each repository with the pgp instance for that.

But if at any point you want to create a ColumnSet, and you have your db object there, then pgp can be accessed via db.$config.pgp 😉 See property $config ;)

[ferlopezm94]

Thanks for your quick response.

The problem is that, as in the demo, in db/index.js I have

const repos = {
  users: require('./repos/users')
};

const options = {
  // Extending the database protocol with our custom repositories:
    extend: obj => {
      obj.users = repos.users(obj, pgp);
    }
};

const pgp = require('pg-promise')(options);

const db = pgp(process.env.DATABASE_URL);

module.exports = {db, pgp};

So when trying to access pgp inside repos, it is not already created.

const {pgp} = require('./../../../database');
const csUser = new pgp.helpers.ColumnSet([...], {table: 'user'});

module.exports = (rep, pgp) => {
   return {
      add: values => rep.one(pgp.helpers.insert(values, csUser) + 'RETURNING id', [], user => user.id)
   }
}

In the demo works fine because we don't have to access pgp outside module.exports.

Searching in the documentation I found that "This property(event) can be set dynamically (before or after initialization).", so I think I can first let pgp = require('pg-promise')(); and then, add the options to it and that will solve the problem :D but I can't find the correct way to set it after pgp initialization.

Sorry, I'm still new using pg-promise but the project I'm working on is having too much hard-code sql statements and I want to follow this approach for the sake of maintainability.

[vitaly-t]

So when trying to access pgp inside repos, it is not already created.

You are wrong there. It is already created. Understand, that event extend happens much later than your pgp declaration below. That's your standard forward declaration in javascript.

And inside the repo file, simply declare your ColumnSet outside of the module.exports, and then do conditional instantiation - i.e. create each ColumnSet if it hasn't been created yet - problem solved.

[ferlopezm94]

Thanks @vitaly-t for the corrections.

And inside the repo file, simply declare your ColumnSet outside of the module.exports, and then do conditional instantiation - i.e. create each ColumnSet if it hasn't been created yet - problem solved.

So basically I have to do the following?

let csUser;

module.exports = (db, pgp) => {
   csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');

   return {
      add: values => rep.one(pgp.helpers.insert(values, csUser) + 'RETURNING id', [], user => user.id)
   }
}

[vitaly-t]

So basically I have to do the following?

Yes, precisely!

[vitaly-t]

b.t.w. instead of {name: 'email'} you can simply use 'email', i.e. a simple text string.

[ferlopezm94]

Thank you so much @vitaly-t!

Being able to extend the protocol is an amazing feature and now the code of my application is cleaner. But if we had like 50+ repos, and with the fact the every thing inside extend is executed in every task/tx, will that slow down the performance of the app?

[vitaly-t]

It shouldn't, but tell me if I'm wrong! :))))))))

[ferlopezm94]

Actually, after including like 5 repos (not su much) to the app, and running the tests, it takes the same amount of time as before :D

Thanks for your support!

[vitaly-t]

I know that including 20 repos showed absolutely no slow down, but I didn't test beyond that.

[ferlopezm94]

Perfect, if a include more than 20 I'll let you know.

[vitaly-t]

@ferlopez94 how is it gone so far? 😉

[ferlopezm94]

Everything has gone so well!

I'm using 8 repos right now and doing massive inserts with sequence and pgp.helpers, such a fantastic library!

Recently I found the pattern of converting inside my repo functions the result obtained from the queries like:

addOne: values => rep.one(pgp.helpers.insert(values, cs) + 'RETURNING *', [], diagnostic => {
         const dateTime = diagnostic.date_time.toISOString();
         const date = dateTime.substring(0, dateTime.indexOf('T'));
         const time = dateTime.substring(dateTime.indexOf('T') + 1, dateTime.indexOf('.'));

         return {
            id: +diagnostic.id,
            dateTime: `${date} ${time}`,
            patientID: +diagnostic.patient_id
         };
      }),
findOne: id => rep.one(`SELECT * FROM ${table} WHERE id=$1`, id, result => {
         const dateTime = result.date_time.toISOString();
         const date = dateTime.substring(0, dateTime.indexOf('T'));
         const time = dateTime.substring(dateTime.indexOf('T') + 1, dateTime.indexOf('.'));

         return {
            id: +result.id,
            dateTime: `${date} ${time}`,
            patientID: +result.patient_id
         };
      }),

Is there a way that pgp.helpers.ColumnSet can help in this situation? I use ColumnSet to convert an object properties to the representation in my database (someColumn to some_column), but I was wondering if it can do the oposite (some_column to someColumn).

Other workaround I thought was to extract the conversion to its own function inside the repo but I don't know if doing this can affect performance.

What can you recommend me?

[vitaly-t]

Is there a way that pgp.helpers.ColumnSet can help in this situation?

Are you talking about converting the results returned from a query? That would be completely separate from the ColumnSet type.

but I was wondering if it can do the opposite (some_column to someColumn).

Are you talking about this? - https://coderwall.com/p/irklcq/pg-promise-and-case-sensitivity-in-column-names

[ferlopezm94]

Are you talking about converting the results returned from a query? That would be completely separate from the ColumnSet type.

Yes. Ok, got it.

Are you talking about this? - https://coderwall.com/p/irklcq/pg-promise-and-case-sensitivity-in-column-names

Thanks! That's useful. But, in the case I need to convert, for example an ID (because it's retrieved as a string), do I need to still make the convertion the way I'm doing it right? Or is there a way to make pgp do it by default?

[vitaly-t]

I'm not really sure what you are asking.

[ferlopezm94]

So the problem was repetition of code. I was passing the same value transformation callback into every rep.one. That problem can be solved with the solution you passed me, that way I don't have to manually change column_name to columnName in my result query.
But there is a catch, because in the value transformation callback I also convert my ID string to an ID integer and I make some tweaks to the dateTime returned.

In order to automate the IDs conversion, I found that Solution 3 you give here could work for me:
http://stackoverflow.com/questions/39168501/pg-promise-returns-integers-as-strings

So the only thing to do in my value transformation callback will be the tweaks to dateTime:

const dateTime = result.date_time.toISOString();
const date = dateTime.substring(0, dateTime.indexOf('T'));
const time = dateTime.substring(dateTime.indexOf('T') + 1, dateTime.indexOf('.'));

         return {
            ...result,
            dateTime: `${date} ${time}`
         };
      }),

And let pgp make the camelCase and IDs conversion.

What do you think about this solution?

[vitaly-t]

So the problem was repetition of code. I was passing the same value transformation callback into every rep.one

Why won't you just define a reusable function and pass that function into each query call that you want?

[ferlopezm94]

🤦🏽‍♂️ yes... definitely that's the way to go. In some way I was worried about the fact that I mentioned in the first comments:

I understand that what we place inside extend function is executed every time we make a task or tx

And I didn't realize that actually we are passing a function there so I could define outside the function and pass it. Sorry for trying a more complicated way.

Could you explain me please why what we place inside extend function is executed every time we make a task or tx and not just once when we create or db object? I don't understand why this is necessary.

Thanks.

[vitaly-t]

Could you explain me please why what we place inside extend function is executed every time we make a task or tx and not just once when we create or db object?

Each task/transaction is associated with its own connection context + internal status. Those need to be fully isolated, which requires a new object, while you need the same consistent protocol on each level, plus extensions that are only supported by tasks/transactions, such as method batch.

[ferlopezm94]

Ok, I understand it. Thank you so much @vitaly-t.

[vitaly-t]

@ferlopez94 pg-promise-demo has been refactored for much better ES6 syntax.

For example, repositories in JavaScript are now ES6 classes.

[ferlopezm94]

@vitaly-t thanks! Should I update something to refactor my own code? I'm already using Node v6.10.0.

[vitaly-t]

@ferlopez94 it's up to you :)

[ferlopezm94]

Hi @vitaly-t!

I'm planning to update my repos to use ES6 classes. I've already looked at your example, but in my case that I mentioned earlier, when I do:

let csUser;

module.exports = (db, pgp) => {
   csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');

   return {
      add: values => rep.one(pgp.helpers.insert(values, csUser) + 'RETURNING id', [], user => user.id)
   }
}

Should I include:

csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');

in the constructor of the class?

So basically it should be:

class UserRepository {
   constructor(db, pgp) {
        this.db = db;
        this.pgp = pgp;
        this.csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');
    }

   add(values) {
        return this.one(pgp.helpers.insert(values, this.csUser) + 'RETURNING id', [], user => user.id)
    }
}

Maybe it's obvious but I don't want to make a mistake that impacts in performance.

[vitaly-t]

@ferlopez94 that would be correct, yes.

[ferlopezm94]

Thanks @vitaly-t . Another doubt:

Is it necessary the conditional?

this.csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');

Or can be set just like this:

this.csUser = new pgp.helpers.ColumnSet([...], {table: 'user');

[vitaly-t]

The latter will needlessly re-create the object, that's a no-no. This should be obvious.

[ferlopezm94]

Yeah, what makes me doubt is the fact that it is inside the constructor so the constructor will always be called once (when it is instantiated), which is different using ES5 where the function is exported in module.exports, it was more obvious for me in that case that this function will be called every time (so the need to create conditionally the ColumnSet). Thanks again @vitaly-t :)

[vitaly-t]

which is different using ES5 where the function is exported...

No, it is not different, it has exactly the same functionality behind it, if you look at how classes actually work.

You would still use the same conditional instantiation for the ColumnSet inside module.exports.

[ferlopezm94]

I think I understand now. So, one last question. Is it ok the way I wrote?:

constructor(db, pgp) {
        this.db = db;
        this.pgp = pgp;
        this.csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');
}

Because I think, instead of:

this.csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');

I should reference the variable outside the class csUser, so:

constructor(db, pgp) {
        this.db = db;
        this.pgp = pgp;
        csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');
}

And the same within the functions, instead of reference the class property this.csUser should be the variable csUser that I declare outside the function. Is it ok my reasoning?

[vitaly-t]

this won't make any difference.

[ferlopezm94]

Ok. So, I was reviewing the code I wrote with ES6:

let csUser;

class UserRepository {
   constructor(db, pgp) {
        this.db = db;
        this.pgp = pgp;
        this.csUser = csUser || new pgp.helpers.ColumnSet([...], {table: 'user');
    }
}

Assigning to this.csUser, should't be?:

this.csUser = this.csUser || new pgp.helpers.ColumnSet([...], {table: 'user'); (using this in both sides).

Because what makes me doubt is the fact that referencing the external variable csUser, will always result in the execution of new pgp.helpers.ColumnSet([...], {table: 'user');.

Thanks.

[vitaly-t]

This isn't gonna work, because you never set your global csUser.

This is the most basic JavaScript, not relevant to this library, I cannot do support for it. Please consult JavaScript for information about static variables and how to use them.

I'm locking this issue.

[vitaly-t]

pg-promise-demo has been updated to include examples of setting up static ColumnSet objects within repositories both for JavaScript and TypeScript.