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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add orchid driver #17

Merged
merged 5 commits into from
May 15, 2024
Merged

feat: add orchid driver #17

merged 5 commits into from
May 15, 2024

Conversation

bingtsingw
Copy link
Contributor

No description provided.

@bingtsingw
Copy link
Contributor Author

bingtsingw commented Apr 29, 2024

I do not implement createTableIfNotExists, is this necessary?

And I can't setup local test, for my new driver and existing drivers, could you please help me run the test?

@Julien-R44
Copy link
Owner

Cool! Thanks a lot for the PR dude. I came across Orchid ORM a while back and thought it looked like Lucid with other some cool features

I do not implement createTableIfNotExists, is this necessary?

Yes, bentocache will automatically create the table if autoCreateTable is true. This allows seamless use of Bentocache without having to manually create a migration if you're lazy: https://bentocache.dev/docs/cache-drivers#databases. You can refer to the Knex or Kysely driver, it should be pretty easy to implement

And I can't setup local test, for my new driver and existing drivers, could you please help me run the test?

What's the problem? There's nothing in particular to do other than launch the docker-compose in the repo which starts a postgres. then launch the tests. But let me know if anything else is giving you trouble

Looks we have some lint issues too

@bingtsingw
Copy link
Contributor Author

@Julien-R44 lint error and peerDependencies has been fixed.

As for createTableIfNotExists, I do not find createTable related method from the #connection, Orchid ORM has a separate package rake-db for migrations and table creations, but it's very heavy to import it into runtime, so I don't want to introduce it only for createTable.

@romeerez Hi, does orchid-orm package provide some lightweight way to createTable

@romeerez
Copy link

@bingtsingw it's very nice to see this PR!

I'm seeing this Bentocache library for the first time and I'm not sure yet what exactly is required from a db driver.

In some cases, it can be enough to just write a bit of custom SQL to create a table. But looks like in this case, it should happen dynamically.

Imagine that OrchidORM has an exported separate function createTable. But how about changing a table? Wouldn't you need much more functionality to add/drop/change/rename columns? No way there is no need to ever change a table.

So I'll look into Bentocache to see what should be changed in Orchid to support it.

@Julien-R44
Copy link
Owner

Julien-R44 commented Apr 29, 2024

We could do a dynamic import of rake-db, but if it is that heavy, then yeah we can probably do without it: it's not dramatic. You can throw an error if the createTableIfNotExists function is called with Orchid driver, saying "This Feature is not supported with OrchidORM. You must manually create your cache table" something like that and that should be okay for now

Imagine that OrchidORM has an exported separate function createTable. But how about changing a table? Wouldn't you need much more functionality to add/drop/change/rename columns? No way there is no need to ever change a table.

In fact we don't really have that need in Bentocache. Bentocache just expects a "cache" table with key,value, and expires_at. If you need to change the table, then yes, traditional migrations are recommended. See the knex driver as reference: https://github.com/Julien-R44/bentocache/blob/main/packages/bentocache/src/drivers/database/adapters/knex.ts#L64

@bingtsingw
Copy link
Contributor Author

