AugustNagro / magnum

A 'new look' for database access in Scala
Apache License 2.0
153 stars 10 forks source link

DbCodec is not recursively applied #24

Open alwins0n opened 3 months ago

alwins0n commented 3 months ago

First of all, I am not a scala expert so it might be I am missing something obvious here.

I assumed the following would work (embedding values in entities)

@Table(H2DbType, SqlNameMapper.CamelToSnakeCase)
case class Company(
    name: String,
    address: Address,
) derives DbCodec

case class Address(
    street: String,
    city: String,
    zipCode: String,
    country: String
) derives DbCodec

object repo extends Repo[Company, Company, String]

transact(cp):
    repo.insert(company)

but the address does not get "expanded" in the geneerated insert statement

Column "ADDRESS" not found; SQL statement:
INSERT INTO company (name, address) VALUES (?, ?, ?, ?, ?, ?, ?)

again - if I am missing something obvious, I apologize, in that case consider this issue a suggestion for documentation

AugustNagro commented 3 months ago

Hi, nested / embedded entities are not supported. I added a FAQ section to the readme in the linked MR, please review and let me know what you think.

Cheers,

alwins0n commented 3 months ago

The PR looks great and provides an alternative solution and you kind of answered my follow up question: whether this is a design decision or a technical limitation.

It seems like its the former and I want to ask: do you see this feature at some point in the library? would you accept pull requests?

AugustNagro commented 3 months ago

do you see this feature at some point in the library

I am biased against it; although I do agree it is a nice face-level developer experience, there's a lot of complexity introduced that can confuse users. For example, what if users want to make a table that embeds the same type multiple times?

@Table(H2DbType, SqlNameMapper.CamelToSnakeCase)
case class Triangle(
    pointA: Point,
    pointB: Point,
    pointC: Point
) derives DbCodec

case class Point(
    x: Int,
    y: Int
) derives DbCodec

How would we handle the column naming?

Or what about embeddings within embeddings within embeddings (etc)? (hard to reason about)

Or what about when a class is embedded in multiple entity classes, but you refactor the column name for just one of those tables? (hard to maintain).

For example see the hibernate docs on Embeddable: https://docs.jboss.org/hibernate/orm/6.5/userguide/html_single/Hibernate_User_Guide.html#embeddables

I think that, since SQL itself does not support embeddings, we should not try to make entity tables appear like they're object-oriented.

alwins0n commented 3 months ago

hey @AugustNagro , I wanna be a little bit annoying here because I think this feature would play very nicely with the overall design of your library.

Think about this scenario

case class Persisted[T](
  @Id id: Long,
  company: Company // as above
)

object CompanyRepo extends Repo[Company, Persisted[Company], String]

now, the elegant solution here is to not having to redefine your entire entity as a creator, but rather compose the repo requirements. even in a generic way, useable throughout the whole app.

I want to briefly address the other issues you mentioned, just for the sake of completeness

I completely understand you, as maintainer, not wanting to open that can of worms but imho the above scenario makes a strong case for this feature

AugustNagro commented 3 months ago

hey @AugustNagro , I wanna be a little bit annoying here because I think this feature would play very nicely with the overall design of your library.

No worries, and I appreciate the feedback.. I'll do my best to be open minded and continue thinking about this usecase, especially in conjunction with adding UDT, JSON, & XML support.

There's definitely pros to supporting embeddings, which is why Hibernate and others do so. Like you say, Persisted[T] would scrap the boilerplate when defining Entity/EntityBuilder pairs. I do like that example a lot.

The downside is that it is a BIG feature to do right, and it's bringing us a lot closer to ORM-territory.

So, I think the requirements would be:

alwins0n commented 3 months ago

I can help conceptually as I do not consider myself a proficient scala library developer.

Few points of feedback:

I thank you for spending a few thoughts on this. I feel like this feature would be a win for the library without completely landing in orm land (with one-to-manies, lazy loading, etc...).

Furthermore, I want to add that "repo" is, at least to me, a high level concept. an abstraction that allows interaction with the database without knowing too much about the database. currently it feels more like a "dao" which is closer to db (again, imho).

This change, I'd say, would move us closer to "repo" as it can handle scala models as you would design them (more or less) without thinking in "rows".