Jooq: Add option to handle nullability in KotlinGenerator

[lukaseder]

jOOQ 3.14 introduced a new KotlinGenerator similar to the existing ScalaGenerator, which produces kotlin code from a database (#6248).

The out of the box implementation generates all data types as T?, which is the only correct type in all cases. In SQL, all types can be nullable (e.g. after a left join), despite there being NOT NULL constraints.

However, many users want some additional type information attached to Field<T> types, running the risk of false negatives, where NPE are possible. We could offer some configuration flags with various degrees of T vs T? type support:

• [x] The default behaviour is to produce false positives (i.e. every type is T?, which they can be, in SQL. e.g. after left joining a non-nullable type, it becomes nullable again). This is never really wrong, but maybe inconvenient. Implemented in #6248
• [ ] An option is to generate false negatives only on records and/or POJOs (i.e. only nullable types are T?, the true positives, but non-nullable types, which are generated as T, would produce false negatives because they can still be null after an outer join, a union, etc.). But it would be a user choice, just like the !! operator. With this choice, TableField<R, T?> references would still always be nullable, only records and/or POJOs are affected, as they're mostly used with SELECT * projections of single tables (no joins)

Options that will not be implemented as of this issue but are left here for future reference:

• An option for generated code is to not declare the TableField type val F = createField(...). That would produce T! types, because createField() is a Java method. Users would still have to "cast" the type to one of Field<T> or Field<T?> in their own APIs, both are possible
• An option is to generate both Field<T> and Field<T?> to give the user the option of choosing, e.g. COL and COL_NULLABLE
• Another option is to generate false negatives everywhere, including on TableField<R, T>, which means the types can even be wrong after outer joins without record or POJO projection. This is quite undesireable but could be offered as an option.

This feature might be implemented separately from the KotlinGenerator in #6248

[KasperKiiskinen]

We would really like to at least Records to respect the NOT NULL constraint and be generated as T

[saibotma]

An option is to generate false negatives only on records and POJOs (i.e. only nullable types are T?, the true positives, but non-nullable types, which are generated as T, would produce false negatives because they can still be null after an outer join, a union, etc.). But it would be a user choice, just like the !! operator. With this choice, TableField references would still always be nullable, only records and POJOs are affected, as they're mostly used with SELECT * projections of single tables (no joins)

is the solution we require.

[lukaseder]

Thanks for your comment @saibotma. This will definitely be included in jOOQ 3.15. While I have to repeat my disclaimers :), I definitely see how this can be very useful as a pragmatic 80/20 solution.

[vojtapol]

@lukaseder When you mention the false positive in case of left join I assume you mean something like:

create.select()
  .from(BOOK)
  .leftJoin(AUTHOR).onKey()
  .where(BOOK.ID.eq(42))
  .fetchOneInto(AUTHOR)

Right? In case the book has no known author, the AuthorRecord would have all fields set to null which would lead to a false positive for fields that are marked as not null.

In my opinion though the call should not result in an AuthorRecord with false positives. Instead, the projection to AUTHOR should throw an exception. For a join like this, I would personally instead use the generic Record type and later project it into AUTHOR only in case where I know I am getting the full AUTHOR record:

var record = create.select()
  .from(BOOK)
  .leftJoin(AUTHOR).onKey()
  .where(BOOK.ID.eq(42))
  .fetchOne();

if (record.get(AUTHOR.ID) != null) {
  var author = record.into(AUTHOR);
}

[lukaseder]

When you mention the false positive in case of left join I assume you mean something like [...]

Yes.

Instead, the projection to AUTHOR should throw an exception

