Jooq: Remove @Nullable annotation from ResultQuery.fetchSingleInto(Class) and Record.into(Class)

[masc3d]

Your question:

@Nullable has been added to many methods in 3.14, but in some places it seems misplaced eg. when mapping records by type:

    @Nullable
    <E> E into(Class<? extends E> type) throws MappingException;

consumers having to check for null on each into seems like unnecessary boilerplate.
should I create individual bugs for those?

Versions:

• jOOQ: 3.14

---

Duplicates:

• https://github.com/jOOQ/jOOQ/issues/10777

[lukaseder]

Thank you very much for your report.

This looks like a bug, yes, but regrettably it isn't. See the RecordMapper.map Javadoc, which governs this behaviour:

Returns:
The mapped value, which may be null in some implementations.

This leaks into all these xyzInto(Class) methods, because RecordMapper implementations are allowed to map a record to null. Specifically, you could do this:

List<Integer> list =
ctx.select(T.NULLABLE_COLUMN)
   .from(T)
   .fetchInto(Integer.class)

Bow some of these Integer values would possibly be null

[masc3d]

Specifically, you could do this

specifically yes @lukaseder.
but those specific cases will now "pollute" much more common not so specific usages.
how about @Nullable intoOne vs @NotNull into? arguably it's a breaking change, though..

[lukaseder]

but those specific cases will now "pollute" much more common not so specific usages.

We will definitely not lie about things! If something is nullable, then it must be @Nullable or not annotated at all...

how about @Nullable intoOne vs @NotNull into? arguably it's a breaking change, though..

We can collect (a lot) of user feedback about this new API and then maybe come to an informed decision. But currently, RecordMapper implementations are allowed to produce null values, and that has nothing to do with the many into() methods, which just delegate to DefaultRecordMapper. Offering different into() method won't fix the underlying issue, nor is this an area worth introducing breaking changes.

So, if this is an area producing enough user feedback, I'm happy to remove the annotation again - assuming you're using Kotlin, this would fall back to producing T! types, rather than T?, then. Maybe there are other solutions, too.

[axelfontaine]

I believe the suggestion was @Nullable fetchInto() vs @NotNull into() which seems entirely reasonable to me. No new methods, no new behavior, no breaking changes.

What am I missing?

[lukaseder]

These into() methods cannot be annotated @NotNull because they can produce null values because they really just delegate to DefaultRecordMapper.

[axelfontaine]

Ok, then let's go down the rabbit hole...

AbstractRecord.into(Class<? extends E> type)

delegates to

Tools.configuration(this).recordMapperProvider().provide((Fields) fields.fields, type).map(this)

which effectively resolves to

DefaultRecordMapper.map(R record) being invoked with an instance of AbstractRecord

From there on none of the potential delegates can produce null for a non-null instance of AbstractRecord and neither can the attach() method.

So again: What am I missing?

BTW: Every single other Record.into() methods is already annotated with @NotNull. This is the only one that isn't.

[masc3d]

yes I was referring to into methods, possibly intoNullable would be better naming.
since nullable overload would only be required for very few specific cases, it may be worth considering. but still breaking 🤷🏻‍♂️

alternatively in kt it would be

r.into(MyPojo::class.java)!!

or not much prettier

fun <R : Record, E> R.intoNotNull(type: Class<out E>): E = this.into(type)!!

r.intoNotNull(MyPojo::class.java)

[lukaseder]

@axelfontaine As I explained here, try this:

DSL.using(SQLDialect.DEFAULT)
    .newRecord(DSL.field("x", Integer.class))
    .values(null)
    .into(Integer.class) // Your Record.into(Class) method
    .byteValue(); // Boom

Boom:

Exception in thread "main" java.lang.NullPointerException
    at ...

BTW: Every single other Record.into() methods is already annotated with @NotNull. This is the only one that isn't.

• <E> E Record.into(E) assumes that <@NotNull E> applies (see Javadoc, it throws a NPE if the parameter is null)
• <R extends Record> R Record.into(Table<R>) returns a new Record, which is never null
• Record Record.into(Field<?>...) and similar also return a new Record, which is never null

The method are not exactly the same...

[lukaseder]

@masc3d yes I was referring to into methods, possibly intoNullable would be better naming.

I understand that this is very annoying for Kotlin users right now, and I'm inclined to remove the @Nullable annotation again, turning the returned E? type back to E!.

There will not be any solution that affects Java developers in such a significant way.

[masc3d]

yes, it seems like a good compromise. although Im already getting used to it ;

