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

[TCK Challenge]: The TCK contains a repository method with an UPDATE query that returns boolean but this is not allowed in the spec and Javadoc #923

Open
1 task done
OndroMih opened this issue Jan 6, 2025 · 3 comments · May be fixed by #924
Labels
challenge TCK Challenge test Something test-related

Comments

@OndroMih
Copy link

OndroMih commented Jan 6, 2025

Specification

boolean move(UUID id, double newX, float yDivisor);

Assertion

assertEquals(true, shared.move(id1, 1.23d, 1.5f));

TCK Version

1.0.1

Implementation being tested

JNoSQL Jakarta Persistence Extension

Challenge Scenario

Claims that a test asserts requirements over and above that of the specification.

Full Description

The test testUpdateQueryWithWhereClause in

assertEquals(true, shared.move(id1, 1.23d, 1.5f));
asserts that the return value from calling the move method is true. However, it's not clear what the return value should represent.

The move method in the MultipleEntityRepo class is annotated with @Query,which contains an UPDATE statement. I didn't find anywhere in the spec of javadoc any mention of what should be returned in this case.

On the contrary, the spec suggests that returning a boolean type is forbidden in this case:

  • The @Query Javadoc says: "For select statements, the return type of the query method must be consistent with the type returned by the query. For update or delete statements, it must be void, int or long."
  • In the specification document: "implementations are encouraged to support void as the return type for a query which never returns a result"

Additional Context

I suggest that the method move in MultipleEntityRepo is modified to return void and remove the assertion.

A future version of the Data spec could introduce the boolean return type and clarify what it means. it could e.g. mean that it's true when a long type would return > 0, false otherwise.

It would also be good to clarify what the int and long return types mean, whether methods should return the number of modified entities or > 0 if some entities modified, or it a value of > 0 means an error. It's not no clearly stated anywhere in the spec of javadocs, I only assume it's the number of modified entities based on the assertions in the TCK tests.

Is there an existing challenge for this?

  • I have searched the existing issues
@OndroMih OndroMih added the challenge TCK Challenge label Jan 6, 2025
@njr-11
Copy link
Contributor

njr-11 commented Jan 6, 2025

@OndroMih nice work finding this inconsistency between the TCK and what is documented for valid return types of @Query. My vote is for accepting this TCK challenge because boolean which is used by the test case is not an allowed return type for @Query. If others agree and this challenge is accepted, then you should be able to run the TCK excluding this test failure to certify a compatible implementation.

I'll look into putting together a PR to fix the test and improve the spec JavaDoc for Data 1.1. For the fix in 1.1, I will plan to switch the test from boolean to int or long so that we still have coverage for @Query with an UPDATE statement having a return value.

@njr-11 njr-11 added the test Something test-related label Jan 6, 2025
njr-11 added a commit to njr-11/data that referenced this issue Jan 6, 2025
@gavinking
Copy link
Contributor

Yes, I believe I argued against requiring this, so leaving it out was deliberate, assuming my memory doesn't mislead.

@OndroMih
Copy link
Author

OndroMih commented Jan 6, 2025

It looks like the tests expect return value true when count > 0, so this inconsistency doesn’t block my implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
challenge TCK Challenge test Something test-related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants