redis / redis-om-spring

Spring Data Redis extensions for better search, documents models, and more
MIT License
604 stars 93 forks source link

Kotlin Support #250

Closed lucasleon2107 closed 1 year ago

lucasleon2107 commented 1 year ago

I using Spring Boot (3.0.2) with JDK 17, Kotlin (1.8.20) and the latest version of Redis OM (0.8.2). I configured the annotation processor using kapt. Here's the snippet:

<plugin>
                <groupId>org.jetbrains.kotlin</groupId>
                <artifactId>kotlin-maven-plugin</artifactId>
                <version>${kotlin.version}</version>
                <extensions>true</extensions>
                <executions>
                    <execution>
                        <id>kapt</id>
                        <configuration>
                            <sourceDirs>
                                <sourceDir>src/main/kotlin</sourceDir>
                            </sourceDirs>
                            <annotationProcessorPaths>
                                <annotationProcessorPath>
                                    <groupId>com.redis.om</groupId>
                                    <artifactId>redis-om-spring</artifactId>
                                    <version>0.8.0</version>
                                </annotationProcessorPath>
                            </annotationProcessorPaths>
                        </configuration>
                    </execution>
                </executions>
                <configuration>
                    <args>
                        <arg>-Xjsr305=strict</arg>
                    </args>
                    <compilerPlugins>
                        <plugin>spring</plugin>
                    </compilerPlugins>
                </configuration>
                <dependencies>
                    <dependency>
                        <groupId>org.jetbrains.kotlin</groupId>
                        <artifactId>kotlin-maven-allopen</artifactId>
                        <version>${kotlin.version}</version>
                    </dependency>
                </dependencies>
            </plugin>

This is the class that is being generated:

public final class UserInfoHash$ {
  public static MetamodelField<UserInfoHash, String> _KEY;

  static {
    try {
      _KEY = new MetamodelField<UserInfoHash, String>("__key", String.class, true);
    } catch(NoSuchFieldException | SecurityException e) {
      System.err.println(e.getMessage());
    }
  }
}

But I'm getting the following error: exception java.lang.NoSuchFieldException is never thrown in body of corresponding try statement

bsbodden commented 1 year ago

Lucas, can you post the code for the UserInfoHash - I think I see what the problem is. I'm assuming that there will be other fields besides the id. But please post the code or similar code so that I can confirm.

lucasleon2107 commented 1 year ago

Sorry, I'm not near my computer right now, but it's a User data class with the @Document annotation, a String @Id that I'm setting myself, not autogenerated. Name and last name with @Searchable and a code String field with @Indexed. I hope that helps

lucasleon2107 commented 1 year ago

This is the class

@Document
data class UserInfoHash(
    @Id
    val accountId: String = "",
    @Searchable
    val firstName: String? = null,
    @Searchable
    val lastName: String? = null,
    @Indexed
    val code: String? = null
)
lucasleon2107 commented 1 year ago

@bsbodden any updates?

bsbodden commented 1 year ago

Not yet, there hasn't been much testing for Kotlin so I'm in the process of building some tests.

lucasleon2107 commented 1 year ago

@bsbodden is there a way that I can make a query by multiple optional query params? I want to filter by name or last name and maybe other additional fields that I could add that will come from the controller

lucasleon2107 commented 1 year ago

@bsbodden I assume I have to use Predicates. Can you show me an example of how to achieve that?

bsbodden commented 1 year ago

@bsbodden is there a way that I can make a query by multiple optional query params? I want to filter by name or last name and maybe other additional fields that I could add that will come from the controller

Lucas, stop by our discord server http://discord.gg/redis and I can help you there. On the issue on this page, it looks like we might need to use the "all open plugin" and make modifications to OM to support Kotlin https://kotlinlang.org/docs/all-open-plugin.html#maven

So in the meantime, unfortunately, I would assume that there is no Kotlin support until announced. I'm building a comprehensive test suite to validate the Kotlin support and I would expect it out in 0.8.4 or so.

bsbodden commented 1 year ago

@lucasleon2107 I think I have a running example working now. It was just a matter of getting kapt to behave correctly. I will post a link to a sample repo later today.

bsbodden commented 1 year ago

