romeerez / orchid-orm

Orchid ORM
https://orchid-orm.netlify.app/
MIT License
488 stars 14 forks source link

Redesign relations chaining #351

Open romeerez opened 1 month ago

romeerez commented 1 month ago

Hi all, I need you opinion on redesigning of chaining relations for querying.

The current way of doing this:

db.user.profile // it's a profile query

// load all profiles of users who are on Bahamas
db.user.where({ country: 'Bahamas' }).profile

db.user.select({
  profile: (q) => q.profile, // selecting user profile

  // chaining a nested relation
  baz: (q) => q.foo.bar.baz,
})

// load related record when you already have data
const userProfile = await db.user.profile(userData)

This is a perfection imo, the simplest possible way.

But it needs to be changed because it's missing the fact that relation names can conflict with query method names, as it was brought up by Kenji in https://github.com/romeerez/orchid-orm/issues/337.

db.orderItem.order // is this a relation "order" or the `order` query method?

To solve the problem, I want to drop the chaining completely, and introduce a function instead.

If someone ever would want to contribute, the simplest solution would simplify the internal code, because the chaining is based on JS proxies and is full of tricks and workarounds.

Let's name it rel, maybe you could criticize this name and suggest a better one. But I like it to be short.

// current: db.user.profile
db.user.rel('profile')

db.user.where({ country: 'Bahamas' }).rel('profile')

db.user.select({
  profile: (q) => q.rel('profile'),

  // chaining a nested relation:
  baz: (q) => q.rel('foo').rel('bar').rel('baz'),

  // probably possible, not sure yet:
  baz: (q) => q.rel('foo', 'bar', 'baz'),
})

A different function would serve to load a related record based on already loaded data. How about naming the function for this case queryRel?

// instead of: db.user.profile(userData)

const userProfile = await db.user.queryRel('profile', userData)

// maybe relBy? - for "load relation by this data".
const userProfile = await db.user.relBy('profile', userData)

Hoping for your ideas and suggestions.

Also wondering how do you feel about this quite significant breaking change.

mordechaim commented 1 month ago

Ouch, I really like the current expressive API.

bingtsingw commented 1 month ago

I like the current API, too, maybe we can do some research on how other ORMs handle this conflict.

IlyaSemenov commented 1 month ago

Honestly I didn't know db.user.profile API even existed (after all, if I need a profile I would query db.profile directly), but I'm using db.user.select({ profile: (q) => q.profile }) all the time, and somehow it also never occurred to me that there was an obvious naming clash.

However, if q.rel('profile') not only helps to prevent naming clashes but also somehow simplifies internals or typings, I'm +1 for the change. Either way, I'm personally good with any decision, keep it or change it.

By the way, what if it's implemented as q.relations.profile? This will be in line with relations = { ... } in the table class, it reads simpler, and it'll be slightly simpler to update the code base. The current relation query accessors are already properties and not methods, so it's not getting slower either (I guess it's using dynamic getters?)

romeerez commented 1 month ago

So far I'm thinking to keep the existing syntax in select, and make a change in other places that you might even didn't know existed. select is most used, so let's keep it shortest. something: (q) => q.order doesn't make sense in select.

There are also other similar callbacks: in where, join, and in couple of other places. It should be fine to keep it consistent and keep it without changes in the callbacks.

But db.orderItem.order would definitely be a query method for ORDER BY, not the order relation of "order item", so for this case I'm going to add rel: db.orderItem.rel('order').

Honestly I didn't know db.user.profile API even existed

Basically, it's "where exists" the other way around: db.user.profile === db.profile.whereExists('user') (with a a possibility to add where in both cases to user or to profile).

By the way, what if it's implemented as q.relations.profile?

Maybe, but I'd like it to be more compact.

mordechaim commented 1 month ago

That's a reasonable compromise. I also never knew you could dot into relations of relations.