ebean-orm / ebean

Ebean ORM
https://ebean.io
Apache License 2.0
1.46k stars 260 forks source link

Q: Why can't kapt kotlin generate var Set to query bean #1803

Closed weiwill closed 5 years ago

weiwill commented 5 years ago
@Entity
@Table(name = "sys_user")
open class SysUser {
    var name: String? = null

    @ManyToMany
    var roles: Set<SysRole>? = null

    @OneToOne
    @JoinColumn(name = "job_id")
    var job: SysJob? = null
}
@Generated("io.ebean.querybean.kotlin-generator")
@TypeQueryBean
class QSysUser : TQRootBean<SysUser, QSysUser> {

  companion object {
    /**
     * shared 'Alias' instance used to provide
     * properties to select and fetch clauses
     */
    val _alias = QSysUser(true)
  }

  lateinit var name: PString<QSysUser>
  lateinit var job: QAssocSysJob<QSysUser>

  /**
   * Construct with a given EbeanServer.
   */
  constructor(server: EbeanServer) : super(SysUser::class.java, server)

  /**
   * Construct using the default EbeanServer.
   */
  constructor() : super(SysUser::class.java)

  /**
   * Construct for Alias.
   */
  private constructor(dummy: Boolean) : super(dummy)
}

lost roles in query bean

If I replace

var roles: Set < SysRole >? = null 

for

val roles: Set < SysRole >? = null

It generates

lateinit var roles: QAssocSysRole<QSysUser>
rbygrave commented 5 years ago

I can't reproduce with the latest version of kotlin-querybean-generator

  kapt 'io.ebean:kotlin-querybean-generator:11.43.2'

Can you upgrade and try the latest version.

As 2 side notes for collection types:

  1. we generally should define those as non-nullable kotlin types so:

    @ManyToMany
    var roles: Set <SysRole> = mutableSetOf()
  2. we generally should prefer to use List over Set. Set implicitly uses equals/hashcode and there are benefits in NOT using Set when we add beans into it that don't yet have an Id value.

So List is preferred for this reason. Hibernate folks use Set because it has better semantics in Hibernate but that isn't the case for Ebean and we recommend List due to it's better behaviour wrt equals/hashcode.

The recommendation would be:

  @ManyToMany
  var roles : List<SysRole>  = mutableListOf()
weiwill commented 5 years ago

I upgraded to 11.43.2. The problem remains. Is there anything else that could lead to this?

and Thank you for your reply and suggestion. It's very useful to me. Thank you very much.

Attached is my gradle configuration

buildscript {
    ext {
        kotlinVersion = '1.3.21'
        springBootVersion = '2.2.0.BUILD-SNAPSHOT'
        ebeanVersion = '11.43.2'
        ebeanGradlePluginVersion = '11.39.1'
        ebeanTestConfigVersion = '11.41.2'
        jjwtVersion = '0.9.1'
        jetcacheVersion = '2.5.11'
    }
    repositories {
        maven { url 'http://maven.aliyun.com/nexus/content/groups/public/' }
        mavenLocal()
        mavenCentral()
        maven { url 'https://repo.spring.io/snapshot' }
        maven { url 'https://repo.spring.io/milestone' }
    }
    dependencies {
        classpath("org.springframework.boot:spring-boot-gradle-plugin:${springBootVersion}")
        classpath("org.jetbrains.kotlin:kotlin-gradle-plugin:${kotlinVersion}")
        classpath("org.jetbrains.kotlin:kotlin-allopen:${kotlinVersion}")
        classpath "io.ebean:ebean-gradle-plugin:${ebeanGradlePluginVersion}"
    }
}

apply plugin: 'kotlin'
apply plugin: 'kotlin-spring'
apply plugin: 'org.springframework.boot'
apply plugin: 'io.spring.dependency-management'
apply plugin: 'io.ebean'
apply plugin: "org.jetbrains.kotlin.jvm"
apply plugin: "org.jetbrains.kotlin.kapt"

