DATAMONGO-2138 - Type-safe Kotlin query extension

[TjeuKayim]

https://jira.spring.io/browse/DATAMONGO-2138

I'm suggesting to add Kotlin extensions for "type-safe queries". This will prevent typos in field names, and improve the readability of a query.

New Query Syntax

We can achieve type safe queries with Kotlin's Property References.

mongoOperations.find<Book>(
  Query(Book::name isEqualTo "Moby-Dick")
)
// Nested Properties (i.e. refer to "book.author")
Book::author / Author::name regex "^H"

To-Do

• Discuss changes in type hierarchy: leave it as is
• Agree on typed query syntax.
• Implement all Criteria features.
• Update documentation.

[pivotal-issuemaster]

@TjeuKayim Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@TjeuKayim Thank you for signing the Contributor License Agreement!

[odrotbohm]

I am very reluctant to introduce changes in the type signature to cater a Kotlin extension. We don't want to add complexity for our Java developers (still more than 90% of our user base) that doesn't provide any obvious benefit. So far, the extensions we added didn't require those kinds of changes and I'd love to see that stay this way.

[TjeuKayim]

You’re right. We should change as less Java as possible, and just add Kotlin extensions. I’m just trying to fit the new class TypedQuery inside the type hierarchy of Query, and it’s not that trivial to find an elegant solution.
If that fails, we just have to overload methods that use Query with TypedQuery using Kotlin Extensions.

[TjeuKayim]

I am finally convinced that is it better not to change the signature of Query. Sometimes, it's hard to let go your idea.

[mp911de]

How would an example look like when querying nested properties? (class Book(val author: Author), class Author(val name: String), query for book.author.name)

[TjeuKayim]

I'm not sure how we should handle nested properties.
KMongo has a special "/" operator for that.
https://github.com/Litote/kmongo/blob/master/kmongo-kdoc/docs/typed-queries.md#nested-properties

[TjeuKayim]

We can also consider 'typed updates'.

operations.updateFirst(Book::name isEqualTo "Moby-Dick", set(Book::price, 11))

[TjeuKayim]

At first, I wanted to restricts the KProperty to members of the Entity class, but it requires much coding to make that work. Every use of Query as parameter will need an Kotlin extension, and the type parameter should be passed through to Criteria.
Maybe we should keep it simple, and trust the developers to use the right type. The concept of typed queries and updates will be more feasible then.

[mp911de]

Let's focus on find operations right now and make sure that we can query properties across the document – basically let's support the same criteria features as we do with the regular Query. We can add update operations later on.

[TjeuKayim]

This version has typed criteria builder functions for gt, lt, ne, size, all, gte, lte, mod, nin, isEqualTo and inValues. They can be chained using typedCriteria().

[TjeuKayim]

Let's discuss the syntax for typed queries.

1. The simplest implementation would just use the existing Criteria builder.

   Criteria(Book::name).eq("Moby-Dick")
       .and(Book::price).lt(1200)

   This requires the least coding, just some extension functions for Criteria.

2. We could use infix functions.

   criteria(
       Book::name isEqualTo "Moby-Dick",
       Book::price lt 1200
   )

   This is a very clean and good looking syntax. However, the functions have to be imported. This could led to conflicting function names.

3. Or we use lambdas with receiver.

   criteria {
       Book::name isEqualTo "Moby-Dick"
       Book::price lt 1200
   }

   The lambda's receiver has infix member functions to build typed criteria. The use of those functions will be restricted to the lambda parameter, and you don't need to import them.

What do you think?

[mp911de]

API usage should impose as little constraints as possible on client code in terms of technical constraints. Importing functions bears the risk of clashing with existing application code/external code.

Examples 2 and 3 read naturally and if option 2 could clash with other imported functions, then option 3 seems the way to go. Reading criteria within a closure easily associates the AND nature of criteria combination.

We should also considerOR queries. Maybe something like:

criteria {
    Book::name isEqualTo "Moby-Dick"
    or {
        Book::price lt 1200
        Book::price gt 240
    }
}

[TjeuKayim]

Looks nice! I was also imagining such "or" queries.
I will go for option 3 then.

[TjeuKayim]

All Criteria features are now supported by my TypedCriteriaBuilder.
Can you review my code @mp911de?

[mp911de]

Pretty decent work, I did a more in-depth review pass and found only minor issues such as formatting and we typically use AssertJ for assertions. Maybe @sdeleuze can have also a look once you're done with changes.

We'd like to include the change in the upcoming milestone release 2.2 M1 in early December. Once this PR is merged, we could have a look on typed updates.

[TjeuKayim]

Thank you for the positive feedback! 😃

[sdeleuze]

I will have look Monday asap back for vacation if that timing is ok from a Spring Data release perspective.

[TjeuKayim]

Pretty decent work, I did a more in-depth review pass and found only minor issues such as formatting and we typically use AssertJ for assertions.

What specific formatting rules do you mean @mp911de?
I tried to apply the coding standards, but those are written for Java.
Most Kotlin files in the project use tabs, but some ones use spaces, like ReactiveAggregationOperationExtensions.kt.

Also, should I start test methods with a line break?

[sdeleuze]

Thanks @TjeuKayim for this contribution.

I think there are issues in the API design that we need to fix before merging that PR. For example I am wondering if we could improve logical operators since I am not super fond of the following syntax:

or(
    { Book::price lt 1200 }
    { Book::price gt 240 }
 )

or({ Book::price lt 1200 })

