Binding parameters to Query / ProcedureCall

[sebersole]

Query has a lot of methods to bind parameters. I propose we clean these up.

https://hibernate.atlassian.net/browse/HHH-15017

Type determination

There are multiple overloads with regards to influencing the "binding type" which should be used to bind these values to JDBC - ultimately resolved to org.hibernate.metamodel.mapping.Bindable behind the scenes.

So the first proposal has to do with what kind of things we want to accept for influencing the binding-type.

We currently have overloads accepting AllowableParameterType as well as BasicTypeReference. We should consolidate these into a single overload via inheritance. AllowableParameterType allows for embeddables, whereas BasicTypeReference can only be used for basic-valued types.

There are obviously many ways to achieve such consolidation...

1. Redefine AllowableParameterType
2. BasicTypeReference implement AllowableParameterType
3. Expose ways to create AllowableParameterType references, including embeddables

Something like:

/** No longer extends SimpleDomainType */
interface AllowableParameterType<T> {
    Class<T> getJavaType();

    static <T> AllowableParameterType<? extends T> parameterType(Class<T> type) { ... }

    static <T> AllowableParameterType<? extends T> parameterType(Class<?> javaType, AttributeConverter<T,?> converter) { ... }

    static <T> AllowableParameterType<? extends T> parameterType(Class<?> javaType, Class<? extends AttributeConverter<T,?>> converter) { ... }

    static <T> AllowableParameterType<? extends T> parameterType(ManagedType<T> managedType) { ... }

    static <T> AllowableParameterType<? extends T> parameterType(jakarta.persistence.metamodel.Bindable<T> jpaBindable) { ... }

    static <T> AllowableParameterType<? extends T> parameterType(org.hibernate.metamodel.mapping.Bindable bindable) { ... }

    ...
}

class BasicTypeReference<T> implements AllowableParameterType<T> {
    // as-is
    ...
}

/**
 * SPI form of AllowableParameterType providing resolution to a (Hibernate) Bindable
 */
public interface ResolvableAllowableParameterType<T> extends AllowableParameterType<T> {
	SqmExpressable<T> resolveExpressable(SessionFactoryImplementor sessionFactory);
}

Query.setProperties

Do people actually use this? I have personally never used it, and have not seen any bug/enhancement requests related to it in years. Should we just drop this?

[gavinking]

Something like:

This looks pretty good to me. It:

• consolidates two overloads into one, and
• gives users a way to obtain an AllowableParameterType.

which resolves my objections nicely.

OTOH, why did you get rid of its type parameter? I would have kept it, I think, for a teensy bit of type safety.

Should we just drop this?

I don't really care if it lives or dies. The only think I would say in its defense is that now with Map.of(), it can be a pretty abbreviated way to bind multiple named parameters.

[sebersole]

It's just a markdown ;)

Yes, I'd keep the generic type

[gavinking]

Perfect :-)

[sebersole]

Changed it above to what I actually have in the code...

[sebersole]

Something like:

This looks pretty good to me. It:

* consolidates two overloads into one, and

* gives users a way to obtain an `AllowableParameterType`.

which resolves my objections nicely.

OTOH, why did you get rid of its type parameter? I would have kept it, I think, for a teensy bit of type safety.

Should we just drop this?

I don't really care if it lives or dies. The only think I would say in its defense is that now with Map.of(), it can be a pretty abbreviated way to bind multiple named parameters.

👍

[gavinking]

One more comment. I'm not too keen on calling these methods from(). Because if I use a static import I get a method with a too-generic name. Or if I don't, I get the whole mess of AllowableParameterType.from(). Perhaps that's not a big deal because we think users hardly ever need this, but worth considering.

[sebersole]

Names... 🤷

[gavinking]

Well, of course.

[sebersole]

Currently we support binding an Object-valued value with a TemporalType. Initially in this work, I dropped that form. However, that led to interesting discovery in fixing up tests. Specifically, there is a test that maps a Instant and then wants to perform a between predicate based on the date portions. Previously it did this...

query.setParameter( "start", Instant.now().minus( 7, ChronoUnit.DAYS ), TemporalType.DATE );
query.setParameter( "end", Instant.now().plus( 7, ChronoUnit.DAYS ), TemporalType.DATE );

