Reason for restriction to data classes

[cyc1ingsir]

Hi, I hoped to be able to use kmapper for a REST backend with different layers. Whilst the object is modelled as a data class, both within the outer REST layer as well as inside the domain layer, the entity object isn't.

The reason for the latter is using JPA for storing the object: https://spring.io/guides/tutorials/spring-boot-kotlin/ ==> Chapter "Persistence with JPA"

Here we don’t use data classes with val properties because JPA is not designed to work with immutable classes

Trying to map the Entity object to the domain object or vice versa

@Mapper
internal interface DomainUser2EntityMapper {
    fun map(domain: DomainUser): UserEntity
}

I'm faced with this restriction: https://github.com/s0nicyouth/kmapper/blob/bb67e21358533556214dfaaf13f546e14a0c0023/processor/src/main/kotlin/com/syouth/kmapper/processor/base/Utils.kt#L44

Could this be opened to enable mapping between e.g.

import jakarta.persistence.Entity

@Entity
class UserEntity(
   @Id var id: UUID,
    var login: String,
    var firstname: String,
    var lastname: String
    var addresses: List<Address>)

and

data class DomainUser(
    val id: UUID,
    val login: String,
    val firstname: String,
    val lastname: String,
    val addresses: List<Address>)

?

[s0nicyouth]

The restriction was imposed mostly to limit the potential area for bugs. It seems that it can be easily lifted(probably just the line you mentioned should be removed). I will try to issue a fix ASAP. PRs are always welcome however :).

[cyc1ingsir]

PRs are always welcome however :).

Loved to do so but having problems to build it locally:

[ksp] java.lang.IllegalStateException: Do not know how to convert kotlin.collections.Map<kotlin.Int, kotlin.Long> with name mapTwo and path mapTwo

I'm probably having screwed up the setup somehow. :-(

[s0nicyouth]

@cyc1ingsir I ran through the code and it seems it's not that easy(though definitely achievable). The problem is in DataClassTypeConverter whose isSupported method implementation is not strict enough and will allow for all the classes pass now including Map, List and other classes. To fix that the implementation should consider class structure itself during execution of isSupported method, specifically the possibility of mapping one class to another and return the boolean based on that. That is a bit more time consuming to implement but certainly doable. Unfortunately I won't be available to work on this this week as I'm on vacation but I will implement it soon.

[cyc1ingsir]

@s0nicyouth I've just got kMapper compiled and deleted the line mentioned above and setup an example but run in the following error.

e: [ksp] java.lang.IllegalStateException: Don't know how to map u to com.syouth.kmapper.testload.dto.nonDataClassTest.UserEntity

Here is the example project (based on the (slightly modified) and extended testload) https://github.com/cyc1ingsir/kmapper-examples If UserEntity and AddressEntity are changed to data class it does compile and run https://github.com/cyc1ingsir/kmapper-examples/blob/aded14821fa5fd5b78d1195735a6835a8b9b5d96/src/main/kotlin/com/syouth/kmapper/testload/dto/nonDataClassTest/UserEntity.kt#L8

So, hopefully, this could serve as a test example.

[cyc1ingsir]

I looked into it and it seems that changing this line https://github.com/s0nicyouth/kmapper/blob/bb67e21358533556214dfaaf13f546e14a0c0023/processor/src/main/kotlin/com/syouth/kmapper/processor/convertors/DataClassTypeConverter.kt#L25 to

return (from.isDataClass() || to.isDataClass()) &&

plus disabling these two tests: https://github.com/s0nicyouth/kmapper/blob/bb67e21358533556214dfaaf13f546e14a0c0023/processor/src/test/kotlin/com/syouth/kmapper/processor/convertors/DataClassTypeConverterTest.kt#L45-L53 is sufficient.

Since I haven't looked at the complete DataClassTypeConverter code I'm not sure if there are some potential pitfalls I haven't thought of. Also, I'm not sure, if the tests should be replaced by some other e.g.

    @Test
    fun `GIVEN neither from type nor to is data class THEN false returned`() {
        Assertions.assertFalse(converter.isSupported(mockKSType(), mockKSType(), PathHolder()))
    }

Instead of deleting the line: https://github.com/s0nicyouth/kmapper/blob/bb67e21358533556214dfaaf13f546e14a0c0023/processor/src/main/kotlin/com/syouth/kmapper/processor/base/Utils.kt#L44 I extended it to

    if (!(from.isDataClass() && to.isDataClass()) &&
        !(from.isDataClass() && !to.isDataClass()) &&
        !(!from.isDataClass() && to.isDataClass()))
        throw IllegalStateException("Either source or destination needs to be a data class")

In case both to and from are plain classes I'm hitting:

e: [ksp] java.lang.IllegalStateException: Data class should have primary constructor

from here: https://github.com/s0nicyouth/kmapper/blob/bb67e21358533556214dfaaf13f546e14a0c0023/processor/src/main/kotlin/com/syouth/kmapper/processor/base/Utils.kt#L54 I haven't investigated any further into the root cause.

However, applying the above changes, I could successfully build and run the examples referenced above. I could assemble a PR with these changes but it's probably best for you to have a closer look at this.

[cyc1ingsir]

@s0nicyouth Had you have the chance to look on my PR yet? So far, I've built my project with a dependency on the locally installed kmapper version including this enhancement locally only. Today, I started to setup a GH action but this can't access the SNAPSHOT version and hence the build fails :-( Would be really great, if you could have a look at it.

[s0nicyouth]

@cyc1ingsir sorry for the delay I had a lot of work to do at work. I took a look at your PR and although it might do the job for you it doesn't seem like a complete solution. First of all if we want to enable Class <-> Class mapping it should work in all 4 directions(all combinations of Data and regular classes). Currently it looks more like a hack for one specific case. Also changing return from.isDataClass() && to.isDataClass() && to return (from.isDataClass() || to.isDataClass()) && might potentially cause a situation when DataClassTypeConverter's isSupported method called first and CollectionTypeConverter will not be used for converting collections which will lead to exception. Probably the second issue is not that severe for not because it can be mitigated by properly ordering converters but the first one should definitely be addressed. I will try to take a look at it this Friday or Saturday if you don't solve it before that.

[cyc1ingsir]

So, if all these conversion/mappings shall be supported (which I agree with):

  from.isDataClass() &&   to.isDataClass()   //  => supported in current version
  from.isDataClass() && ! to.isDataClass()   //  => supported with the changes 
! from.isDataClass() &&   to.isDataClass()   //  => supported with the changes
! from.isDataClass() && ! to.isDataClass()   //  => not yet supported

the checks can be removed entirely from Utils.kt , can't they?

I wonder if the DataClassTypeConverter should be made capable to convert to and from ~PoJo-~ plain- classes or if an additional PlainClassTypeConverter should be added. However, I must admit, I haven't been able to work out, what type of classes the NonCollectionTypeConverter is responsible for.

when DataClassTypeConverter's isSupported method called first and CollectionTypeConverter will not be used for converting collections which will lead to exception

I have tried to construct an example for it over at https://github.com/cyc1ingsir/kmapper-examples but so far I haven't been successful.

[cyc1ingsir]

if you don't solve it before that.

Hopefully, I've tackled all of it.

[s0nicyouth]

The PR is merged. I will upload new release to maven soon.

[s0nicyouth]

@cyc1ingsir Version 1.1.0 was uploaded to the maven.

[cyc1ingsir]

@s0nicyouth Brilliant! :tada: I can confirm it's working (I have yet to make the project public).