Conversation
8017828 to
aa0d1e5
Compare
6200ddd to
0b788b5
Compare
98af8a8 to
453cf3a
Compare
8f5d724 to
a8ac75b
Compare
yatharthranjan
left a comment
There was a problem hiding this comment.
Half-way through the review, please find the comments for half
| } | ||
|
|
||
| kapt { | ||
| keepJavacAnnotationProcessors = true |
There was a problem hiding this comment.
which annotations is this for? can you check if there support for kapt from other equivalent libs?
There was a problem hiding this comment.
The spring-context-indexer dependency, annotationProcessor "org.springframework:spring-context-indexer:$springVersion", was used for the mixed Java-Kotlin codebase. I'll remove this configuration.
| } | ||
|
|
||
| jar { | ||
| duplicatesStrategy = DuplicatesStrategy.INCLUDE |
There was a problem hiding this comment.
which deps are duplicated? Might be good to add a resolution strategy
There was a problem hiding this comment.
The file spring-configuration-metadata.json exists in two locations: one generated by KAPT and the other copied from the main resource directory, which ends up in the build location:
radar-appserver/build/tmp/kapt3/classes/main/META-INF/spring-configuration-metadata.json
radar-appserver/build/resources/main/META-INF/spring-configuration-metadata.json
The contents of both files are different. I created a custom task to read the content of the duplicate files, and the results are in the attached file
| hibernateValidatorVersion = '8.0.0.Final' | ||
| minioVersion = '8.5.10' | ||
| kotlinVersion = '1.9.25' | ||
| jacksonKotlinVersion = '2.15.4' |
There was a problem hiding this comment.
would prefer moving to kotlinx serialisation instead of jackson
There was a problem hiding this comment.
Currently, both the appserver and Jersey use Jackson. Once I complete the Jersey work, I'll migrate from Jackson to Kotlinx Serialization by next week.
|
|
||
| //Kotlin | ||
| implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion" | ||
| implementation "com.fasterxml.jackson.module:jackson-module-kotlin:$jacksonKotlinVersion" |
There was a problem hiding this comment.
can be removed when moving to kotlinx with json contentNegotiation
| implementation("io.ktor:ktor-client-core:$ktorVersion") | ||
| implementation "io.ktor:ktor-client-cio:$ktorVersion" | ||
| implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutinesVersion" | ||
| testImplementation "org.mockito.kotlin:mockito-kotlin:$mockitoKotlinVersion" |
There was a problem hiding this comment.
are we moving to mockk instead of mockito ?
There was a problem hiding this comment.
Yes, when writing the unit test for jersey I'll create the test cases using mockk.
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| data class DataMessageStateEventDto ( | ||
| var id: Long? = null, |
There was a problem hiding this comment.
any reason all of these are var. Avoid vars where possible, especially in a data contract/interface.
There was a problem hiding this comment.
Since JPA doesn't encourage the use of data classes with val fields, I kept the fields as var and nullable in DTOs. This way, they can be easily mapped without needing strict null checks (!!) for every field.
For more information on not using data classes and val fields with JPA: Medium - A practical point of view on Kotlin data classes and JPA/Hibernate.
|
|
||
| @field:NotNull | ||
| @field:NotEmpty | ||
| var projectId: String? = null, |
There was a problem hiding this comment.
can we make this non-nullable and remove the annotation?
There was a problem hiding this comment.
Same reason as above.
| class FcmDataMessages { | ||
|
|
||
| @field:Size(max = 200) | ||
| private var _dataMessages: MutableList<FcmDataMessageDto> = mutableListOf() |
There was a problem hiding this comment.
you can use val here and have a constructor for setting dataMessages instead of fun withDataMessages
There was a problem hiding this comment.
Okay, I'll update this.
| @Suppress("unused") | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| class FcmDataMessageDto(dataMessageEntity: DataMessage? = null) : Serializable { | ||
| var id: Long? = dataMessageEntity?.id |
There was a problem hiding this comment.
you can use val with constructors?
There was a problem hiding this comment.
Same reason for using var fields as mentioned in the above comment.
| @Suppress("unused") | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| class FcmNotificationDto(notificationEntity: Notification? = null) : Serializable { | ||
| var id: Long? = notificationEntity?.id |
There was a problem hiding this comment.
Similar you can use val with constructors instead of with... If you need to update the properties of the object somewhere, you can make a copy
There was a problem hiding this comment.
Okay, I'll update this.
Refactor AppServer to Kotlin
Solves: #332