AugustNagro / magnum

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

Incorrect value for field #2

Open EvgenyAfanasev opened 1 year ago

EvgenyAfanasev commented 1 year ago

Hi. I have next model:

  @Table(PostgresDbType, SqlNameMapper.CamelToSnakeCase)
  final case class Modules(
      @Id id: Int,
      title: String,
      parentId: Int,
      description: Option[String]
  )

With next values:

Modules(1, "Module title", 2, Some("Module Description"))

And with next table definition:

id INT title VARCHAR(255) description VARCHAR(255) course_id INT

And when I tried add entity using insertReturning method I received error

Incorrect value for int: Module Description

I checked columns in my table definition and see problem with order of model attributes. I have to create ordering of case class properties like in database for working. Is it correct behavior ? IMHO, developer hasn't to think about ordering tables columns for creating dao layer

AugustNagro commented 1 year ago

We can change that by using the ResultSet columnLabel methods instead of columnIndex. Like getInt(String) instead of getInt(int)

Another option is to add an @Order(int) annotation and then put documentation in the readme.

I think I like Option 1 better. That's what spring data does and it's very user friendly, despite being less efficient (ResultSet javadoc says "In general, using the column index will be more efficient")

WDYT?

EvgenyAfanasev commented 1 year ago

Yeah, 1 option is more friendly

AugustNagro commented 1 year ago

Yep agreed. I think we will want to add readSingle(rs: ResultSet, columnName: String): E to DbCodec as well.

EvgenyAfanasev commented 1 year ago

But what do think about separation DbCodec on two parts: DbCodec as reader by column names and DbIndexedCodec as reader by index ? I can take it to work

AugustNagro commented 1 year ago

Sweet, thanks for taking it on. I am biased towards just having it all in the one DbCodec, since there are no cases where you can do one but not the other.

On Sun, Jun 25, 2023, 7:16 AM Evgeny Afanasev @.***> wrote:

But what do think about separation DbCodec on two parts: DbCodec as reader by column names and DbIndexedCodec as reader by index ? I can take it to work

— Reply to this email directly, view it on GitHub https://github.com/AugustNagro/magnum/issues/2#issuecomment-1606109391, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQOKNT7OE6CYDTW64GXUM3XNBB37ANCNFSM6AAAAAAZSU4ULI . You are receiving this because you commented.Message ID: @.***>

lbialy commented 1 year ago

@EvgenyAfanasev just a quick remark - tuples don't make much sense with named args but they do make a lot of sense with positional args. I guess derivation will have to make that choice and go with labels for case classes and indexes for tuples.

AugustNagro commented 1 year ago

Right. If we end up going with readSingle(rs: ResultSet, columnName: String): E I think it's ok to throw UnsupportedOperationException for Tuples. We should also add a check in the RepoDefaults macro to make sure each case class field's dbCodec.cols.length == 1, since nesting gets weird when using names, and it's a good practice for entity classes to be flat anyway.

lbialy commented 1 year ago

I'd still support tuples, they are awesome for arbitrary queries that pull small pieces of larger tables. Is it a big deal to support both labeled and positional writing/reading in DbCodec?

cornerman commented 3 months ago

Just read this thread, and thought that the proposal for named tuples in scala might relevant here as well: https://github.com/scala/improvement-proposals/pull/72

I think, it would be great to support them when they are available.