@lucasleon2107 see https://github.com/redis-developer/redis-om-spring-kotlin-skeleton-app

lucasleon2107 commented 1 year ago

@bsbodden This great, thanks! I'll try it later today. I have a question. Do you have an example of how to make queries with the Java Predicate class? I did some tests, but they currently didn't work for me

bsbodden commented 1 year ago

There are some examples in that repo.

lucasleon2107 commented 1 year ago

I tested your repo, and it does not consistently work for me. I got it to work once, but then I ran mvn clean compile and also tested with just ./mvnw spring-boot:run, but I'm getting this error:

cannot find symbol
[ERROR]   symbol:   class Person
[ERROR]   location: class com.redis.om.skeleton.models.Person$

Also, I copied the kapt configuration from the repo to my project, but I'm still getting exception java.lang.NoSuchFieldException is never thrown in body of corresponding try statement

Regarding the filtering with the Java Predicate class, according to the filter method, it can receive the following classes:

But in your repo, there are only examples with SearchFieldPredicate. I would like to know how to send Predicate to achieve filtering since the MetaModel generation is not working for me.

lucasleon2107 commented 1 year ago

I made a change in the kotlin-maven-plugin . I removed <extensions>true</extensions> and added this to the execution:

<goals>
    <goal>kapt</goal>
</goals>

I'm still getting the cannot find symbol error, but at least the metamodel is generated without causing issues. Also, I just noticed that I needed to set all fields as var to get all the fields generated in the metamodel. I'll continue my tests and let you know if I find any more issues.

lucasleon2107 commented 1 year ago

I tested the findByFirstNameAndLastName method in your repo by sending null parameters, and I got this error:

Cannot invoke "Object.getClass()" because "maybeText" is null

In my case that I'm doing queries with contains I get this error when sending a null param:

Error details: Cannot invoke "Object.toString()" because the return value of "com.redis.om.spring.search.stream.predicates.fulltext.LikePredicate.getValue()" is null
bsbodden commented 1 year ago

Thanks, if you get it all working, can you send a PR to that repo? I'll add some null checking on those predicates on the next release.

lucasleon2107 commented 1 year ago

@bsbodden, is there a way to bypass those errors with the current version?

lucasleon2107 commented 1 year ago

Hey @bsbodden, could you help me by sharing a code snippet illustrating how to implement filtering based on optional query parameters? It would be great if the snippet were compatible with the current version. Thanks!

lucasleon2107 commented 1 year ago

So I tried to achieve that with the Example class, but RedisDocumentRepository does not support it. Disappointing

bsbodden commented 1 year ago

Obviously much more testing required for full Kotlin support. Likely a 1.0.0 feature.

lucasleon2107 commented 1 year ago

I understand, but I don't think this is a Kotlin issue. Does sending null for MetaModels predicates or sending a Predicate work on Java?

lucasleon2107 commented 1 year ago

@bsbodden I achieved what I wanted by doing something like this:

fun search(
        firstName: String? = null,
        lastName: String? = null,
        code: String? = null
    ): List<UserInfoHash> {
        var entityStream = entityStream.of(UserInfoHash::class.java)

        val predicates = mutableListOf<SearchFieldPredicate<UserInfoHash, String>>()

        code?.let {
            val predicate = `UserInfoHash $`.CODE.eq(code.uppercase())
            predicates.add(predicate)
        }

        firstName?.let {
            val predicate = `UserInfoHash$`.FIRST_NAME.containing(firstName.uppercase())
            predicates.add(predicate)
        }

        lastName?.let {
            val predicate = `UserInfoHash$`.LAST_NAME.containing(lastName.uppercase())
            predicates.add(predicate)
        }

        if (predicates.isNotEmpty()) {
            val combinedPredicates = combinePredicates(predicates)
            entityStream = entityStream.filter(combinedPredicates)
        }

        return entityStream.collect(Collectors.toList())
    }

    private fun <E, T> combinePredicates(predicates: List<SearchFieldPredicate<E, T>>): SearchFieldPredicate<E, T> {
        return object : SearchFieldPredicate<E, T> {
            override fun getSearchFieldType(): Schema.FieldType? {
                return null
            }

            override fun getField(): Field? {
                return null
            }

            override fun getSearchAlias(): String? {
                return null
            }

            override fun test(t: T): Boolean {
                return predicates.all { it.test(t) }
            }

            override fun apply(node: Node): Node {
                var currentNode = node
                for (predicate in predicates) {
                    currentNode = predicate.apply(currentNode)
                }
                return currentNode
            }
        }
    }

