Skip to content
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

Add model snake/camel-case conversion flag #472

Closed
vladshcherbin opened this issue Aug 6, 2017 · 74 comments
Closed

Add model snake/camel-case conversion flag #472

vladshcherbin opened this issue Aug 6, 2017 · 74 comments

Comments

@vladshcherbin
Copy link
Contributor

vladshcherbin commented Aug 6, 2017

Mysql and postgres have snake case convention by default. Since lodash is already a dependency, it'll be very nice to have a model flag and a built-in conversion. Something like this:

class Animal extends Model {
  static get useSnakeCase() {
    return true;
  }
}

The result of turning this flag on will be conversion of output object properties to camel case and input object properties to snake case in model hooks.

I tried using camel case columns in my databases to avoid conversion, but it feels weird after using snake case columns before. There are also some cons of camel case columns - e.g. you have to use quotes in raw sql queries, knex migrations and default database indices/foreign keys use snake case and don't match camel case table and column names.

It'll be very useful to have this feature built in for different purposes.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 6, 2017

This is also useful in knex migrations, there is timestamps method which by default creates created_at, updated_at columns.

table.timestamps(false, true)

// vs current

table.timestamp('createdAt').notNullable().defaultTo(knex.fn.now())
table.timestamp('updatedAt').notNullable().defaultTo(knex.fn.now())

@fl0w
Copy link
Contributor

fl0w commented Aug 6, 2017

Personally, I think this is outside of scope of objection.js and flirts with feature creep.

@vladshcherbin
Copy link
Contributor Author

@fl0w can you explain, why is it outside of scope of an ORM package for databases that have snake case column name convention for decades?

@fl0w
Copy link
Contributor

fl0w commented Aug 6, 2017

objection isn't a traditional ORM, more like a layer on top a query builder. There's very little (if anything) magic involved and very little data manipulation done by the library. I like the focus thus my previous comment.

There's nothing today in objection that stops you from using snake case, and it's easy enough to implement a conversion if you so chose - but IMO it's not something I see as a necessity in core.

If anything, it should be a plugin. This is just my opinion.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 6, 2017

