jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
191 stars 55 forks source link

Wrong typed comparison expressions in TCK #391

Open beikov opened 1 year ago

beikov commented 1 year ago

src/com/sun/ts/tests/jpa/core/query/apitests/Client#queryAPITest27, src/com/sun/ts/tests/jpa/core/query/apitests/Client#queryAPITest29 and src/com/sun/ts/tests/jpa/se/entityManager/Client#typedQueryMethodsAfterClose16Test assume that comparison expressions A = B can be constructed, where A and B are not of compatible types.

queryAPITest27 compares a date against a string e.hireDate = '2000-02-14', queryAPITest29 compares a timestamp against a string d.tsData = '2006-11-11 10:10:10'. JPA specifies the JDBC escape syntax for such literals and should IMO use that in the TCK to allow providers do strict type validation.

typedQueryMethodsAfterClose16Test compares a string against an integer expression, but the test is only aiming at verifying the exception that is thrown from a query after the associated entity manager has been closed. The typing of the test should simply be fixed.

scottmarlow commented 1 year ago

Could one of the Persistence project committers please add challenge label to this issue.

scottmarlow commented 1 year ago

Also, if the TCK challenge is accepted, please add the accepted label as well.

lukasj commented 1 year ago

queryAPITest27/queryAPITest29 JPA specifies the JDBC escape syntax for such literals and should IMO use that in the TCK to allow providers do strict type validation.

Given The JDBC escape syntax may be used for the specification of date, time, and timestamp literals. in the spec, removal or change of existing tests looks wrong to me. It is OK to add new tests covering the usage of the JDBC escape syntax

typedQueryMethodsAfterClose16Test

OK

beikov commented 1 year ago

queryAPITest27/queryAPITest29 JPA specifies the JDBC escape syntax for such literals and should IMO use that in the TCK to allow providers do strict type validation.

removal or change of existing tests looks wrong to me

Don't you think that comparing a temporal path expression against a string literal is wrong? There is no mention in the spec about the expected format of the string literal. It was only by chance that databases against which tests are being run happen to support the format used by the TCK. I don't think that TCK tests should rely on that kind of behavior. Also, JPA providers should have the freedom to disallow this unsound comparison and force the user to use the proper literal syntax with defined behavior.

gavinking commented 1 year ago

There is no mention in the spec about the expected format of the string literal.

Right. The JPA spec leaves string representation of datetimes completely undefined. As far as I recall, it doesn't even have an operation for parsing/casting a string to a datetime.

So for the TCK to mandate support for such an implicit conversion looks completely wrong.

lukasj commented 1 year ago

Any idea why the usage of the JDBC escape syntax is defined as may be used instead of must be used? As far as I can find, the escape syntax is defined since JDBC 3 at least (=for a long time already). Is it because not all JPA vendors supported this syntax? Or DB driver vendors? Or support for string literals corresponding to SQL temporal types was expected?

Don't you think that comparing a temporal path expression against a string literal is wrong?

Let me rephrase this a bit: Is comparing a temporal path expression against a string literal representing SQL temporal type literals wrong ? - I think the answer in this case should be: No, it is not wrong.

In any case: Isn't there time to improve this a bit ie by requiring usage of the JDBC escape syntax or string literals in some form of ISO 8601 format (note RFC 3339 allowing space to be a delimiter)?

gavinking commented 1 year ago

I think we could definitely add support for something like ANSI SQL-style date/time literals. I can open an issue proposing this if you like, since I already worked on this for Hibernate and I remember most of the subtleties. (Actually I had planned to propose it.)

But notice that the difference there is that the type is explicit because you have a keyword. It’s not an implicit type conversion.

beikov commented 1 year ago

No, it is not wrong.

I'd be fine to allow this if the format were specified, but since it is not, I can only conclude that these tests were relying on something that wasn't specified.

Isn't there time to improve this a bit ie by requiring usage of the JDBC escape syntax or string literals in some form of ISO 8601 format (note RFC 3339 allowing space to be a delimiter)?

I don't know the timelines, but I was of the impression that JPA 3.1 is done and at this point only test exclusions are allowed. For JPA 3.2 we can certainly clarify this matter by specifying the supported format.

scottmarlow commented 1 year ago

I verified that the staged TCKs (http://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl//jakarta-persistence-tck-3.1.2.zip + https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-jakartaeetck-10.0.2.zip) do not run the doesn't run the newly excluded tests.

CI results are in https://ci.eclipse.org/jakartaee-tck/job/10/job/eftl-standalonetck-build-run-100/116 for http://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl//jakarta-persistence-tck-3.1.2.zip and https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck/job/10.0.x/48/testReport/com.sun.ts.tests.jpa.core.query.apitests/Client for https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-jakartaeetck-10.0.2.zip

I will create follow up issues to release the updated TCKs and update the https://jakarta.ee/specifications (Persistence + Platform) web pages.