fun Attachable.configurationOrThrow(): Configuration = this.configuration()
    ?: throw IllegalStateException("missing configuration")

[lukaseder]

I'll implement this for jOOQ 3.15.0 and 3.14.1: The issue will remove @Nullable again from all RecordMapper affected methods. This will revert to 3.13 behaviour, which has been acceptable to Kotlin users for a long time.

There's no acceptable solution that I can see for this problem in the short run. We might eventually discuss, whether https://github.com/jOOQ/jOOQ/issues/3212 was a really good feature to add to DefaultRecordMapper. I personally like it very much. It's very convenient, time and again. But it needs to be weighed against other issues, this being one of them. In any case, it's been there for so long, it's probably very widely used.

Clearly, returning an E? type is not nice in Kotlin. But returning E (@NotNull) is just wrong. Luckily, we can return E!

[lukaseder]

although Im already getting used to it

Not sure what your code example means? You can get used to it, no problem. Because once we return E! you can still assign the values to E? types, if I'm not mistaken? This revert will not be source incompatible with 3.14.0, I'd say?

[masc3d]

no it's all good E! is perfectly fine compromise.

getting used to writing kt extensions for methods which rarely yield null that is, like Attachable.configuration().
but it's off-topic now and I won't complain about it, as it's not really frequently used.

[axelfontaine]

@axelfontaine As I explained here, try this:

DSL.using(SQLDialect.DEFAULT)
    .newRecord(DSL.field("x", Integer.class))
    .values(null)
    .into(Integer.class) // Your Record.into(Class) method
    .byteValue(); // Boom

Boom:

Exception in thread "main" java.lang.NullPointerException
  at ...

Ah! I see now. I wasn't aware of this case. I feel it is somewhat inconsistent with the rest of the API.

Whereas fetch().into() collapses to fetchInto(), here we have fetch().value1() collapsing into fetch().

One option to consider for the future could be to add the fetch() overloads for all currently supported value types, deprecate them and add a fetchValue() or fetchValue1() method instead.

[lukaseder]

@masc3d getting used to writing kt extensions for methods which rarely yield null that is, like Attachable.configuration().
but it's off-topic now and I won't complain about it, as it's not really frequently used.

I don't know, is it? You can still open up another issue if you like. I don't think I'm aware of an issue in this area, would love to know more.

[lukaseder]

@axelfontaine Ah! I see now. I wasn't aware of this case. I feel it is somewhat inconsistent with the rest of the API.

Perhaps, but this can be discussed separately, and for a far away major release.

Whereas fetch().into() collapses to fetchInto(), here we have fetch().value1() collapsing into fetch().

That's not it. fetch().value1() doesn't pass through a RecordMapper but produces the actual T1 data type. fetch(Integer.class) passes through RecordMapper and converts T1 to Class<E> using org.jooq.tools.Convert

One option to consider for the future could be to add the fetch() overloads for all currently supported value types, deprecate them and add a fetchValue() or fetchValue1() method instead.

The future is more composability, i.e. less overloads, not more, see: https://github.com/jOOQ/jOOQ/issues/9288

But to finish this, I think you two were very eager to "fix" the underlying problem, instead of just reverting the change of adding @Nullable to this particular set of methods. The underlying problem can't be fixed easily, and doesn't need to be addressed right now.

To give you additional context, adding @Nullable and @NotNull to a library after the fact will always be a compromise, and never be perfect. For example, no generic type variable or parameter has been annotated yet (https://github.com/jOOQ/jOOQ/issues/10780). I'm convinced that some types cannot be annotated reasonably for all cases without prior breaking changes in jOOQ. Each of these cases needs to be reviewed carefully.

Nevertheless, I think that 99.5% of all the annotations I've added in #6244 are going to be very useful to Kotlin users, so having 1-2 methods that are the exception to this rule will be OK.

[lukaseder]

Only two methods seem to be affected by this change:

• <E> E Record.into(Class<E>)
• <E> E ResultQuery<R>.fetchSingleInto(Class<E>)

All the other related methods either:

• Produce a List<E>, which is not annotated yet (see #10780)
• Can produce null also because there is no record, e.g. <E> E ResultQuery<R>.fetchOneInto(Class<E>), or <E> E ResultQuery<R>.fetchAnyInto(Class<E>), or <E> E Cursor<R>.fetchNextInto(Class<E>), and many more

[lukaseder]

Fixed in jOOQ 3.15.0. Backport scheduled for 3.14.1 (#10781)