@fl0w a layer on top of query builder is called ORM (it's even written in readme).

So, what you are suggesting - instead of adding a flag that everyone can use right from the start (which doesn't require additional packages, code changes or performance downgrade), you suggest that you have to implement this yourself in every project or download an additional package.

Makes sense.

@fl0w
Copy link
Contributor

fl0w commented Aug 6, 2017

So, what you are suggesting - instead of adding a flag that everyone can use right from the start, you suggest that you have to implement this yourself in every time or download an additional package.

Makes sense.

I provided my opinion (and stated so multiple times), you might disagree and that's alright. The attitude is not. Don't get childish just because someone disagrees with you.

Not everyone does conversion, just like not everyone does soft deletes. And yes, I do believe it makes sense, thus the rebuttal on a feature request. You shouldn't re-implement things anyway - do a plugin and publish it.

@vladshcherbin
Copy link
Contributor Author

@fl0w I respect your opinion, just amazed that people like you are against a feature that has no downsides for you, but you are against just because you don't need it.

Comparing this to soft deletes is like comparing apples to oranges.

@devinivy
Copy link
Collaborator

devinivy commented Aug 6, 2017

I agree that objection shouldn't necessarily handle this. We certainly use our own class inheriting from Objection.Model for our models, in order to provide our own base functionality. If objection were to handle attribute/column transformation I'd expect it to be handled in a more generalized way. A plugin for this would be very cool.

@fl0w
Copy link
Contributor

fl0w commented Aug 6, 2017

just amazed that people like you are against a feature that has no downsides for you, but you are against just because you don't need it.

Because I value lean libraries, and I value the unix philosophy. I also dislike libraries that feature creep. I'm actually equally amazed at users expecting every library to fit their own needs. I favour composition.

@koskimas
Copy link
Collaborator

koskimas commented Aug 6, 2017

Mysql and postgres have snake case convention by default

What do you mean by this? I'm aware that Mysql and postgres are case insensitive and you have to escape identifiers that have mixed case, but I'm not aware of any conventions.

I have never really understood why people want to use snake case in database and camelCase in code. I can almost understand it if you need to write a lot of raw SQL, but with node.js, knex and objection you really shouldn't have to. Can you explain this to me?

I have to agree with @fl0w and @devinivy. There already is a generic way to do column name transformations and that is the $parseDatabaseJson/$formatDatabaseJson hook pair. I realise that copy-pasting the snake_case conversion to every project can be frustrating and I agree that a plugin would be cool. Using a plugin is only marginally more verbose than enabling a flag. I don't see any other downsides especially if the plugin brings in no additional dependencies. Applying a plugin is 3 lines of code (package.json line, import line and the mixin call). Enabling a flag is also 3 lines:

  static get useSnakeCase() {
    return true;
  }

I'm even considering moving some of the existing less used features into plugins.

I can implement the plugin myself. You'll see it released within a week.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 6, 2017

@koskimas

What do you mean by this? I'm aware that Mysql and postgres are case insensitive and you have to escape identifiers that have mixed case, but I'm not aware of any conventions.

In most tutorials, guides and frameworks (not only js) snake case is the default for mysql, postgres databases and this has been for decades. It's a standard most developers use and it feels weird when you use camel case after you were taught to use snake case. You can even google "postgres naming conventions" and all answers I see suggest you to stick with best practices and use snake case. This is what usually novices do and what they use from the start.

When you use camel case column names, you also need to use quotes (select table."columnName" instead of select table.column_name) and when you write complex queries, it's often easier to write sql query first to get what you need and then to convert to ORM models. Having to add quotes in queries is sad.

knex also uses snake case convention, you have migration tables with snake case table and column names. knex methods also generate you snake case columns. You can't even redefine this.

As for the plugin part, in my opinion, plugins usually add features on top of standard ones (for example soft deletes, user password hashing, etc). This feature is fundamental, that's why I think it should be in core. With babel, this is one line:

class Animal extends Model {
  static useSnakeCase = true
}

Compare this elegant flag to having to add a new plugin dependency and adding weird mixins:

import SnakeCaseMixin from 'another-package'

class Animal extends SnakeCaseMixin(Model) {}

It is even more sad when it comes to this:

class Animal extends AnotheMixin(LimitInFirst(SnakeCaseMixin(Model))) {}

It's always easier to judge when you don't need a feature than when you actually need it and can feel all the pain of using it.

@jack-stasevich
Copy link

I'd be also happy to see this in core as all our existing project databases have snake case table and column names.

Having to add a package for this basic functionality is over-engineering.

@koskimas
Copy link
Collaborator

koskimas commented Aug 6, 2017

@vladshcherbin

I don't think it's ever a good reason to follow a convention just because "everyone else seem to do so". I'm also surprised that the convention is rooted so deeply that people feel "weird" when not following it.

knex also uses snake case convention, you have migration tables with snake case table and
column names. knex methods also generate you snake case columns. You can't even redefine this.

How? The timestamps method? Why do you even use it? It saves you one single line of code.

Based on my own experience people that follow this convention are a minority. I have personally never been a part of, or even seen, a project that follows that convention in real life. I've only seen a couple of issues about this in objection's issues.

I may be horribly wrong about this though. If you could somehow prove me that the majority follow that convention, I would add the feature to the core. But based on my experience, that's really not the case. We cannot add every feature everyone needs to the core.

The other sad thing you mentioned has nothing to do with this one. I just opened #473 to address it.

@koskimas
Copy link
Collaborator

koskimas commented Aug 6, 2017

I need to add that off course I have seen people use snake cased identifiers, but in those cases they also used them in code.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 6, 2017

@koskimas this conventions are very useful when you have a different language background so that you don't waste time on arguing on how to name things in database.

I can tell you a story how knex was created and by what inspired:

Laravel is one of the most popular php frameworks and the whole docs and ecosystem is using snake case. For people who want camel case there is a model flag, which you can't even find in the docs.

knex is also using this convention because it was inspired by Laravel query builder which is mentioned in the docs:

Special thanks to Taylor Otwell and his work on the Laravel Query Builder, from which much of the builder's code and syntax was originally derived.

This convention can be seen in migration table name (migrations_lock), column names (migration_time), indices (users_username_unique), foreign keys (user_tokens_user_id_index), etc.

You can also select postgres in knex docs and search for _id, you'll find out that it's used in all examples. You can also check Bookshelf orm, built by creator of knex on top of knex. Snake case is also used there everywhere.

So, basically Objection is using knex under the hood but does not follow the conventions that were used while knex was developed.

There are only a few issues because Objection is not that widely used currently (downloads/month):

If the above facts don't prove that the majority uses this convention, I don't know what else can.

As for myself, I need camel case transformation for easy usage with GraphQL, but I've very happy with snake case in database as this is what I was taught, used in my first database and what I and my colleagues are using everywhere (not only in js).

I understand that when you don't use snake case, you can think that no one does, but in this case it's exactly the opposite.

@koskimas
Copy link
Collaborator

koskimas commented Aug 6, 2017

Don't you think I compared the number of issues to the total number of objection's issues? I'm very aware of how popular objection is compared to other libraries. Please give me some credit. The fact that no one else has suggested this is (for me) an indication that the current way of doing things in objection is not so bad.

You are still only listing documentation conventions, not actual projects. Nothing in knex suggests that you should use snake case names in your own project. They have just selected to use that casing in the documentation. It's very possible that I'm wrong and more people use snake casing than camel casing, but that doesn't mean that they are unable to change their minds when they start using objection.

And they don't even have to change their minds. It's not like it's impossible to use snake casing in objection. There has been a recipe for that from the beginning. I don't know why you consider "adding another dependency" such a bad thing if it only adds the same amount of code the feature in the core would. As I mentioned, I would implement the plugin so that it has zero dependencies and the maintainer would be the same person that maintains objection. What is the downside of the dependency here?

I will add this to core if this issue gets a lot of attention and other people agree with you.

@fl0w
Copy link
Contributor

fl0w commented Aug 6, 2017

To call, having two overrides in a base model which all other application models extends from, "over-engineering and painful", I feel it's over dramatic. The fact that it is so easy to implement as a third party plugin is a testament to how malleable objection is - and that's the goal (according to what I recall from original blog posts by elhigu on moron).

An ORM will never solve all issues and in trying to do so, most ORMs becomes un-maintainable, and hard to progress. I prefer having a core minimal with great recipes, plugins and easy to understand codepaths.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 6, 2017

My point is if ORM uses underlying query builder (knex in this case), conventions of it should be first class citizen, not some plugins or mixins.

@koskimas sorry if I may sound like I don't like Objection, english is not my first language so I may sound harsh. I provided stats only to show that there are a lot of projects that follow the docs and conventions. Here is a simple example - search for knex created_at, compare it to search for knex createdAt. Add number of projects that use timestamps() for this.

Indeed, I tried another js ORMs - Bookshelf, Waterline and Sequelize before Objection and all of them are not even close to the level of satisfaction and support of Objection. I'm using it every day and recommend it to anyone asking for a js ORM. This is actually one of my favorite js packages.

If you don't want it in core, that's okay, I just wanted it to be more user-friendly for people following conventions.

@koskimas
Copy link
Collaborator

koskimas commented Aug 6, 2017

@vladshcherbin My comment was also a bit too pointy. Sorry about that. Actually I modified it, but you read the original. Let's keep this open to see if it collects more opinions.

@koskimas koskimas reopened this Aug 6, 2017
@Vincit Vincit deleted a comment from jack-stasevich Aug 7, 2017
@koskimas
Copy link
Collaborator

koskimas commented Aug 7, 2017

Whoops, didn't mean to delete @StasevichJack's comment 😲

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Aug 8, 2017

By the way, Objection docs also have snake case columns in some of examples.

@koskimas
Copy link
Collaborator

koskimas commented Aug 8, 2017

Yes, they are there to point out that you can use snake case if you want. It's up to the user. I'm not trying to advertise camel case, as I'm not willing to advertise snake case either. My point has been all along that objection attempts to be generic. Adding a built in mechanism for snake case (when there already is a generic mechanism for that) would be opinionated.

@jack-stasevich
Copy link

Having this flag is also useful in plugins that add columns, for example timestamps or soft deletes. If there is a flag, it's possible to check it and add created_at or createdAt accordingly.

There are so many benefits and possibilities with having this flag and zero cons.

@koskimas
Copy link
Collaborator

koskimas commented Aug 8, 2017

@StasevichJack Con(s) mentioned in this issue:

Personally, I think this is outside of scope of objection.js and flirts with feature creep.

My point has been all along that objection attempts to be generic. Adding a built in mechanism for snake case (when there already is a generic mechanism for that) would be opinionated.

@jack-stasevich
Copy link

@koskimas this cons are from people, who don't use snake case columns in their projects so this is already opinionated.

It is really hard to understand, why you don't want to make it easier for another developers since changes required for this are small and there are no real (not opinionated) cons to adding this. The number of provided benefits on the other side is already much more.

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

@vladshcherbin You are really trying to avoid writing any code aren't you 😄 Ok, I'll write mappers you can import through objection like this:

const { knexSnakeCaseMappers } = require('objection');

const knex = Knex({
  client: 'pg',
  connection: {},
  // Merge snake_case to camelCase conversion mappers.
  ...knexSnakeCaseMappers()
});

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

That way they are a separate module and don't mess up objection's internals.

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

And here I was thinking that everyone agreed that this should be moved to knex and that it's enough 😄 Apparently you didn't bother to even read the conversation.

@vladshcherbin
Copy link
Contributor Author

@koskimas cmon, man, I read the conversation and replied back then. I thought, this conversion will be added to knex, not just options to create and support it myself.

The reason is simple - I want reusable code that I can trust. You know the internals far better than me and if anything will change later or break - I'll be sure you'll update this conversion too.

When another developer comes to me and asks, how to do this conversion, what am I to tell him:
a) go read the docs, write conversion by yourself and support it
b) here is an awesome one line you can add to config and everything will just work