group = 'com.ysh'
version = '0.0.1'
sourceCompatibility = '1.8'

compileKotlin {
    kotlinOptions {
        freeCompilerArgs = ['-Xjsr305=strict']
        jvmTarget = '1.8'
    }
}

compileTestKotlin {
    kotlinOptions {
        freeCompilerArgs = ['-Xjsr305=strict']
        jvmTarget = '1.8'
    }
}

repositories {
    maven { url 'http://maven.aliyun.com/nexus/content/groups/public/' }
    mavenLocal()
    mavenCentral()
    maven { url 'https://repo.spring.io/snapshot' }
    maven { url 'https://repo.spring.io/milestone' }
}

dependencyManagement {
    imports { mavenBom("org.springframework.boot:spring-boot-dependencies:${springBootVersion}") }
}

ebean {
    debugLevel = 1
}

test {
    testLogging.showStandardStreams = true
    testLogging.exceptionFormat = 'full'
}

dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-aop'
//    implementation 'org.springframework.boot:spring-boot-starter-cache'
    implementation 'org.springframework.boot:spring-boot-starter-security'
    implementation 'org.springframework.boot:spring-boot-starter-validation'
    implementation('org.springframework.boot:spring-boot-starter-web', {
        exclude group: 'org.springframework.boot', module: 'spring-boot-starter-tomcat'
    })
    implementation 'org.springframework.boot:spring-boot-starter-undertow'
    implementation 'com.fasterxml.jackson.module:jackson-module-kotlin'

    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8"
    implementation "org.jetbrains.kotlin:kotlin-reflect"

    implementation 'mysql:mysql-connector-java'

    implementation "io.ebean:ebean-querybean:$ebeanVersion"
    implementation "io.ebean:ebean:$ebeanVersion"
    kapt "io.ebean:kotlin-querybean-generator:$ebeanVersion"

    implementation "io.jsonwebtoken:jjwt:$jjwtVersion"

    // https://github.com/alibaba/jetcache/wiki/Home_CN
    implementation "com.alicp.jetcache:jetcache-anno:$jetcacheVersion"
    implementation "com.alicp.jetcache:jetcache-autoconfigure:$jetcacheVersion"

    runtimeOnly 'org.springframework.boot:spring-boot-devtools'
    testImplementation 'org.springframework.boot:spring-boot-starter-test'
    testImplementation 'org.springframework.security:spring-security-test'
    testImplementation "io.ebean.test:ebean-test-config:$ebeanTestConfigVersion"
}

if need any other information?

weiwill commented 5 years ago

If var and Set, List or Map exist at the same time, the fields of query beans cannot be generated.

weiwill commented 5 years ago

admin.zip

I uploaded a sample project

Running kaptKotlin will find that the QSysUser generated by SysUser lacks the role field.

But if you change val roles: Set < SysRole >? = null to var roles: Set < SysRole >? = null in SysUser

The field lateinit var roles: QAssocSysRole < QSysUser > is generated.

rbygrave commented 5 years ago

Note: In show bytecode we see

  // access flags 0x2
  // signature Ljava/util/List<+Lcom/admin/system/domain/SysRole;>;
  // declaration: java.util.List<? extends com.admin.system.domain.SysRole>
  private Ljava/util/List; roles
  @Ljavax/persistence/ManyToMany;()
  @Ljavax/persistence/JoinTable;(name="sys_users_roles", joinColumns={@Ljavax/persistence/JoinColumn;(referencedColumnName="id", name="user_id")}, inverseJoinColumns={@Ljavax/persistence/JoinColumn;(referencedColumnName="id", name="role_id")})
  @Lorg/jetbrains/annotations/NotNull;() // invisible

Note the wildcard declaration. This almost ... almost looks like a Kotlin compiler bug because I'm not sure that the wildcard extends is expected here for roles. Hmmm.

