feat(kotlin): provide operators through extension functions

[davinkevin]

To use a more fluent syntax, we provide some extension functions in the kotlin context. All the code is just a bridge with real java implementation.

Closes #11

FYI:

• Kotlin code use Java code to consistent
• I've defined extension functions only for operators, I've kept the standard format for postgres functions (with goal to look like original SQL)
• I've added a doc bloc with @see to the related method
• I've not (yet) implemented tests for the kotlin part… (I prefer to be sure we are ok on the implementation before doing tests)
• I've ported non-extension method to simplify import from kotlin code, only one class should be used.

Questions:

• What do you want for the test part ?
• Currently, extensions functions and operators are all in the same package, I think it should be better to define a package com.github.t9t.jooq.json.jsonb to separate extenions/operators. Something with enclosing class could do the job with Companion but I don't think this is a good way to do it.
• I've not ported field methods due to name conflict in the "same package" (described in the previous bullet point)

[t9t]

Hi @davinkevin! Sorry, my email provider decided to direct all of the Github mails to the spam folder, so I missed this one! Unfortunately I don't have time to look at it right now, but I'll make some time free tomorrow to have a look.

[davinkevin]

No problem, I usually work on open source during weekends and during time given by my company for this kind of project 😉

[t9t]

Looks good! I tried it out a little bit in a playground project and it's definitely a nice addition. I'm already very happy with this contribution!

I have a couple of remarks & questions of my own:

• In the signatures I see types such as Field<JSON?>? and Field<String?>? (for example, for arrayElementText()). I am not too familiar with best practices around extensions functions, but I don't think these functions could ever be called on a null Field (as that would lead to an NPE any way), and I also don't think the functions will ever return a null Field. So I think the ? for the Field could probably be dropped (but please let me know if I'm wrong on this one or check with some more seasoned Kotlin experts). I am unsure about the ? in the generic type (eg. JSON? and String? in the above example). I guess a Field could encapsulate a nullable JSON or String field, so then this is actually valid.

• Could you format JsonbDSL.kt in the same way as JsonDSL.kt? ;-) - If you use IDEA, then autoformat would be nice. 👍

To answer your questions.

What do you want for the test part ?

To be honest, the functionality of the functions is already properly tested in the integration-tests module, so I don't feel there is a lot of value in duplicating the entire test suite also for the Kotlin extension functions (especially since they just delegate to the Java code directly).

The most value is in tests proving that the extension functions work correctly (on a code/compilation level) with the jOOQ APIs and don't do anything funky (also with respect to the nullability aspect I described above). So probably two Kotlin-based test classes (for Json/Jsonb) in the integration-tests module (as there, the Postgres DB and jOOQ codegen is set up) with one very simple test function per extension function would suffice.

Let me know if this is enough information to proceed, or if you need some more help with that. Perhaps setting up the test data could yet be a bit tricky, depending on your experiences.

Currently, extensions functions and operators are all in the same package, I think it should be better to define a package com.github.t9t.jooq.json.jsonb to separate extenions/operators. Something with enclosing class could do the job with Companion but I don't think this is a good way to do it.
I've not ported field methods due to name conflict in the "same package" (described in the previous bullet point)

I'm having a little trouble understanding this one. Do you mean that you would like to introduce a "classless" field() function to match the JsonDSL.field() and JsonbDSL.field() methods, but that it's not possible to introduce them both for JSON and JSONB types in the same package because of the naming conflict?

The remark about the Companion class is something I don't really get, so it would be nice if you could elaborate that one.

In general I like the suggestion about separate packages I think.

Apart from that, I can also imagine extension functions on JSON and JSONB. Eg. for a JSONB instance:

fun JSONB.toField() : Field<JSONB> {
    return JsonbDSL.field(this)
}

I do wonder a bit about the value of a "classless" alternative to JsonbDSL.field(String) in Kotlin, as that Java method is perfectly usable in Kotlin as well. ;-)

[davinkevin]

I have a couple of remarks & questions of my own:

• In the signatures I see types such as Field<JSON?>? and Field<String?>? (for example, for arrayElementText()). I am not too familiar with best practices around extensions functions, but I don't think these functions could ever be called on a null Field (as that would lead to an NPE any way), and I also don't think the functions will ever return a null Field. So I think the ? for the Field could probably be dropped (but please let me know if I'm wrong on this one or check with some more seasoned Kotlin experts). I am unsure about the ? in the generic type (eg. JSON? and String? in the above example). I guess a Field could encapsulate a nullable JSON or String field, so then this is actually valid.
• Could you format JsonbDSL.kt in the same way as JsonDSL.kt? ;-) - If you use IDEA, then autoformat would be nice. 👍

Good catch, I totally forget to change this part of the API. After investigation, Field should not be null and the inner type could not be null too, due to the way JSON(B) are constructed.

[davinkevin]

What do you want for the test part ?

To be honest, the functionality of the functions is already properly tested in the integration-tests module, so I don't feel there is a lot of value in duplicating the entire test suite also for the Kotlin extension functions (especially since they just delegate to the Java code directly).

The most value is in tests proving that the extension functions work correctly (on a code/compilation level) with the jOOQ APIs and don't do anything funky (also with respect to the nullability aspect I described above). So probably two Kotlin-based test classes (for Json/Jsonb) in the integration-tests module (as there, the Postgres DB and jOOQ codegen is set up) with one very simple test function per extension function would suffice.

Let me know if this is enough information to proceed, or if you need some more help with that. Perhaps setting up the test data could yet be a bit tricky, depending on your experiences.

I've added tests in the integration project to check the equality between JSONx#methods and kotlin equivalent.