I'd be super happy with the 'b' answer.

@vladshcherbin
Copy link
Contributor Author

@koskimas here is the conversion I'm using now:

import memoize from 'fast-memoize'

function convertCamelToSnakeCase(string) {
  return string.replace(/(?=[A-Z])/g, '_').toLowerCase()
}

function convertSnakeToCamelCase(string) {
  return string.replace(/_[a-z]/g, match => match[1].toUpperCase())
}

export const camelToSnakeCase = memoize(convertCamelToSnakeCase)

export const snakeToCamelCase = memoize(convertSnakeToCamelCase)

It's working fine for me now. Does it have any side-effects or edge cases? I don't know, maybe.

I'd be very happy to swap it with yours one and use it instead. 🎉

@vjpr
Copy link

vjpr commented Nov 8, 2017

@vladshcherbin How did you integrate into knex?

@vladshcherbin
Copy link
Contributor Author

@vjpr currently, this conversion is in the model hooks:

// Convert to database snake case format
$formatDatabaseJson(json) {
  const snakeCaseJson = Object.entries(super.$formatDatabaseJson(json))
    .map(([key, value]) => ({ [camelToSnakeCase(key)]: value }))

  return Object.assign({}, ...snakeCaseJson)
}

// Convert from database to camel case format
$parseDatabaseJson(json) {
  const camelCaseJson = Object.entries(json)
    .map(([key, value]) => ({ [snakeToCamelCase(key)]: value }))

  return super.$parseDatabaseJson(Object.assign({}, ...camelCaseJson))
}