I think the root issue is that the API is not composable (KProperty extensions return Unit), and I am not sure we need to use lambdas with receiver for that use case, since criteria are built from a KProperty (they are usually used to propose a set of top level functions to the developer via autocomplete).

What about just using KProperty extensions that returns Criteria and modify or and and to be infix extensions of Criteria is order to allow to translate

Criteria("title").isEqualTo("Moby-Dick")
    .orOperator(Criteria("price").lt(1200), Criteria("price").gt(240))

To (Book::title isEqualTo "Moby-Dick") or (Book::price lt 1200 and Book::price.gt 240)

That seems closer to the Java API principle, while leveraging Kotlin capabilities to provide a more readable syntax.

Any thought?

[odrotbohm]

I don't think the two statements are the same. This:

Criteria("title").isEqualTo("Moby-Dick")
    .orOperator(Criteria("price").lt(1200), Criteria("price").gt(240));

is probably meant to be:

(Book::title isEqualTo "Moby-Dick") and (Book::price lt 1200 or Book::price.gt 240)

Note the swapped and and or.

[TjeuKayim]

Alright then. Let's use top level functions, like before bc12a92.
The builder functions will return a TypedCriteria object that stores the operation and lazily implements CriteriaDefinition.

[TjeuKayim]

I did a quick refactor to change the syntax.

mongoOperations.find<Book>(Query(
  Book::name isEqualTo "Moby-Dick"
))
typedCriteria(
  Book::name isEqualTo "Moby-Dick",
  orOperator(
    Book::price lt 1200,
    Book::price gt 240
  )
)

[TjeuKayim]

I think these expressions should match:

(Book::title isEqualTo "Moby-Dick") and ((Book::price lt 1200) or (Book::price gt 240))

Criteria().andOperator(
	Criteria("title").isEqualTo("Moby-Dick"),
	Criteria().orOperator(Criteria("price").lt(1200), Criteria("price").gt(240))
)

[mp911de]

andOperator adds another indirection, so combination through AND on the top-level should reflect

Criteria("title").isEqualTo("Moby-Dick")
    .orOperator(Criteria("price").lt(1200), Criteria("price").gt(240));

andOperator is useful when specifying constraints for the same field multiple times (e.g. $and: [ { price: { $ne: 1.99 } }, { price: { $exists: true } } ])

[TjeuKayim]

Do you mean that and() and andOperator() should be two different functions? That makes sense.

[sdeleuze]

@TjeuKayim Do you think it could be possible to make KProperty extensions directly returning Criteria instances and remove TypedCriteria, typedCriteria() and typedQuery()?

[TjeuKayim]

Merge a subset of the changes we've seen so far and produce some documentation.

That's probably a good idea.
This feature request is for typed queries. So instead of plain strings, this pull request should add extensions to use Kotlin's Property references. Support for nested properties is needed. Also some extensions to use it within the Criteria builder (where(KProperty) & Criteria.and(KProperty).
I will push a new version with just those changes, and create a follow-up ticket later.

[mp911de]

I've got a new idea. What if we stick more to the original Criteria syntax. So instead of KProperty extensions, we can make new where() and and() functions that take a KPropery and delegate to their Java counterpart. Other methods of criteria can be extended with infix Kotlin functions.

We should stick to (Book::title isEqualTo "Moby-Dick") and (Book::price lt 1200).
The proposed where(Book::name) isEqualTo "Moby-Dick" and Book::price lt 1200 format is less consistent and less readable.

[sdeleuze]

@mp911de I agree with your last comment and your proposal to deal with that in 2 steps.

[sdeleuze]

@TjeuKayim Please ping me when I can review the PR updated according to @mp911de feedback, I would like to have a look before the merge.

[TjeuKayim]

I also prefer this syntax (Book::title isEqualTo "Moby-Dick") and (Book::price lt 1200).

What other features should be included in this pull request?
Just the infix KProperty extensions? Without and, nor and or.

Accidentally hit the "Close" button on my smartphone

[mp911de]

Just the infix KProperty extensions? Without and, nor and or.

Exactly. Please also squash your commits. This makes it easier to merge.

[TjeuKayim]

@sdeleuze, you can review the PR if you like.
I also squashed my commits.

How can you set asciidoctor's output to HTML? (I'm using the Maven plugin)

[mp911de]

How can you set asciidoctor's output to HTML? (I'm using the Maven plugin)

Not sure I follow. Do you want to generate the reference documentation? If so, then run Maven with the distribute profile in the project root:

$ mvn install -Pdistribute

You can find the generated documentation in target/site/reference/html/index.html.

[TjeuKayim]

Yes, I wanted to generate the reference documentation, but used the wrong maven command. With the distribute profile, it’s working. Thank you helping.

Where should I add the documentation for this feature? I was thinking of a new subheading below “Additional Query Options”, and calling it “Typed Queries for Kotlin”.

[sdeleuze]

Thanks @TjeuKayim, the latest commit is ok for me.

[mp911de]

Awesome work @TjeuKayim. Let's move Typed Queries with Kotlin directly after Fluent Template API. We will do the final review tomorrow and merge your changes to master so it can be shipped with Moore M1.

[TjeuKayim]

Great tot hear that. 😃
I will be available tomorrow from about 11:00 CET.

[TjeuKayim]

I placed the documentation after Fluent Template API.

[mp911de]

Thanks a lot. We'll take this pull request from here.

[mp911de]

That's merged and polished now. Thanks a lot for this excellent pull request.