Generic type signature of FullTextQuery should not be as flexible as the JPA Query

Description

In its current format we shouldn't suggest that the return type matches the types passed. Make sure to return a Query<T> whose T is Object or at best{{Object[]}} if projections are enabled. Keep in mind that in ORM a projection of one single element might actually not return an array but the instance directly.

See https://hibernate.atlassian.net/browse/HSEARCH-2225#icft=HSEARCH-2225 and https://hibernate.atlassian.net/browse/HSEARCH-1788#icft=HSEARCH-1788

Activity

Yoann RodièreFebruary 8, 2017 at 11:36 AM
Edited

Any opinion on this?

To sum up, the choice is:

  1. either we make createFullTextQuery return a FullTextQuery<?> or FullTextQuery<Object>, but break existing code that used non-raw lists of results.

  2. or we leave it as is (FullTextQuery is a generic type, but we only ever return raw instances)

  3. or we remove the generic parameter from FullTextQuery and make it extend Query instead of Query<T> (extend the raw type)

Given that we can't return a properly parameterized FullTextQuery anyway, I'd say we should just go for solution 3, and introduce generics in a later version (probably 6).

Yoann RodièreFebruary 1, 2017 at 8:40 AM

Here is an example of a failing build, with the compiler screaming at our pre-generic tests: https://travis-ci.org/yrodiere/hibernate-search/builds/196650030

So, maybe we should cancel this ticket?

Yoann RodièreFebruary 1, 2017 at 8:38 AM
Edited

When you mention "even in our tests, this translates into more than 90 compilation errors" that's not comparing to 5.6 codebase right? Or are we potentially breaking code of pre-generic Query usage as well?

We are potentially breaking code of pre-generic FullTextQuery usage as well.

The reason is simple: before 5.7, FullTextQuery wasn't generic, so we could write this kind of code:

List<Foo> result = fullTextSession.createFullTextQuery( luceneQuery ).list();

We'd get a warning ("unchecked cast to..."), but since List is convertible to List<Foo> automatically, this would compile just fine.
In 5.7.0.CR1 (and earlier) we simply changed FullTextQuery to make it generic (FullTextQuery<T>), and changed FullTextQuery.list() so that it returned a List<T>. But createFullTextQuery still returned a non-parameterized (raw) FullTextQuery, and users only manipulated raw FullTextQuery s in their code, so this was mostly a non-breaking change (FullTextQuery.list would still return a raw List).
But if we make createFullTextQuery return a FullTextQuery<?> or FullTextQuery<Object>, now we have a problem, because the returned result of type List<?> or List<Object cannot be converted to List<Foo>.

I see no big value in adding a createTypedFullTextQuery at this stage; we can consider such things for 5.8 .. but as you say, it might be short-lived, unless we want to already flesh out the "right one", which will then become the only one in 6+. That would be ideal, and somewhat help the users eventually migrate to 6.

Yeah, the ideal would be to get it right immediately, but if we want to make these methods return query builders as we discussed last week, it isn't possible. It's simply too much change.
So, yes, let's keep it simple for now.

Sanne GrinoveroJanuary 31, 2017 at 6:47 PM

Looks like we'll need a 5.7.0.CR2 to make sure we get such things right. Other open issues are also more invasive than expected, so I expected one anyway.

I'm not concerned with breaking APIs with development releases, like the previous 5.7.x. The backwards compatibility "promise" should apply to people migrating from 5.6 to 5.7.

When you mention "even in our tests, this translates into more than 90 compilation errors" that's not comparing to 5.6 codebase right? Or are we potentially breaking code of pre-generic Query usage as well?

I see no big value in adding a createTypedFullTextQuery at this stage; we can consider such things for 5.8 .. but as you say, it might be short-lived, unless we want to already flesh out the "right one", which will then become the only one in 6+. That would be ideal, and somewhat help the users eventually migrate to 6.

Yoann RodièreJanuary 30, 2017 at 5:56 PM

Actually, this would be a breaking change...

In 5.7.0.CR1, one can write this kind of code:

List<Foo> result = fullTextSession.createFullTextQuery( luceneQuery ).list();

Or this:

List<Foo> result = fullTextSession.createFullTextQuery( luceneQuery, Foo.class ).list();

Or even this:

FullTextQuery<Foo> query = fullTextSession.createFullTextQuery( luceneQuery, Foo.class );

All of which will work thanks to the FullTextQuery returned by fullTextSession being a raw type. Even if there will be compiler warning.

With our change, the snippets of code above will give users a compilation error due to incompatible types. Users will have to adapt their code and add explicit casts... (even in our tests, this translates into more than 90 compilation errors)

To sum up the problem, we can't fix the "mutable return type" issue without breaking the API: we'd need to remove the setProjection method. So we definitely cannot make createFullTextQuery return a typed (<T>) query. But adding <Object> or <?> to the returned FullTextQuery type will break existing user code, including code compatible with 5.7.0.CR1.

So the two solutions I see are:

1. Leave createFullTextQuery() as is, returning a raw FullTextQuery.
2. Add <Object> or <?> to the returned FullTextQuery type, but release a CR2 or somehow warn users.

Also, maybe we should introduce other methods, beside createFullTextQuery(), to support typed queries? E.g. createTypedFullTextQuery? Then it'd be up to the user to provide the correct type, and we could add some early runtime checks to ensure setProjection/setResultTransformer are used properly. It'd be a shame to introduce new API so late in the release cycle, though, particularly considering that these APIs will have to go in HSearch 6.

By the way, the current situation in ORM isn't so bright as I understood from Steve's explanations: there is currently (in ORM 5.2) no replacement for the deprecated setResultTransformer method, so anyone wanting to transform results will have to use this non-typesafe API...

WDYT?

Fixed

Details

Assignee

Reporter

Fix versions

Priority

Created January 27, 2017 at 2:28 PM
Updated February 21, 2017 at 8:47 PM
Resolved February 20, 2017 at 2:12 PM

Flag notifications