I haven't tried using it in knex 0.14 and I hope @koskimas will save us all and add this to objection. 🙏

@demisx
Copy link

demisx commented Nov 8, 2017

I still believe this conversion doesn't belong to Objection.js code. If needed, it can be done at the Knex level. And for new projects, just use camelCase names in DB and forget about this conversion altogether.

@vjpr
Copy link

vjpr commented Nov 8, 2017

just use camelCase names in DB

Nope. This is painful when you want to execute raw queries in the console.

This should be in the Model as it is in Bookshelf.

@vladshcherbin
Copy link
Contributor Author

@demisx camelCase is painful when writing raw sql queries. Plus, there are people, who actually like and prefer having snake_case columns a database.

@demisx
Copy link

demisx commented Nov 8, 2017

Nope. This is painful when you want to execute raw queries in the console.

I don't see what the pain is. Just typing extra quotes? The autocomplete still works. We do this all the time and it's not that bad as it may seem initially. I think it's more painful to realize that after implementing your conversion you still have to mix camelCase and snake_case in your app code. When writing raw queries, if I am not mistaken. I remember correctly, this conversion would not cover all cases.

There are people who actually like and prefer having snake_case columns a database.

Just let them implement it at Knex level then. Why to bring this code into ORM? I realize Bookshelf did it, but doesn't mean the Objection.js has to do it.

Sorry, I don't mean to argue. Just wanted to add my 2 cents and share what worked for our team.

@jordaaash
Copy link
Contributor

The feature was just added to knex, there are several proposed solutions above that don't require adding this to core, and the solution itself is documented here, and in the documentation. I personally love that objection is unopinionated about column and table names, and just makes it easy to convert as desired.

Annoyance at capitalizing/quoting your identifiers on the CLI is not a reason to add this to the library, and using raw queries in objection/knex is straightforward if you just ??/:columnName: bind your identifiers, which you should do anyway, regardless of the convention you use.

This should be in the Model as it is in Bookshelf.

It is in the model, it's just not in this library, and I don't think feature parity with Bookshelf is a design goal. What's the problem with just using $parseDatabaseJson/$formatDatabaseJson in a base model class, as indicated above?

@vjpr
Copy link

vjpr commented Nov 8, 2017

Yeh actually, this should be done with postProcessResponse and wrapIdentifier in knex. Easy.

http://knexjs.org/#Installation-post-process-response

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

@demisx @jordansexton I completely agree with you, but I don't want to see another issue about this 😄 Adding the mappers doesn't actually bloat objection core (other than the number of bytes). They are in a completely separate file. I can live with that.