It's not ideal, but at least it works. I also noticed that the containing query needs 3 to 4 characters to make a match. Is there a way where I can send just one character to get results?

bsbodden commented 1 year ago
lucasleon2107 commented 1 year ago

Well, it looks like the inconsistent behavior is still present. I tried that new config that I mentioned, but I still get this error after mvn clean compile:

cannot find symbol
symbol:   class Person
location: class com.redis.om.skeleton.models.Person$

It still generates the MetaModel and works when I run the project with IntelliJ. And even though subsequent mvn compile executions do not create more errors, it always fails when I run mvn clean compile, breaking my Jenkins pipeline because it starts from a clean state.

bsbodden commented 1 year ago

The issue is that the order should be:

This somehow happens in the right order with a regular Maven build using just Java, but it is out of order when using KAPT. I'll figure that out soon.

lucasleon2107 commented 1 year ago

I just fixed it by adding this plugin

<plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>build-helper-maven-plugin</artifactId>
    <version>3.2.0</version>
    <executions>
        <execution>
            <id>add-source</id>
            <phase>generate-sources</phase>
            <goals>
                <goal>add-source</goal>
            </goals>
            <configuration>
                <sources>
                    <source>src/main/kotlin</source>
                </sources>
            </configuration>
        </execution>
    </executions>
</plugin>
lucasleon2107 commented 1 year ago

I already created the PR. Also, I ran FT.CONFIG SET MINPREFIX 1, but I'm still needed to send 3 characters or sometimes even 4 when doing a containing query

bsbodden commented 1 year ago

In your example, you have:

code?.let {
    val predicate = `UserInfoHash $`.CODE.eq(code.uppercase())
    predicates.add(predicate)
}

firstName?.let {
    val predicate = `UserInfoHash$`.FIRST_NAME.containing(firstName.uppercase())
    predicates.add(predicate)
}

lastName?.let {
    val predicate = `UserInfoHash$`.LAST_NAME.containing(lastName.uppercase())
    predicates.add(predicate)
}

By adding the null checks and having the predicates be ignored if the params are null you can now do several chained filter commands where the params of the predicates could be null. This will AND the predicates. There is an or chaining method in Predicate that I haven't retrofitted to support this. So for now, only AND is supported.

Here's the PR https://github.com/redis/redis-om-spring/pull/251 - Once it's reviewed it should publish v0.8.3-SNAPSHOT

lucasleon2107 commented 1 year ago

Hi @bsbodden, I tried sending null values in the latest version (0.8.4), but it's still not working

bsbodden commented 1 year ago

@lucasleon2107 can you modify the sample at https://github.com/redis-developer/redis-om-spring-kotlin-skeleton-app and add a reproducer. I'm working on adding a proper set of Kotlin specific tests to the redis-on repo.

lucasleon2107 commented 1 year ago

Hi @bsbodden, can you please provide more specifics about what do you need me to add to the repo?

bsbodden commented 1 year ago

Hi @bsbodden, can you please provide more specifics about what do you need me to add to the repo?

Maybe a quick test on the main app https://github.com/redis-developer/redis-om-spring-kotlin-skeleton-app/blob/main/src/main/kotlin/com/redis/om/skeleton/OmKotlinSkeletonApplication.kt to show the failure, modify the models as needed. I'll then add it to the Kotlin set of tests I'm making.

lucasleon2107 commented 1 year ago

Hi @bsbodden, I added a quick test on the main file. You can see that in this branch: https://github.com/lucasleon2107/redis-om-spring-kotlin-skeleton-app/tree/feature/test-null-filter-values

Also, I noticed that the generated Redis command for contains queries is: %%%{query}%%%. According to the docs, this performs a fuzzy match, and I'm getting unexpected results. It seems that the correct command should be: *{query}*