endersoncosta / herbs2knex

Other
0 stars 2 forks source link

`runner` is the wrong abstraction for `first` and `last` #8

Open dalssoft opened 3 years ago

dalssoft commented 3 years ago

It seems first and last are marked as done on the to do list and this is acomplished by using runner:

const ret = await itemRepo.runner.select().first()
const ret = await itemRepo.runner.select().last()

Here it seems to be "leaking abstraction" since now the user of a repository is exposed to a query builder. It can be useful for more advanced scenarios but first and last should be a high level abstraction.

A better interface (suggestion):

const ret = await itemRepo.findBy({ name: ["marie"] }).first()  // or .limit(1)
endersoncosta commented 3 years ago

maybe if we make the knex select available with all the functions it has, we can just put the "interpreter" in the column name, so the communication is the same, but direct with the knex ...

something like:

`this.select = (params) => this.runner.select( params.map( field =>  this.dataMapper.toTableField( field ))`

This will be simple to use but with the power of the knex... where, limit, etc...

dalssoft commented 3 years ago

For the record, we should use Rails ActiveRecord as an inspiration.

So for this use case, there are a few ways to achieve the same result:

Class.find_by(type: 4)
Class.where(:type => 4).first
Class.find(:first, :conditions => ["type = ?", 4])

Info: https://stackoverflow.com/questions/12135745/what-is-the-fastest-way-to-find-the-first-record-in-rails-activerecord

Code: Where / Find: https://github.com/rails/rails/blob/e7d207f02fe8bf6e351de7da106839b0ab14bbea/activerecord/lib/active_record/relation/query_methods.rb First: https://github.com/rails/rails/blob/e7d207f02fe8bf6e351de7da106839b0ab14bbea/activerecord/lib/active_record/relation/finder_methods.rb

dalssoft commented 3 years ago

Looking at ActiveRecord and Knex complexity and what they have achieved I think your suggestion (if I got it right) should be the way to go. It would be too hard to recreate all the methods. So we should create wrappers to make it a better UX for developers.

On the other hand Knex is query builder, not a repository, so there are a lot of methods there that we don't need.

We could stard with a lower lever abstraction and build the next methods (higher level) on top of this abstraction, similar to ActiveRecord.

Lower lever abstractions:

Knex:

const ret = await knex('users').where({ first_name: 'Test' }).first()

Herbs2Knex

const ret = await userRepo.where({ firstName: 'Test' }).first()

Higher lever abstractions:

Knex:

const ret = await knex('users').where({ first_name: 'Test' }).firts()

Herbs2Knex

const ret = await userRepo.findBy({ firstName: 'Test' })
endersoncosta commented 3 years ago

I managed to do it in a different way than that "function cascade", with this, the options like: first, last or any other, we can pass in an object to the function...

 const ret = await userRepo.where({ firstName: 'Test' }, { first: true })

11

jhomarolo commented 3 years ago

I believe that we should proceed with as little rewriting as possible and using the way that the knex already performs, to facilitate its use later.

Always aim at it as a query builder than an ORM

dalssoft commented 3 years ago

@endersoncosta on your last PR:

const ret = await itemRepo.where({ stringTest: "marie" }, { first: true })

I think a better way would be:

const ret = await itemRepo.where({ stringTest: "marie" }).first()

using Fluent Interface.

endersoncosta commented 3 years ago

I understand, and this was my first idea, but when I tried I was blocked.

Because of the fluent interface of the Knex, to call .first I need check if it is really called, but with fluent interface I can't do it...

Because it's distincts methods, then or I call the function twice times, or I call all the data and return an Array.shift() to first and Array.pop to last...

But this has no performance...

Would you have any way to solve this problem?

dalssoft commented 3 years ago

You could be inspired be Knex itself, where the method only change the state and return a instance of the this.

https://github.com/knex/knex/blob/master/lib/query/builder.js#L1004

If you call it twice, make sure to change the state only once or throw a exception.

endersoncosta commented 3 years ago

I think I understand how it works, but the way I managed to do it was to setup it, and then use run() to perform the final execution...

Something like :

    const ret = await itemRepo.select().where({ stringTest: "marie" }).run()
    const ret = await itemRepo.select().where({ stringTest: "marie" }).first().run()

I'm thinking about how to improve this now ...

dalssoft commented 3 years ago

it's a improvement... to be honest I don't know how to know the last method is the last method and run the query

endersoncosta commented 3 years ago

Without some type of artifice, make a chain of methods is too complex...

We need or use run() to define the end of chain, like this: table.where(options).run() table.where(options).first().run()

Or make an chain and use this on a "runner", like this: const buildedQuery = table.where(options).first() table.runner(query)

We can use options to define how can call the command, like this:

table.where({ stringTest: "marie" }, { first: true })

Or we can create a structure that englobe all the possibilities, like knex himself...

https://github.com/knex/knex/blob/master/lib/knex-builder/make-knex.js