Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JPA + custom type + Number + NumberExpression throws IllegalArgument Unsupported target type #261

Open
HonoluluHenk opened this issue Jan 21, 2024 · 9 comments

Comments

@HonoluluHenk
Copy link

Observed vs. expected behavior

(This is a continuation from the original QueryDSL issue #3550)

When using the following Combination

  • a custom type extending Number (imho this seems to trigger the strange behavior)
  • a custom @Converter/AttributeConverter (also happens with all other Hibernate custom types like UserType)
  • a NumberExpression in the JPAQuery: sumAggregate() (formerly: sum())

Example:

var result = new JPAQuery<>(em)
		.select(myEntity.myCustomNumber.sumAggregate())
		.from(myEntity)
		.fetchOne();

Actual outcome:

java.lang.IllegalArgumentException: Unsupported target type : MyCustomNumber is thrown instead of a valid result.

Expected outcome:

The query-result.

Steps to reproduce

I created a reproducer with unit tests showing what works and what doesn't:
https://github.com/dvbern/bug-reproducers-querydsl-jpa-hibernate6/tree/openfeign

Environment

Querydsl version: 6.0.0.RC1

Querydsl module: querydsl-jpa

Database: any (e.g. hsqldb, postgresql)

JDK: 17 (but the stacktrace indicates this is not a JDK-problem)

@HonoluluHenk
Copy link
Author

Digging around the code:

Conversions.java around line 64:

  private static boolean needsNumberConversion(Expression<?> expr) {
    expr = ExpressionUtils.extract(expr);
    return Number.class.isAssignableFrom(expr.getType()) && !Path.class.isInstance(expr);
  }

If a value needs to be treated as Number is determined solely if it is assignable to Number.

if needsNumberConversion() returns true, com.querydsl.core.util.MathUtils#cast is called lateron. The big if-else over all JDK-Number-subtypes runs into the else branch because my custom type implements Number but is of none of the listed types.

Possible fix: needsNumberConversion should check all the same types listed in MathUtils#cast, not just if the Number-assignment works.

I cannot try this out since I cannot get QueryDSL to compile on my machine, any hints appreciated :)

@ft0907
Copy link

ft0907 commented Jan 26, 2024

I would like to ask if they have not been published to the maven library and can only be introduced into the project in the form of manual jars.

@danielvion
Copy link
Collaborator

I think this is an issue with the way hibernate 6 is doing the type mappings. It used to be that sum() always returned the same type of NumberPath as the summed path. This is no longer possible with hibernate 6 since they have added some mapping for the sum function. So longs and ints are mapped to longs and doubles, BigDecimals, Floats to BigDecimals and so on.

The custom implementation may need some extra work to get it to work properly.

@danielvion
Copy link
Collaborator

@HonoluluHenk why is it not compiling for you? Did you add the toolchain.xml file to your .m2 folder? That is used for building querydsl using different java versions. I just use sdkman for installing and managing my jdks.

<?xml version="1.0" encoding="UTF-8"?>
<toolchains>
  <!-- JDK toolchains -->
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>1.8</version>
    </provides>
    <configuration>
      <jdkHome>$PATH_TO_JDK_1.8</jdkHome>
    </configuration>
  </toolchain>
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>11</version>
    </provides>
    <configuration>
      <jdkHome>$PATH_TO_JDK_11</jdkHome>
    </configuration>
  </toolchain>
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>21</version>
    </provides>
    <configuration>
      <jdkHome>$PATH_TO_JDK_21</jdkHome>
    </configuration>
  </toolchain>
    <toolchain>
    <type>jdk</type>
    <provides>
      <version>17</version>
    </provides>
    <configuration>
      <jdkHome>$PATH_TO_JDK_17</jdkHome>
    </configuration>
  </toolchain>
</toolchains>

Also I build using java 21.

@HonoluluHenk
Copy link
Author

Thanks for the hint with toolchain.xml, I can build now!