[davinkevin]

Currently, extensions functions and operators are all in the same package, I think it should be better to define a package com.github.t9t.jooq.json.jsonb to separate extenions/operators. Something with enclosing class could do the job with Companion but I don't think this is a good way to do it.
I've not ported field methods due to name conflict in the "same package" (described in the previous bullet point)

I'm having a little trouble understanding this one. Do you mean that you would like to introduce a "classless" field() function to match the JsonDSL.field() and JsonbDSL.field() methods, but that it's not possible to introduce them both for JSON and JSONB types in the same package because of the naming conflict?

The remark about the Companion class is something I don't really get, so it would be nice if you could elaborate that one.

In general I like the suggestion about separate packages I think.

Apart from that, I can also imagine extension functions on JSON and JSONB. Eg. for a JSONB instance:

fun JSONB.toField() : Field<JSONB> {
    return JsonbDSL.field(this)
}

I do wonder a bit about the value of a "classless" alternative to JsonbDSL.field(String) in Kotlin, as that Java method is perfectly usable in Kotlin as well. ;-)

Thank for the answer. I've introduced two packages, json and jsonb hosting both extensions files. Thank to that, we can have the toField() functions. 🎉.

[davinkevin]

And finally, during testing, I found that two extension functions defined on Field<JSONB> are conflicting with original methods of Field<T>.

Right now, I've add an underscore to the method name:

• fun Field<JSONB>._concat(field2: Field<JSONB>): Field<JSONB> = JsonbDSL.concat(this, field2)
• fun Field<JSONB>._contains(other: Field<JSONB>): Condition = JsonbDSL.contains(this, other)

I really don't think this is a solution, but I don't know what you want to do with those functions name. Do you have a better idea on this ?

/cc @lukaseder if you want to comment on this 😉.

[lukaseder]

/cc @lukaseder if you want to comment on this 😉.

On what in particular?

[davinkevin]

In the message I /cc you. Name conflict between json functions and Field<T>. If you have some good idea on or recommandation, it could be good 👍.

[lukaseder]

Well, how about jsonConcat and jsonContains...

[davinkevin]

Thanks for the answer/advice @lukaseder. @t9t I let you choose 😉.

I've removed draft attribute from the PR. If you need me to modify something, let me know 👍.

[t9t]

Thanks for the extra work! I think concatJson and containsJson roll of the tongue slightly better.

For the sake of speed, I wanted to push some small tweaks (including the above rename) to your branch before merging it, but it looks like I don't have access to your branch. I think there's a checkbox when opening/editing a pull request, to allow maintainers of the target repo to push commits to your branch.

• In the signatures I see types such as Field<JSON?>? and Field<String?>? (for example, for arrayElementText()).
  Good catch, I totally forget to change this part of the API. After investigation, Field should not be null and the inner type could not be null too, due to the way JSON(B) are constructed.

I did a little digging, and I think in jOOQ version 3.12, an SQL NULL value lead to a JSON(B) object with a null value inside of it, but this was changed in 3.13 to return a null value for the JSON(B) instead. Check these out:

• DefaultBinding should return null on null JSON values, instead of JSON[B].valueOf(null) jOOQ/jOOQ#9283
• Add support for JSON / JSONB types jOOQ/jOOQ#7242 (comment)
• https://www.jooq.org/javadoc/latest/org.jooq/org/jooq/JSON.html
• https://github.com/t9t/jooq-postgresql-json/blob/master/integration-tests/src/test/java/com/github/t9t/jooq/json/JsonBindingIT.java#L55

The way I understand it is that if the field value itself is SQL NULL (eg. when left-joining a table with a missing row), then the JSON(B) object will be Java null. On the other hand, if the field has an actual value, but that value is the JSON value of null, then there will be a JSON(B) instance containing the Java value null (with data() returning that null).

Then again I'm not entirely sure what effect that has on the nullability of the Field type and its usage in Kotlin. But if I would take a guess, then querying by Field<JSONB> (ie. not nullable) would result in a Record<JSONB>, of which the corresponding component function (eg. value1()) can't return null. I personally don't use jOOQ with Kotlin much yet, so I don't have enough experience to be able to judge what it would do interoperating with the jOOQ generated Java code here.

I'm sorry I should have looked into this more before making my previous comment!

So now I'm thinking about what the practical approach is to continue with this. I think it's perfectly backwards compatible to first accept and merge it as it is (with Field<JSON>) and then if it leads to issues later, it can easily be changed to Field<JSON?> then (the other way around would be a breaking change).

[davinkevin]

On my side, I've checked the case…

If you want to cherry pick and edit, there is no problem 👍

[t9t]

Ah, sorry! Looks like I added the fork by HTTPS rather than SSH, which gave me some authentication issues when pushing. :)

I've now pushed my changes to the branch, just some small clean-ups like removing the "Created by" comments and the function rename discussed earlier. Also added a small test which just functions to see some jOOQ+Kotlin interactions, mainly for myself to see what it looks like. You can see the commits here: https://github.com/t9t/jooq-postgresql-json/pull/12/commits

I'll merge the change now and make a release. Thank you so much for providing this addition and your patience in dealing with the comments and feedback. If you don't object, I'd like to add a "Contributors" section to the readme and place your name there, as attribution. :)

[t9t]

@davinkevin This is now released in version 1.3.0. :) Let me know if it works for you, and if you encounter any issues using it or find any omissions, please feel free to create another issue (or even pull request)!

[davinkevin]

All seems to be ok on my side 👍. Thank you for the release, and there is no problem to add me as a contributor ❤️.

Thank you and have a merry Christmas and an happy new year 🎉.