jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
196 stars 58 forks source link

Issue 238: Clarify mixing input parameter types for queries #341

Closed dazey3 closed 2 years ago

dazey3 commented 2 years ago

fixes #238

Signed-off-by: Will Dazey dazeydev.3@gmail.com

dazey3 commented 2 years ago

I decided to go with changing "undefined" with "invalid" because the EntityManager API for createQuery(String qlString) states the following:

    @throws IllegalArgumentException if the query string is found to be invalid

I tested current behavior with OpenJPA, EclipseLink, and Hibernate as well. Both OpenJPA/EclipseLink providers currently throw appropriate IllegalArgumentException from EntityManager.createQuery(...) calls when the query is parsed mixing input parameter types. Hibernate though doesn't fail and creates a query. This seems to violate the current Section 4.6.4: Input Parameters of the specification, but they aren't the reference implementation.

OpenJPA:

org.apache.openjpa.persistence.ArgumentException: Query "SELECT a FROM Story a WHERE a.name = :name AND a.id = ?1" Contains both named and positional parameters this is not allowed by the JPA specification. Detected parameters "[name, 1]".
    at org.apache.openjpa.kernel.jpql.JPQLExpressionBuilder.validateParameters(JPQLExpressionBuilder.java:2462)
...
    at org.apache.openjpa.persistence.EntityManagerImpl.createQuery(EntityManagerImpl.java:102)

NOTE: org.apache.openjpa.persistence.ArgumentException extends java.lang.IllegalArgumentException

EclipseLink:

java.lang.IllegalArgumentException: An exception occurred while creating a query in EntityManager: 
Exception Description: Syntax error parsing [SELECT a FROM Story a WHERE a.name = :name AND a.id = ?1]. 
[37, 42] Named and positional input parameters must not be mixed in a single query.
[54, 56] Named and positional input parameters must not be mixed in a single query.
    at org.eclipse.persistence.internal.jpa.EntityManagerImpl.createQuery(EntityManagerImpl.java:1620)
beikov commented 2 years ago

I don't know how you tested this with Hibernate, but there are compatibility configurations that you must set in order for Hibernate to behave fully spec compliant. I'm pretty sure that if you set those setting, Hibernate will throw an exception as well.

dazey3 commented 2 years ago

@beikov

There are compatibility configurations that you must set in order for Hibernate to behave fully spec compliant. I'm pretty sure that if you set those setting, Hibernate will throw an exception as well.

Hibernate is knowingly not spec compliant by default and you must instead set properties to be JPA spec compliant? I have not heard of this before. Mind sharing a link to the documentation for this?

dazey3 commented 2 years ago

Regardless, if all providers fail, that adds even more weight that the specification should in fact describe mixing of input parameter types as "invalid" rather than just "undefined"

beikov commented 2 years ago

Here are the various JPA compliance related configurations: https://docs.jboss.org/hibernate/orm/5.6/userguide/html_single/Hibernate_User_Guide.html#configurations-jpa-compliance

dazey3 commented 2 years ago

@beikov

Testing with the Hibernate "" property from the documentation you linked did result in Hibernate throwing an exception:

java.lang.IllegalArgumentException: org.hibernate.hql.internal.ast.QuerySyntaxException: Cannot mix positional and named parameters: SELECT a FROM model.Story a WHERE a.name = :name AND a.id = ?1 [SELECT a FROM model.Story a WHERE a.name = :name AND a.id = ?1]
    at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:138)

NOTE: Hibernate 5.2.18 did not change behavior, but 5.5.6 did. This issue must have been fixed at some point.

I understand this Hibernate discussion is a side note to this JPA Spec issue, but does Hibernate have the ability to process the property <property name="hibernate.jpa.compliance" value="true"/>? It would be convenient to simple say "I want to run with a JPA compliant provider requiring only 1 provider specific property".

sebersole commented 2 years ago

@beikov

There are compatibility configurations that you must set in order for Hibernate to behave fully spec compliant. I'm pretty sure that if you set those setting, Hibernate will throw an exception as well.

Hibernate is knowingly not spec compliant by default and you must instead set properties to be JPA spec compliant? I have not heard of this before. Mind sharing a link to the documentation for this?

Is it really that odd that a provider does things better by default? I mean really?

sebersole commented 2 years ago

I understand this Hibernate discussion is a side note to this JPA Spec issue, but does Hibernate have the ability to process the property <property name="hibernate.jpa.compliance" value="true"/>? It would be convenient to simple say "I want to run with a JPA compliant provider requiring only 1 provider specific property".

Yes, I just added this for 6.0 - https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/cfg/AvailableSettings.java#L2035-L2051

dazey3 commented 2 years ago

@beikov

There are compatibility configurations that you must set in order for Hibernate to behave fully spec compliant. I'm pretty sure that if you set those setting, Hibernate will throw an exception as well.

Hibernate is knowingly not spec compliant by default and you must instead set properties to be JPA spec compliant? I have not heard of this before. Mind sharing a link to the documentation for this?

Is it really that odd that a provider does things better by default? I mean really?

"Better" is a matter of opinion. Also, all I said was that I had not heard of this before for Hibernate and asked for a link to the documentation. Did I say something wrong?

sebersole commented 2 years ago

"Better" is a matter of opinion.

Fair enough. I'll let usage numbers tell the tale of what the general opinion is.

Hibernate existed for many years before JPA. In fact, most parts of JPA were influenced by Hibernate. So some of this is "legacy". Well one - hibernate.jpa.compliance.list is strictly a backwards compatibility setting. Hibernate not only predates JPA, it also predates annotations, and initially there was only XML mapping. Sometimes that XML bias still comes through. This List handling is one.

But honestly, for the most part Hibernate simply does do things better by default in these particular areas.

sebersole commented 2 years ago

And we really should not be having this conversation about Hibernate on the spec GitHub group. Feel free to ping us on Zulip, etc as outlined at https://hibernate.org/community/ if you want to continue discussing

lukasj commented 2 years ago

Is it really that odd that a provider does things better by default? I mean really?

could we leave this sort of comments away from this place and stick to topic, please? IMHO it doesn't matter what the provider does "by default" as long as it satisfies the TCK rules

Thank you.

sebersole commented 2 years ago

could we leave this sort of comments away from this place and stick to topic, please? IMHO it doesn't matter what the provider does "by default" as long as it satisfies the TCK rules

I was answering a direct question asked here. But,

And we really should not be having this conversation about Hibernate on the spec GitHub group...

dazey3 commented 2 years ago

@lukasj Thank you for the review