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

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

Closed
weiwill opened this issue Aug 24, 2019 · 20 comments
Closed

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

weiwill opened this issue Aug 24, 2019 · 20 comments
Assignees
Labels
Milestone

Comments

@weiwill
Copy link

weiwill commented Aug 24, 2019

@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
Copy link
Member

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()
  1. 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
Copy link
Author

weiwill commented Aug 24, 2019

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

weiwill commented Aug 24, 2019

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

@weiwill
Copy link
Author

weiwill commented Aug 24, 2019

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

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

Note that for we also need this fix:
#1804

@rbygrave
Copy link
Member

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

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

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

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

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

weiwill commented Aug 25, 2019

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

weiwill commented Aug 25, 2019

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

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

weiwill commented Aug 25, 2019

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

@rbygrave
Copy link
Member

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 rbygrave self-assigned this Aug 25, 2019
@rbygrave rbygrave added the bug label Aug 25, 2019
@rbygrave rbygrave added this to the 11.43.3 milestone Aug 25, 2019
@rbygrave
Copy link
Member

Fixed by the fix to:

  • kotlin-querybean-generator 11.43.3
  • ebean 11.43.3

@rbygrave
Copy link
Member

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

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

weiwill commented Aug 25, 2019

Wonderful, successful repair

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

No branches or pull requests

2 participants