rbygrave commented 5 years ago

Note that for we also need this fix: https://github.com/ebean-orm/ebean/issues/1804

rbygrave commented 5 years ago

Side notes, suggestions for making more things Kotlin non-nullable types:

Instead of this ...

    @NotNull
    var enabled: Boolean? = null

Make it a non nullable type ...

    var enabled: Boolean = false

Instead of ...

open class SysUser : BaseModel() {

    @NotBlank
    @Column(unique = true)
    var username: String? = null

Have username as a constructor parameter of non-nullable String ...

open class SysUser(username: String) : BaseModel() {

    @NotBlank
    @Column(unique = true)
    var username: String = username
rbygrave commented 5 years ago

Side note: The use of LocalDateTime type looks suspicious (due to missing timezone aspect). I'd instead expect one of Instant, ZonedDateTime, OffsetDateTime or java.sql.Timestamp.

    var lastPasswordResetTime: LocalDateTime? = null
rbygrave commented 5 years ago

Generally if this is a new DB schema I'd pretty much not expect any @JoinColumn or @JoinTable annotations.as these all have good defaults via the naming convention.

For example, the @JoinColumn can be removed - that matches the naming convention so isn't needed.

    @OneToOne
    @JoinColumn(name = "job_id")   // <-- we can remove this as it matches the naming convention
    var job: SysJob? = null
rbygrave commented 5 years ago

We can review the db-create-all.sql for the generated foreign key names like:

alter table sys_user add constraint fk_sys_user_job_id foreign key (job_id) references sys_job (id) on delete restrict on update restrict;

alter table sys_user add constraint fk_sys_user_dept_id foreign key (dept_id) references sys_dept (id) on delete restrict on update restrict;
rbygrave commented 5 years ago

The Id type is varchar(255) .. probably isn't ideal/correct. We'd expect a Long or UUID generally and a small String when there is a good ISO code (Countries, Currencies etc).

open class BaseModel : Model() {
  @Id
  var id: String? = null    // UUID or Long = 0  ... are more expected  
weiwill commented 5 years ago

Side note: The use of LocalDateTime type looks suspicious (due to missing timezone aspect). I'd instead expect one of Instant, ZonedDateTime, OffsetDateTime or java.sql.Timestamp.

    var lastPasswordResetTime: LocalDateTime? = null

Yes, I'm going to change to Timestamp.

weiwill commented 5 years ago

The Id type is varchar(255) .. probably isn't ideal/correct. We'd expect a Long or UUID generally and a small String when there is a good ISO code (Countries, Currencies etc).

open class BaseModel : Model() {
  @Id
  var id: String? = null    // UUID or Long = 0  ... are more expected  

When I was working on crawlers, I found that some website data could be obtained through circular numbers, so I wanted to avoid these in my project.

rbygrave commented 5 years ago

obtained through circular numbers

Yes quite right. Note that UUID can be encoded to binary(16) for databases that don't have native uuid support. So binary(16) could be considered better that varchar in the sense of smaller storage, smaller indexes etc.

weiwill commented 5 years ago

I donated $10 as a thank you. I really like your project and admire your work.

rbygrave commented 5 years ago

Yes I see that just arrive, great thanks!!

If more people / organisations did that we could dedicate more time, get better documentation etc. Thanks for the donation!!

Cheers, Rob.

rbygrave commented 5 years ago

Fixed by the fix to:

rbygrave commented 5 years ago

Note that as part of this I bumped Gradle to 5.6. That didn't fix the issue but probably the recommendation is to be on Gradle 5.2 or greater as that in theory has better annotation processing support.

rbygrave commented 5 years ago

Also note that the latest versions of ebean-gradle-plugin has only been released to the gradle plugins repo and not to maven central.

For example, 11.43.2 is in gradle plugins but not in maven central.

weiwill commented 5 years ago

Wonderful, successful repair