balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.85k stars 1.95k forks source link

discussion: replace waterline-sequel with a community-supported query builder #5959

Closed tjwebb closed 5 years ago

tjwebb commented 10 years ago

I'm wondering if the project would benefit from using a more popular and more thoroughly-maintained query builder such as knexjs. Interested in hearing feedback.

particlebanana commented 10 years ago

I looked into it a bit when discussing the idea of waterline-sequel. It came down to mapping the waterline query language into knex or porting over the code we already had in both postgresql/mysql to work. Moving the code over seemed easier and quicker at the time. As the past few days have gone I'd much rather have a battle tested query builder to rely on so i'm all game for this if it can work.

tjwebb commented 10 years ago

Maybe @tgriesser would be interested in helping :)

eddieajau commented 10 years ago

I personally prefer the Mongo syntax for query building as it's relatively ubiquitous and easily translated to SQL where necessary (going the other way is a bit harder).

However, what might be really good is if the core devs could maybe schedule a few Google hangouts or similar to go over the inner workings of the code to a) help us understand it, and b) allow that to lead to we who are starting to rely on Waterline for projects a chance to know how and where to improve it.

tjwebb commented 10 years ago

I agree with supporting Mongo syntax. This could be done with a very thin syntax mapping layer on top of knex.

arcseldon commented 10 years ago

+1 including support for Mongo syntax if this goes ahead.

eddieajau commented 10 years ago

Bump. Any thoughts on how to possibly get this moving alone?

Note to self - https://github.com/goodybag/mongo-sql

tgriesser commented 10 years ago

I'd be willing to help out on this however I could. Any ORM is going to have varying degrees of opinion on what they should or shouldn't do / how they're done, but it'd be cool if we could share a common base layer just to have more eyes on it to continue to harden it out (and not reinvent wheels).

Not sure what all would need to be supported with the way of mongo syntax, but I can't imagine it'd be too tricky.

@tjwebb or @particlebanana let me know how you were thinking this might work, I could take a look sometime.

tjwebb commented 9 years ago

@particlebanana What are your thoughts on this? If @tgriesser himself is willing to help out, I think the project would benefit in a major way.

Reinventing the wheel is acceptable in cases where the resulting wheel utterly dominates all other wheels. I think waterline-sequel is okay, but I think knex is better, and is a much more popular, robust, and better tested wheel :) Moreover it'd be one less thing that the sails.js team has to maintain, and we'd have more bandwidth to contribute back to knex as well as the constituent sails projects. I'd be interested in @mikermcneil's thoughts as well, since I think part of sails.js history is divestiture from sequelize.

connor4312 commented 9 years ago

Aside from the issue referenced above... Not really liking the idea of using Mongo syntax. SQL functions best when it's used as SQL. Mongo-style usage is great (I love mongo!) but they rarely leads to the most efficient SQL usage. That's not to say it's impossible to have very efficient mongo-style querying, but it doesn't lend itself well. Introducing a layer on top of Knex doesn't seem necessary: aside from the inherent performance cost, it takes additional time to develop and maintain, and Knex already is a very nice and fluent builder by itself. It seems to be solving a problem that doesn't really exist.

The use case where I could maybe see it being useful is in .query() for the end user, but I would assume that having chosen SQL for their application they're at least moderately familiar with its functionality (perhaps moreso than they are with Mongo).

tjwebb commented 9 years ago

I would assume that having chosen SQL for their application

This is wrong. Not all sails adapters are SQL-based. I'd guess that SQL is actually in the minority.

knex only builds SQL queries, not mongo queries. A single syntax for both schemaful and schemaless databases to use. Mongo syntax seems like a reasonable choice. The reverse -- building mongo queries from knex syntax -- doesn't make a lot of sense.

connor4312 commented 9 years ago

I understand that not all (most) waterline adapters are not SQL based, I was referring mainly to those who use only SQL or primarily SQL in their application.

A single syntax would be nice, but relational databases are fundamentally different from nosql systems. You can pursue using a single system, but you're probably going to have to comprise the abilities of SQL and/or NoSQL to do this. Or use the same generalish syntax but with nuances between adapters (which is worse for an end-user, in my view).

tjwebb commented 9 years ago

A single syntax would be nice,

This is what we have now. A single json-based query syntax, with the ability to write custom SQL if you want. I don't see a case for changing that.

connor4312 commented 9 years ago

I'm not saying to change the public API. It works for the most part. I'm talking about for internal usage within the adapter (in reference to the previous issue), and exposing the building in the SQL-specific .query method.

tgriesser commented 9 years ago

So is all you're looking for here the ability to write sql as JSON with knex? I could add something like:

QueryBuilder.prototype.fromJS = function(js) {
  _.each(_.keys(js), function(val, key) {
    if (Array.isArray(val)) {
      this[key].apply(this, val);
    } else {
      this[key].call(this, val);
    }
  }, this);
  return this;
};

And that should allow you do do something like:

knex.fromJS({
  select: ['*'],
  from: 'accounts',
  where: {id: 1},
  orWhere: ['user_id', 'in', [1, 2, 3]],
  leftOuterJoin: ...
})

/cc @tjwebb

tjwebb commented 9 years ago

hey @tgriesser I saw you made the speakers list for jsconf. Let's meet up and chat. sails.js is interested in using knex in a number of ways (namely, to replace waterline-sequel in a number of places), and it'd be great to get buy-in from both communities and pick your brain on how best to go about this. (folks on our side of things are pretty enthusiastic :+1: ).

tgriesser commented 9 years ago

Definitely! Yeah that'd be sweet - I'm in the process of streamlining a ton of the internals https://github.com/tgriesser/knex/pull/786 and beefing up transactions (automatic savepoints when transactions are nested, etc) and brainstorming a better approach for writing and extending adapters (so we can easily get mssql and built-in support some of the great new postgres features)

It'd be great to see where we can collaborate and not all simultaneously reinvent the wheel on a lot of this.

dmarcelino commented 9 years ago

I think a good approach for this would be to replace waterline-sequel for a "waterline-knex" with the same inputs and same (or better) outputs, that way we could use it as a drop in replacement in sails-postgresql and sails-mysql.

mugli commented 9 years ago

@tgriesser Just saw you jsconf video. Awesome talk! I saw you mentioning sails at the end of the talk. Could you and sails guys meet there and make any progress on integrating knex with waterline?

gaurav21r commented 9 years ago

@tjwebb @particlebanana @dmarcelino @tgriesser Any Updates on this?

dmarcelino commented 9 years ago

Kind of... @tjwebb has been working on a version of sails-postgresql using knex. So far it passes all integration tests.

sailsbot commented 9 years ago

Thanks for posting, @tjwebb. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

Thanks so much for your help!

tkrotoff commented 9 years ago

This sailsbot is pretty annoying... This issue should stay open.

devinivy commented 9 years ago

@tjwebb how has been your experience with the postgres module using knex?

tjwebb commented 8 years ago

@devinivy extremely positive.

harish2704 commented 7 years ago

Hi all, I have written a simple module which can transform mongo like queries to knex queries. These are the tests passed at the moment.

I am not sure whether it is relevant in this discussion or not. But simply posting here in the hope that, it will be helpful to someone.

atiertant commented 7 years ago

@harish2704 we wrote a knex based adapter, you could have a look at it here

hunsche commented 7 years ago

Is required have an ORM that manages migrations.

connor4312 commented 7 years ago

@hunsche check out umzug for a nice, framework-agnostic way of handing migrations. Hooking it up to Waterline should take minutes.