@romeerez Thank you for your support, in Bentocache, it has an autoCreateTable option which needs to implement createTableIfNotExists, this is the Kysely version of implementation:

  async createTableIfNotExists(): Promise<void> {
    await this.#connection.schema
      .createTable(this.#tableName)
      .addColumn('key', 'varchar(255)', (col) => col.primaryKey().notNull())
      .addColumn('value', 'text')
      .addColumn('expires_at', 'bigint')
      .ifNotExists()
      .execute()
  }

There is no need to change it in this situation.

@bingtsingw
Copy link
Contributor Author

@Julien-R44 Thank you for your explain, I thought it was a must.
@romeerez Seems there is no need to add extra methods, I'll test if it works

@romeerez
Copy link

@bingtsingw I have looked around, and, well, any ORM is an overkill for this case, even Knex and Kysely are overkill, since Bentocache requires only limited functionality from a driver, it should be easy enough and better for performance to do all queries (to create a table, get value, set value) with raw SQL by calling pg or postgres libs directly.

ORM has a ton of additional functionality that won't be used: relations management, various features like callbacks on create/update/delete, deletedAt, and many more. Query builder also has a ton of functionality that is redundant for this: various ways of doing joins, and wrappers for various SQL statements.

@bingtsingw
Copy link
Contributor Author

@romeerez Yes, in fact ORM is overkill for this tiny cache table, that's why I do not use Knex or Kysely and want to implement an orchid driver, cause I already use it in my project, making it a driver of bentocache is natural.

@romeerez
Copy link

Aha, yes that makes sense.

In such case, IMO it would be better for Bentocache to have a pg lib adapter, then it could work with any ORM that depends on pg driver.

You could extract pg.Pool from the db instance and the Bentocache pg driver would use it for performing queries directly:

const db = orchidORM({ ...options }, { ...tables })

// extract pg.Pool from db instance:
const pgPool = db.$adapter.pool

const value = await pgPool.query('SELECT value FROM cache WHERE key = ?', ['key'])

@Julien-R44
Copy link
Owner

Julien-R44 commented Apr 29, 2024

@bingtsingw I have looked around, and, well, any ORM is an overkill for this case, even Knex and Kysely are overkill, since Bentocache requires only limited functionality from a driver, it should be easy enough and better for performance to do all queries (to create a table, get value, set value) with raw SQL by calling pg or postgres libs directly.

ORM has a ton of additional functionality that won't be used: relations management, various features like callbacks on create/update/delete, deletedAt, and many more. Query builder also has a ton of functionality that is redundant for this: various ways of doing joins, and wrappers for various SQL statements.

Yeah, this is actually something I've hesitated about on bentocache. Do I create adapter for pg, sqlite, @libsql/client etc rather than adapters for ORMs. In the end, I preferred to use ORMs because there are almost more dialects than ORMs. So it seemed simpler to me to maintain the adapters of the most popular ORMs.

For example, just for Postgres, I would have had to make adapters for @neondatabase/serverless, @xata.io/client, pg, postgres, node-postgres etc... Where I can just add a Drizzle Adapter that will do all this work for me

I hope I didn't make a mistake 馃槄

@Julien-R44
Copy link
Owner

Julien-R44 commented Apr 29, 2024

btw @romeerez, I've just had a closer look, sorry about the comparison with Lucid ORM, Orchid ORM has nothing to do with it. I got it confused with https://sutando.org/guide/getting-started.html which indeed look similar to Lucid

Orchid ORM looks really cool, well done for the work !!

@romeerez
Copy link

Thank you!

It's indeed similar to Lucid, since Lucid is on Knex, and Orchid is syntactically inspired by Knex to be familiar for node.js devs.

Lucid's creator posted some interesting notes about reaching a good type-safety level with ORMs, and this is the exact main goal of Orchid ORM to reach.

@bingtsingw
Copy link
Contributor Author

@Julien-R44 Hi, I setup the test, seems it fails with expires_at column type.
image

@Julien-R44
Copy link
Owner

Weird. I'm not sure why. The tests run fine in CI though: https://github.com/Julien-R44/bentocache/actions/runs/8880648142/job/24383374119?pr=17#step:7:128
So i would say, probably a configuration issue on your side

Maybe you can ignore the local execution of the knex tests. Just run the tests you added on Orchid. Or even let the CI execute them

@bingtsingw
Copy link
Contributor Author

@Julien-R44 Hi, I've completed this pr including autoCreateTable.

@bingtsingw bingtsingw changed the title [WIP] feat: add orchid driver feat: add orchid driver May 15, 2024
@Julien-R44
Copy link
Owner

Awesome. thanks a lot for taking the time to create this PR. I will give a final look at it and merge it before the end of the week

@bingtsingw
Copy link
Contributor Author

Some tests did not pass, I took a look at the error messages, not sure if it's related to orchid driver

@Julien-R44
Copy link
Owner

We have some flakky test in the suite. It was one of them 馃槃 All green now

@Julien-R44
Copy link
Owner

Okay that looks perfect to me. Let's merge it. Thanks again for the PR !

@Julien-R44 Julien-R44 merged commit 695cd71 into Julien-R44:main May 15, 2024
4 checks passed
@Julien-R44
Copy link
Owner

Released with bentocache@1.0.0-beta.9

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

Successfully merging this pull request may close these issues.

None yet

3 participants