I've almost implemented the mappers now and actually there are some objection-specific things to consider which makes it almost a good idea to add the mappers to objection. For example, there are many ways to convert a string t o snake_case. lodash snakeCase function does this:

_.snakeCase('*') // --> ''
_.snakeCase('foo:bar') // --> 'foo_bar'

which doesn't work with objection since objection uses : as a separator in some complex join stuff.

@vladshcherbin
Copy link
Contributor Author

Guys, since knex has a way to do this now, the question is not about the model - it's obvious we need to move this code from model to knex level.

The question is about a mapper, so we can use it this way: #472 (comment)

With this mapper everyone will be happy 🙏

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

Also implementing postProcessResponse and wrapIdentifier doesn't seem to be that simple. This is what I got so far:

function camelizeKeys(obj, toCamelCase) {
  const keys = Object.keys(obj);
  const out = {};

  for (let i = 0, l = keys.length; i < l; ++i) {
    const key = keys[i];
    out[toCamelCase(key)] = obj[key];
  }

  return out;
}

function knexSnakeCaseMappers() {
  const toSnakeCase = memoize(snakeCase);
  const toCamelCase = memoize(camelCase);

  return {
    wrapIdentifier(identifier, origWrap) {
      return origWrap(toSnakeCase(identifier));
    },

    postProcessResponse(result) {
      if (Array.isArray(result)) {
        if (result.length === 0 || !result[0] || typeof result[0] !== 'object') {
          return result;
        } else {
          const output = new Array(result.length);

          for (let i = 0, l = result.length; i < l; ++i) {
            output[i] = camelizeKeys(result[i], toCamelCase);
          }

          return output;
        }
      } else {
        return camelizeKeys(result, toCamelCase);
      }
    }
  };
}

For loops are there for performance reasons.

@jordaaash
Copy link
Contributor

@koskimas Got it. I think people who want different naming conventions among data and code that directly touches that data are asking for trouble, but if you want to help them, that last point about columns on nested relation joins is fair. I've been trying to come up with a reason for why objection would need this if it's doable in knex (aside from convenience), and I think that's a good example.

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

@vladshcherbin and others, the mappers are now in master. They may still be buggy, and the snake_case conversion may not work as you'd expect. It would be really cool if you could test it out before I release it.

Here's how to use them

https://github.com/Vincit/objection.js/blob/master/doc/includes/RECIPES.md#snake_case-to-camelcase-conversion

@vladshcherbin
Copy link
Contributor Author

@koskimas thank you, this is very useful for some users of Objection! <3

@vjpr
Copy link

vjpr commented Nov 8, 2017

@koskimas Bug.

Migrations broke because result can be a boolean...

    postProcessResponse(result) {
      if (Array.isArray(result)) {
        if (result.length === 0 || !result[0] || typeof result[0] !== 'object') {
          return result; //?
        } else {
          const output = new Array(result.length);

          for (let i = 0, l = result.length; i < l; ++i) {
            output[i] = camelizeKeys(result[i], toCamelCase);
          }

          return output;
        }
      } else if (typeof result === "boolean") { <------------------ needed
        return result
      } else {
        return camelizeKeys(result, toCamelCase); //?
      }

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

Thanks @vjpr ! Keep the bugs coming 😄

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

@vjpr What's in the migration that causes the boolean result? I'd like to write a test for it.

@vjpr
Copy link

vjpr commented Nov 8, 2017

I think it was the schema.hasTable before the createMigrationLockTable.

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

@vjpr Should be fixed now in master

@koskimas
Copy link
Collaborator

koskimas commented Nov 8, 2017

For some reason hasTable returns results like this:

[ [ { fieldCount: 0,
      affectedRows: 0,
      insertId: 0,
      serverStatus: 2,
      warningCount: 0,
      message: '',
      protocol41: true,
      changedRows: 0 },
    undefined ],
  false ]

on all databases. Is that normal or is there a bug in knex?

@koskimas
Copy link
Collaborator

koskimas commented Nov 9, 2017

@vladshcherbin Could you also find time to test this? I don't want to see a bunch of bug reports as soon as I release this.

@vladshcherbin
Copy link
Contributor Author

@koskimas sure, I'll test it before the weekend.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Nov 11, 2017

@koskimas hey, I've tested it in one of my projects and didn't find any errors. Everything is working fine for me.

@vladshcherbin
Copy link
Contributor Author

vladshcherbin commented Nov 11, 2017

@koskimas nah, I found an error. I'll write you in chat.

const users = await User.query().where('id', 1).first()
await users.$relatedQuery('tokens')

This code used to update users with fetched relation. When using knexSnakeCaseMappers, it does not.

Solved

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

No branches or pull requests