However, that no longer matches any setParameter signature.

• Just dropping the temporal precision (using the full Instant) won't work as it changes the results.
• Passing in a more date-centric type (Date, LocalDate, etc) does not work either because the types do not "match" and validation fails

Long story short, I cannot see a way to make this work from a user perspective in this not unusual situation.

I can see a few options...

1. Add setParameter overrides accepting Instant and TemporalType
2. Change bind value validation to better handle "treatable" situations (we can "treat" a Date as if it were an Instant)

And of course, we could do both. (1) is pretty straight-forward. Yes, TemporalType does explicitly mention just Date and Calendar but imo this fits the same pattern for parameter binding. JPA uses TemporalType for mapping as well (@Temporal) which makes no sense for Instant or any of the java.time types. But imo TemporalType means something slightly more broad when used for parameter bindings - namely what part of the temporal to use

[gavinking]

Well conceptually this is actually the affect TemporalType has for Query parameters.

Right, sure, to clarify: when there's an explicit TemporalType, that's logically equivalent to an explicit typecast, and narrowing is just peaches. I'm not suggesting that should change.

What I was wondering about was the case where there's no explicit TemporalType, which implicit conversions should we accept?

[gavinking]

So why are we setting it?

So this is a good question.

• So, I mean, one case is where we have a Java type as an argument whose mapping to JDBC we can't guess—an enum value, or something with a converter, for example—and we can infer the type of the query parameter from one side of an operator or whatever. And that way we know how to bind the argument to JDBC. Perfect. That's something we want, I think.
• But in other cases we can guess the JDBC type perfectly well from the type of the Java object we're given as an argument. And in those case we're currently treating it as a typing error if the type we're inferring and the type of the argument don't align. And I wonder how useful that is, in general.

Now, it's not useless. We can protect the user from depending on unportable or dangerous implicit type conversion that happens in the database. And that seems to have some value. If we can protect people from writing a BigDecimal to a column of type INTEGER, it seems to me that we're probably doing them a favor.

But on the other hand, a typecheck that can only happen right before we actually send the SQL to the db isn't as useful as typechecks that can happen at startup time or at compile time or in the IDE or whatever. And let's face it: in a good majority of cases the db will actually figure out something fairly sensible to do with a value that isn't of exactly the right type.

So, that's just a long-winded way of saying I think the argument for being very fastidious about typechecking argument-parameter assignments at runtime is not strong, and we should err on the side of being a bit forgiving.

[sebersole]

What I was wondering about was the case where there's no explicit TemporalType, which implicit conversions should we accept?

Right, these are not well defined at the moment, though some do work. But it would be better to be explicit here in terms of documented behavior.

So let's talk about the larger picture here. Temporals are just an illustration of the overall problem.

Certainly widening conversions should be allowed. And even certain narrowing conversions imo.

A lot of this, I think, is actually already handled by the fact that we JavaType#coerce the bind value based on the anticipated-type or explicit type of the parameter. I think this coercion is a good start for this. In fact, at first glance, I wonder if validation here might (for the most part) be a simple matter of whether the coercion works or not - provided we know or can deduce the type.

I do keep coming back to the problem though of setting the anticipated type too aggressively, which I think will help with much of this.

[sebersole]

So, that's just a long-winded way of saying I think the argument for being very fastidious about typechecking argument-parameter assignments at runtime is not strong, and we should err on the side of being a bit forgiving.

👍

a typecheck that can only happen right before we actually send the SQL to the db isn't as useful as typechecks that can happen at startup time or at compile time or in the IDE or whatever

So the problem obviously is that we don't know the bind values for parameters until they are bound. Unless you mean inspecting the calling code earlier prior to runtime, which Hibernate itself can obviously not do.

good majority of cases the db will actually figure out something fairly sensible to do with a value that isn't of exactly the right type

Well, we do as well with JavaType#coerce. And I think the portability aspect there is very worthwhile. Though I will agree that, as I said above, I am not sure we need validation per-se. Not to mention that code is fairly complex and would be nice to avoid, both from a maintenance and performance perspective.

[gavinking]

Yup, I agree with all that. I think the next step is to just write up the explicit list of things we think should work. (And document it, for sure.)