No. (If only I had 0.02$ for every time I'm explaining these things 😁)

1. First, you'll get your exception, just a bit later. You won't gain that much by making it more explicit.
2. Should we really waste all these CPU cycles to reverse engineer how a random flat Result came to be? Results would have to keep a reference to the query that produced them, and we'd have to analyse it too.
3. And what if we don't have all the information about the producing query? E.g. when we use plain SQL templating, we might not be able to access nullability information. The behaviour would again be different, and thus be very confusing.

4. What makes you think it's so obvious to recognise "bad queries"? I invite you to do the exercise of going through the entire jOOQ API and assessing for each and every operator whether it can tamper with the nullability of an expression (hint: I think I can spend my time more wisely). Some examples, what about these queries?

   create.select()
     .from(BOOK)
     .fullJoin(AUTHOR).onKey()
     .fetchInto(AUTHOR);
   

   Or this one?

   create.select(val(null).as(AUTHOR.ID))
     .fetchOneInto(AUTHOR)
   

Who is jOOQ to judge anyone about their intentions? In SQL, I can have a structurally typed record that looks like a nominally typed record (e.g. that of a table). I can cast them back and forth (even in SQL, e.g. PostgreSQL PL/pgSQL and Oracle PL/SQL offer such tools), and pretend they're the same thing, irrespective of any constraints. At this point, it becomes once more clear that in SQL, nullability is not a type modifier but a constraint (which can even be deferred!). It works differently from what most of you wish it did, which is why this bike shed is being discussed so often time and again on all the channels.

The only way to get all of this right would be via an external DSL (i.e. native SQL), which allows for keeping track of nullability via the usual means a language spec and compiler can offer. In an internal DSL, this seems impossible, even with much more sophisticated languages than Java or Kotlin. (More on that below)

I've seen this fail many times in other libraries that eagerly and cleverly implemented some kotlin T? vs T or Scala Option[T] vs T magic only to compromise (a lot!) on SQL functionality (besides, even simple SQL queries become extremely hard to write in those APIs because of the cognitive friction this "type safety" introduces, let alone hard to compile). jOOQ does not make such a compromise. When it comes to this null bikeshed vs. completeness of vision in terms of SQL support, jOOQ will always choose SQL.

Yes, 80% of developers write boring CRUD operations where super fancy SQL does not matter. But jOOQ has been made for the 20% as well. And even if you're part of those 80%, you might occasionally want a NOT NULL DEFAULT CURRENT_TIMESTAMP modifier on your column, and then what?

In my opinion though the call should not result in an AuthorRecord with false positives

As always, when someone wants an improvement deep down in feature X, what they may have actually wanted is feature Y (https://xyproblem.info). You don't really want AuthorRecord to be ridden of false positives in your specific examples. What you want is an alternative jOOQ that cannot produce AuthorRecord with false positives in the first place.

Maybe we can offer this in the future, see https://github.com/jOOQ/jOOQ/issues/11049, https://github.com/jOOQ/jOOQ/issues/11054, but it will not work with the internal DSL. Perhaps, in the future, we may be able to offer an API that parses SQL and reflects on it (at compile time) to produce the correct nullability information for your records and data classes per query.

This issue here will not discuss that, and it will definitely ship with the offered compromise of occasionally getting these false positives. I mean, heck, just call create.newRecord(AUTHOR) and boom there you have it. False positives wherever you look!

[sgrimm]

An option is to generate false negatives only on records and POJOs

Do POJOs and records need to solve this the same way? That seems to be kind of implied in some of the discussion but perhaps it's okay for POJOs to have a different approach to nullability than records, or at least the option of a different approach. For my use cases, I'd be happy if POJOs had non-nullable fields for NOT NULL columns and records had nullable ones.

[lukaseder]

@sgrimm It would be an option, yes, with the usual caveat of probably breaking the <interfaces/> feature: https://github.com/jOOQ/jOOQ/issues/10509

[pauliusrum]

We currently use java generator + annotations to achieve nullability in Kotlin. Kotlin compiler takes into consideration java annotations (Nonnull, nullable) and compiles into nullable or non nullable types accordingly.

I'm kinda confused why java generator + annotations are not interchangeable with kotlin generator. I understand the left join reasoning but does it not apply to java generator + annotations the same way as it does to kotlin generator?

[lukaseder]

@pauliusrum Do you think those annotations should also be produced in kotlin code? Would that be idiomatic?

[pauliusrum]

@pauliusrum Do you think those annotations should also be produced in kotlin code? Would that be idiomatic?

Adding annotations to kotlin code does not have the same effect as with java. The compiler ignores the annotations when they are added to kotlin code.

data class Foo(
    @Nonnull val bar: String?
)

fun main() {
    val foo = Foo(null) // even if the annotation is applied the compiler allows null value
    val bar: String = foo.bar // compilation error since the type is nullable
}

[lukaseder]

So, what are you trying to say, then?

The JavaGenerator behaviour defaults to generating T! types, which are the best default for jOOQ, IMO. But T! types are not denotable in kotlin, so the status quo is to default to T? types, which aren't wrong, but not user friendly.

Yes, this needs to be improved. But differently, not like the Java annotations...

[pauliusrum]

Yes, this needs to be improved. But differently, not like the Java annotations..

Agree. I was just wondering why java annotations were introduced. What problems did they solve? The same ones that we are discussing in this issues or something else?

[lukaseder]

Agree. I was just wondering why java annotations were introduced.

Well, this bikeshed can be painted in any language! 😉

[sgrimm]

@sgrimm It would be an option, yes, with the usual caveat of probably breaking the <interfaces/> feature: #10509

Not sure if this totally addresses that concern, but nullable interface methods/properties can be implemented as non-nullable. For example:

interface NullableInterface {
  val property: String?
}

data class NonNullableImplementation(
  override val property: String
) : NullableInterface

val example = NonNullableImplementation("x")

// Property is of type String when accessed on implementation
assert(example.property.length == 1)

// Property is of type String? when accessed using interface
assert((example as NullableInterface).property?.length == 1)

So if I understand the problem correctly, the properties in the interface would be nullable, would be implemented as nullable in records as they are today, but would be non-nullable in POJOs.

[lukaseder]

@sgrimm Since T <: T?, I can see how covariant overloading a val covariantly can work. But we generate var, not val since it is desirable to be able to set values on interfaces and records, perhaps a bit less so on data classes, which may be immutable. There is no possibility of a covariant override of a val property, because of the setters. You can try it in your code. This doesn't work:

interface NullableInterface {
  var property: String?
}

data class NonNullableImplementation(
  override var property: String
) : NullableInterface

We could, of course, consider whether we generate <immutablePojos/> and <immutableInterfaces/>, but this does start to get really complicated, then...

[JohannesZick]

I was also thinking of this feature. I'm currently using the Kotlin generator, and I write a lot of !! after fetching data.

I see a big difference though between Records and POJOs (POKOs?). Records track what has been set, so if you try to read an unset field that should be non-null... what do you expect to happen? I don't think exceptions in these cases make anything easier to use. POJOs are different because they don't have this distinction. By making fields non-null, you loose the ability to construct incomplete POJOs, e.g. for inserts. But IMHO in that case you should use Records to mark which columns should be touched at all in the query.

Even in a POJO you might not have an id though. So, maybe columns that are marked as GENERATE ALWAYS or similar need to be nullable anyway? As much as I want this, it seems like a slippery slope into many many corner cases.

[lukaseder]

As much as I want this, it seems like a slippery slope into many many corner cases.

My words :) I'm convinced most people in this discussion don't actually want what they think they want.

[JohannesZick]

OK, so what I think could work without too much confusion, but also limitations:

An option to make all POJO fields non-null using the same information as used for annotations. Not on Records, and on all fields for POJOs.

This limits usability of POJOs. You cannot easily use them for inserts or updates because generated values (e.g. ids) have to be set in the constructor. But you should really use Records there IMHO, exactly to make the distinction between "set to null" and "set to generated value".

The use case mentioned before with JOINs is I think even less relevant. These POJOs are meant to represent exactly one row in exactly one table. When you create a new table by joining, the POJO will not match and you should run your own.