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.
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())
Personally, I think this is outside of scope of objection.js and flirts with feature creep.
@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?
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.
@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.
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.
@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.
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.
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.
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.
@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.
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.
@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.
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.
@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.
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.
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.
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.
@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.
Whoops, didn't mean to delete @StasevichJack's comment 馃槻
By the way, Objection docs also have snake case columns in some of examples.
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.
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.
@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.
@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.
this cons are from people, who don't use snake case columns in their projects so this is already opinionated.
@StasevichJack I have never used a database professionally without snake case. MyOur current project even does snake-case. Don't inject your own beliefs on peoples usage please.
@StasevichJack Because at this point there are 2 people that want this. If I were to add every feature every 2 people want, objection would be a heaping pile of crap by now. I already meantioned that I will add this feature if more people come here and vote this. Just like what happened with #211 and is happening with #448.
@StasevichJack Also objection doesn't favor camelCase. It's not opinionated in any way when it comes to casing. The default is not camelCase it's "no case conversion". You can use snake case without any extra code. It is just not converted to camel case.
@koskimas even though only a few people took part in conversation, what's about all the benefits already provided, isn't it enough to add this small feature?
Automatic conversion is only one of possible variants of usage.
@StasevichJack Do you really think I will change my mind if you keep repeating the same argument over and over? Let's see if other people also want this.
Been watching this one for a while, and it's starting to sound like a "tabs vs. spaces" round-and-round. Regardless of my opinion, (which I will keep to myself for now) I think that above all else, to me it's important to remember that:
People will find this post and chime in if they really want such a thing, but so far it's not tipped the scales in favor of doing so. Maybe more people will chime and it will be clear that it's needed. Maybe Sami will go on a Vision Quest this weekend and come back loving the idea on Monday. I don't know, but one thing is also for sure: back-and-forth with this same small group right now isn't doing much good. I think this issue needs to be left open to marinate, and we'll see what kind of attention it attracts.
I respect and appreciate you all! 馃檱
I've actually used camelcase properties and snakecase columns, and I still don't think this should be in the core. If you need this in all your models, why not just use a base model class that extends objection's Model? This seems like a reasonable practice anyway when you have shared functionality between your models.
class AbstractModel extends Model {
// implement $formatDatabaseJson and $parseDatabaseJson
}
class Person extends AbstractModel {}
class Address extends AbstractModel {}
class Post extends AbstractModel {}
The way it is now, the core is simple but fully extensible, and doesn't favor any particular implementation (or the magic that something like snakeCase = true inherently implies).
It looks like there is traction for being able to provide column name transformations in Knex's configuration directly https://github.com/tgriesser/knex/issues/2084
I don't know if the proposed solution there would necessarily provide a resolution of the issue here, but it seems like it would at least inform any implementations, wherever in code they end up being.
Sorry for being silent here... I've been on vacations and hardly have opened laptop/mails during that time.
I think implementing this in objection level might be a bit nasty to implement if one likes to write also queries using different naming convention so that you can refer in all selects / where etc. statements with camelCase names, but db fields are still snake_case.
So I would prefer 100x more this being implemented on knex side, which would make life of every other knex based ORM easier too. If this is done in knex you could even write
knex.raw('select ??,?? from ??', ['col1Name', 'col2Name', 'MyTable'])
and it can automatically make query to be for example like this (transformation functions should be a bit more intelligent in this case though to know which identifier is table and which one is not):
select "col1_name","col2_name" from "MyTable";
As @skray mentioned there has been discussion about this in knex issues already, I believe there should be also some older issues talking about the same theme.
Other bonus in knex side implementation is that it can be used with knex.schema migration APIs as well.
yeah, I would also prefer this on knex side if they decide to add it.
@VladShcherbin @elhigu is a knex core contributor as well - this is looking promising :)
@elhigu I'll buy you a beer or two if you implement this in knex 馃嵒
Looks like I need to reorder my todo list since there seem to be beer involved :) In knex hopefully this is just a few lines of code there. I'll do my best to get this done for the next release (unless I encounter some weird problems).
At least identifier wrapper overloading is coming in next knex release
https://github.com/tgriesser/knex/pull/2217
And now also result modifying hook is waiting for comments and both will be released in knex 0.14... I still have few PRs and fixes to get through before release. Maybe this can be closed and just add recipe for implementation (without support for separate configuration per model)?
(Sorry for being late to this party).
Having spent more than half a decade in the ActiveRecord trenches, I'd strongly suggest minimizing magick features.
If people have a snake_cased db schema, they've already got two great options: let them use snake_case class members, or let them add a plugin to do this conversion. @koskimas has been a good steward here to keep the core library tight and focused. This "little feature" will have a huge impact on the surface area that needs to be tested for this library.
Agreed. IMHO, this is not something that should be fattening the core of the framework. We've moved away from snake case in pg and the only inconvenience so far is that we need to quote the table/column names that use mixed case when querying in CLI. Other than that, everything works great with Objection.js, since knex already quotes everything anyway.
@mceachen actually I'm not sure if this can be implemented as plugin (requires just custom knex configuration), but yeah I don't think that it is necessary to implement this in core. That's why I suggested closing this issue and just adding recipe.
@demisx I also use camel-case names in DB so for me this feature doesn't matter much. I got used to write quotes in sql pretty quickly too.
Now that knex 0.14 is out the conversion should be done in knex config using the new postProcessResponse and wrapIdentifier hooks. Actually there is no clean way to implement this as a Model property since the knex configuration is global and not per-query or per-model.
@elhigu I shall deliver the beers in the near future as promised. Thank you 馃槃
So, nothing has changed, you still need to manually configure the conversion yourself in every project, it just moved from model to knex part. 馃槥
@koskimas any chance of having Model.knex(knexConnection, { useSnakeCase: true })?
@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()
});
That way they are a separate module and don't mess up objection's internals.
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.
@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.
@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. 馃帀
@VladShcherbin How did you integrate into knex?
@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. 馃檹
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.
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.
@demisx camelCase is painful when writing raw sql queries. Plus, there are people, who actually like and prefer having snake_case columns a database.
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.
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?
Yeh actually, this should be done with postProcessResponse and wrapIdentifier in knex. Easy.
http://knexjs.org/#Installation-post-process-response
@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.
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: https://github.com/Vincit/objection.js/issues/472#issuecomment-342897914
With this mapper everyone will be happy 馃檹
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.
@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.
@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
@koskimas thank you, this is very useful for some users of Objection! <3
@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); //?
}
Thanks @vjpr ! Keep the bugs coming 馃槃
@vjpr What's in the migration that causes the boolean result? I'd like to write a test for it.
I think it was the schema.hasTable before the createMigrationLockTable.
@vjpr Should be fixed now in master
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?
@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.
@koskimas sure, I'll test it before the weekend.
@koskimas hey, I've tested it in one of my projects and didn't find any errors. Everything is working fine for me.
@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
Most helpful comment
Looks like I need to reorder my todo list since there seem to be beer involved :) In
knexhopefully this is just a few lines of code there. I'll do my best to get this done for the next release (unless I encounter some weird problems).