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

KotlinGenerator should generate open properties in generated records #13467

Closed
HaydenMeloche opened this issue Apr 20, 2022 · 9 comments
Closed

Comments

@HaydenMeloche
Copy link

HaydenMeloche commented Apr 20, 2022

Use case:

Using the KotlinGenerator for JOOQ, it creates various records such as

open class ExampleRecord() : UpdatableRecordImpl<ExampleRecord>(Example.ExampleRecord), Record9<UUID?, UUID?, UUID?, String?, LocalDate?, LocalDate?, LocalDateTime?, LocalDateTime?, Boolean?> {

    var id: UUID?
        set(value): Unit = set(0, value)
        get(): UUID? = get(0) as UUID?

Some use cases require field level properties within classes to be open. For example, ModelMapper requires fields to be not final when provided with custom type mappings. By default, Kotlin fields are considered final.

Making the field open fixes these sort of compatibility issues.

open var id: UUID?
        set(value): Unit = set(0, value)
        get(): UUID? = get(0) as UUID?

Possible solution you'd like to see:

Similar to the visibility modifier I think it would be nice for JOOQ to provide a native configuration option to specify fields as open.

Possible workarounds:

The only workaround I'm aware of is manually editing the fields generated.

Versions:

  • jOOQ: 3.16.5
  • Java: 11.0.14
@lukaseder
Copy link
Member

Thanks for your suggestion. Why does ModelMapper require that, given that there are getters and setters, which it should use instead?

@HaydenMeloche
Copy link
Author

Thanks for your suggestion. Why does ModelMapper require that, given that there are getters and setters, which it should use instead?

@lukaseder That I don't particularly know. I know it only applies when you bring in a custom mapping. A default setup does not require any fields to be open. modelmapper/modelmapper#148 (comment)

I'm not familiar with the JOOQ codebase, but if pointed in the right direction I may be able to tackle this.

@lukaseder
Copy link
Member

I see. I checked the bytecode generated from the two versions. The difference is:

Without open:

  // Method descriptor #28 ()Ljava/lang/Integer;
  // Stack: 2, Locals: 1
  @org.jetbrains.annotations.Nullable
  public final java.lang.Integer getI$jooq_test_codegen_kotlin();
    0  aload_0 [this]
    1  iconst_0
    2  invokevirtual org.jooq.codegen.test.recordtomutablepojomapper.db.tables.records.TRecord.get(int) : java.lang.Object [33]
    5  checkcast java.lang.Integer [35]
    8  areturn
      Line numbers:
        [pc: 0, line: 19]
      Local variable table:
        [pc: 0, pc: 9] local: this index: 0 type: org.jooq.codegen.test.recordtomutablepojomapper.db.tables.records.TRecord

With open

  // Method descriptor #28 ()Ljava/lang/Integer;
  // Stack: 2, Locals: 1
  @org.jetbrains.annotations.Nullable
  public java.lang.Integer getI$jooq_test_codegen_kotlin();
    0  aload_0 [this]
    1  iconst_0
    2  invokevirtual org.jooq.codegen.test.recordtomutablepojomapper.db.tables.records.TRecord.get(int) : java.lang.Object [33]
    5  checkcast java.lang.Integer [35]
    8  areturn
      Line numbers:
        [pc: 0, line: 19]
      Local variable table:
        [pc: 0, pc: 9] local: this index: 0 type: org.jooq.codegen.test.recordtomutablepojomapper.db.tables.records.TRecord

So, the difference is that these getters and setters are either final (default) or not. I guess we won't lose much by making these methods non-final for purposes like yours. It would more closely reflect what the JavaGenerator produces.

I don't want to spend too much time making this configurable, or pointing you in directions, given that you don't have access to the integration tests or the commercial source code. I'll just go ahead and implement this minor change for jOOQ 3.17.

@lukaseder lukaseder changed the title KotlinGenerator supporting open field propeties within classes KotlinGenerator should generate open properties in generated records Apr 21, 2022
@lukaseder
Copy link
Member

As far as I can tell, only records are affected? "POJOs" produce data classes by default, which are implicitly final. Interfaces are open by default.

@lukaseder
Copy link
Member

Can you try if this fixes the issue for you?

https://github.com/jOOQ/jOOQ/commit/0b3c99e00ac76f8882ae4b0cfa10b688bdf28229.patch

@lukaseder
Copy link
Member

Of course, the patch causes a regression when we generate @set:JvmName, because of https://stackoverflow.com/q/47504279/521799

@lukaseder
Copy link
Member

It seems we can just apply the usual ugly workaround of annotating those properties with @Suppress("INAPPLICABLE_JVM_NAME"):

https://youtrack.jetbrains.com/issue/KT-31420

Since this only affects properties produce the conflict documented here: #11912, I can live with this ugliness.

@lukaseder
Copy link
Member

Done

@HaydenMeloche
Copy link
Author

@lukaseder Thanks you!

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

No branches or pull requests

2 participants