After you comment on hibernate 6 not returning the specific datatype anymore, I digged into to JPA spec and found:
Hibernate 6 got more JPA compliant and this is actually expected behavior.

See JPA 3.1 chapter 4.8.5 Aggregate Functions in the SELECT Clause and also JPA 2.2, same chapter:

SUM returns Long when applied to state fields of integral types (other than BigInteger); Dou-
ble when applied to state fields of floating point types; BigInteger when applied to state fields
of type BigInteger; and BigDecimal when applied to state fields of type BigDecimal.

@danielvion
Copy link
Collaborator

Hence the changes with sum(). I am not really happy with the solution though. I kept thinking about it and I think it would have been better to have some subclasses of NumberPath like LongPath which retun the proper typed path when calling the sum method.

@HonoluluHenk
Copy link
Author

For those arriving via google and being in a hurry: here's a workaround:

// create a custom, re-usable QueryDSL wrapper:
static class TypeWrapper<DBType, MyType> implements com.querydsl.core.types.FactoryExpression<MyType> {
    private final Class<MyType> valueClass;
    private final Function<DBType, MyType> factory;
    private final List<Expression<?>> args;

    public TypeWrapper(Expression<DBType> arg, Class<MyType> valueClass, Function<DBType, MyType> factory) {
        this.valueClass = valueClass;
        this.factory = factory;
        this.args = List.of(arg);
    }

    @Override
    public <R, C> @Nullable R accept(Visitor<R, C> v, @Nullable C context) {
        return v.visit(this, context);
    }

    @Override
    public Class<? extends MyType> getType() {
        return valueClass;
    }

    @Override
    public List<Expression<?>> getArgs() {
        return args;
    }

    @Override
    @Nullable
    public MyType newInstance(Object... args) {
        @SuppressWarnings("unchecked")
        DBType arg = (DBType) args[0];
        return factory.apply(arg);
    }
}

// ... and use it like this:
@Test
void query_with_NumberExpression_and_aggregate() {
    MyCustomNumber actual = new JPAQuery<>(em)
            .select(new TypeWrapper<>(
                    myEntity.myCustomNumber.sumBigDecimal(),
                    MyCustomNumber.class,
                    MyCustomNumber::new
            ))
            .from(myEntity)
            .fetchOne();

    // instead throws:
    // java.lang.IllegalArgumentException: Unsupported target type : MyCustomNumber

    assertThat(actual)
            .isInstanceOf(MyCustomNumber.class)
            .isEqualTo(MY_CUSTOM_NUMBER);
}

@HonoluluHenk
Copy link
Author

Just got an idea: why not add this function to NumberExpression - or any Expression for that matter?

public <MyType> TypeWrapper<T, MyType> wrapValue(Class<MyType> myTypeClass, Function<T, MyType> factory) {
    return new TypeWrapper<>(this, myTypeClass, factory);
}

Not the most elegant but this could be a nice little typesafe but convent way for all Projections on value types.

@nikospara
Copy link

Hello!

I did some digging for this issue. Wrote a relevant test, where I added a type Amount, identical to MyCustomNumber, and the field Employee.salary to com.querydsl.jpa.domain.Employee. Attaching the stack trace to facilitate the discussion:

