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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DbCodec is not recursively applied #24

Open
alwins0n opened this issue May 21, 2024 · 6 comments · Fixed by #26
Open

DbCodec is not recursively applied #24

alwins0n opened this issue May 21, 2024 · 6 comments · Fixed by #26

Comments

@alwins0n
Copy link

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 AugustNagro mentioned this issue May 21, 2024
@AugustNagro
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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

  • embedding within embeddings - why not? domain modeling with case classes does this all the time no?
  • multiple embeddings in one class - well, the lib does not shy away from annotations, I think it could be qualified (prefixed) somehow
  • as for the refactoring scenario, well that would be your responsiblity as developer

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
Copy link
Owner

AugustNagro commented Jun 7, 2024

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:

  • We need to switch to using column names instead of integer positions when building case classes from ResultSets as described in this issue: Incorrect value for field #2 . Do we want to benchmark this change? And what are other libraries doing?
  • Figure out how would this feature interact with UDTs. Would we want separate annotations, like @embed and @udt?
  • We should explore ideas on implementing XML & JSON deserialization support as a prerequisite so they can work together harmoniously. Support Json & XML Column Deserialization #27
  • Need to support a prefix parameter like @embed(prefix = "point_a") for when a user embeds the same type multiple times. If prefix is not used, the user should receive a compile-time error with a helpful message.

@AugustNagro AugustNagro reopened this Jun 7, 2024
@alwins0n
Copy link
Author

alwins0n commented Jun 8, 2024

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

Few points of feedback:

  • I think there should be 1 embed annotation. maybe udt could be a parameter on that annotation (type name or boolean flag if derived from the class name). alt: an @Embeddableannotation similar to @Table where these options can be specified (if the dialect imported supports it)
  • the prefix would be great. I think the "attributeoverrides" in hibernate are pure boilerplate as in 99% of all cases you just want a prefix for your cols. In my opinion you could consider automatically adding a prefix derived from the field name. Again this is 99% what I do with multiple embeddables in hibernate
  • If UDTs and Arrays, and, following from that, Arrays of UDTs are supported magnum would be able to support a lot of "aggregate" datastructures out of the box, such as an user with his phonenumbers. From a lib-user perspective that would be awesome, but I see your concerns about the scope of this lib.

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".

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 a pull request may close this issue.

2 participants