java.lang.IllegalArgumentException: Unsupported target type : Amount

	at com.querydsl.core.util.MathUtils.cast(MathUtils.java:85)
	at com.querydsl.core.support.NumberConversion.newInstance(NumberConversion.java:57)
	at com.querydsl.jpa.FactoryExpressionTransformer.transformTuple(FactoryExpressionTransformer.java:48)
	at org.hibernate.sql.results.internal.RowTransformerTupleTransformerAdapter.transformRow(RowTransformerTupleTransformerAdapter.java:30)
	at org.hibernate.sql.results.internal.StandardRowReader.readRow(StandardRowReader.java:102)
	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:205)
	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:33)
	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.doExecuteQuery(JdbcSelectExecutorStandardImpl.java:211)
	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.executeQuery(JdbcSelectExecutorStandardImpl.java:83)
	at org.hibernate.sql.exec.spi.JdbcSelectExecutor.list(JdbcSelectExecutor.java:76)
	at org.hibernate.sql.exec.spi.JdbcSelectExecutor.list(JdbcSelectExecutor.java:65)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.lambda$new$2(ConcreteSqmSelectQueryPlan.java:139)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.withCacheableSqmInterpretation(ConcreteSqmSelectQueryPlan.java:382)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.performList(ConcreteSqmSelectQueryPlan.java:302)
	at org.hibernate.query.sqm.internal.QuerySqmImpl.doList(QuerySqmImpl.java:526)
	at org.hibernate.query.spi.AbstractSelectionQuery.list(AbstractSelectionQuery.java:423)
	at org.hibernate.query.spi.AbstractSelectionQuery.getSingleResult(AbstractSelectionQuery.java:555)
	at com.querydsl.jpa.impl.AbstractJPAQuery.getSingleResult(AbstractJPAQuery.java:218)
	at com.querydsl.jpa.impl.AbstractJPAQuery.fetchOne(AbstractJPAQuery.java:336)
	at com.querydsl.core.support.FetchableQueryBase.fetchFirst(FetchableQueryBase.java:48)
	[...]
	at com.querydsl.jpa.AbstractJPATest.sum_custom_number(AbstractJPATest.java:2089)

The problem is, what would be the best way to approach it? I can think of a few options:

Have QueryDSL understand the JPA converters infrastructure

In this case QueryDSL would apply MyCustomNumberConverter.convertToEntityAttribute() to the BigDecimal result of the query. From the stack trace it seems that NumberConversion or MathUtils.cast would be good candidates to implement this. The problem here is how to discover the appropriate JPA converter from within QueryDSL? The case demonstrated in this issue is easy - the annotation processor can read the @Convert on the field. But what if the converter is autoApply? Or if it is defined in the XML configuration? I do not know any way to query the JPA implementation of all available converters, so for this to be complete, we would have to re-implement the converter discovery specification of JPA!

One alternative would be to limit ourselves to reading any @Convert on the field, other methods are not supported, sorry! This seems like an honest compromise.

Manually provide QueryDSL with information about which converter to use per case

In this case we may want to tweak the NumberExpression. What would be the best way? The simplest would be to add a series of methods accepting a converter, something like public <N extends Number> NumberExpression<T> sumAggregate(AttributeConverter<T,N>). This would not protect the developer from accidentally invoking the plain sumAggregate() and ending up with a runtime error.

There may be a way to enforce compile-time safety, but I am not experience enough in the inner workings of QueryDSL to even think of a way. If people think this is the best way to go, I would investigate.

This solution looks a lot like the TypeWrapper proposed by HonoluluHenk, but without introducing a new type (TypeWrapper is essentially equivalent to the AttributeConverter).

Allow the developer to override/extend the mapping logic of QueryDSL

At least for the case of a type that extends Number, we could implement a mechanism to allow the developer to override/extend MathUtils.cast globally.

A much more generic mechanism of mapping types; or extending the QueryDSL metamodel

I imagine that the same could happen if I want to persist e.g. an Email value type. This is essentially a string with more semantics attached to it; you can apply all the operations from StringPath and superclasses, but QueryDSL would not recognize it as a string (I guess; haven't tried!!!)

Maybe it would be worth investigating if the QueryDSL metamodel could be made plugable.

What else???

Ideas???


This is just a bunch of ideas, which may be irrelevant in the end; or may be good but not worth the effort in the end; or the outcome of this conversation is that it is a bad practice to let the persistence model know of custom value classes(*). I am investigating QueryDSL for my current project and I am happy to have found that there is an effort to keep it alive. Thank you @velo and team, I hope I will be able to contribute my own penny.

(*) Until now I have applied this rule of thumb for my entities: use a set of rich value classes in the model (MyNumber, Amount, Percentage, Email, etc) but keep the simple types in JPA (Number, String).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants