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

[Bug]: Contains and EndsWith should not be mandatory, but the TCK makes it mandatory. #768

Closed
Tracked by #762
otaviojava opened this issue Jun 13, 2024 · 8 comments · Fixed by #771
Closed
Tracked by #762
Assignees
Labels
bug Something isn't working test Something test-related

Comments

@otaviojava
Copy link
Contributor

otaviojava commented Jun 13, 2024

Specification Version

1.0.0

Bug report

By specification:

An implementation of Query by Method Name backed by a document or graph database is not required to support Contains, EndsWith, StartsWith, Like, IgnoreCase, In, or Null. A repository method must raise java.lang.UnsupportedOperationException or a more specific subclass of the exception if the database does not provide the requested functionality.

Ref: https://github.com/jakartaee/data/blob/main/spec/src/main/asciidoc/method-query.asciidoc#27-query-by-method-name-conditions

However, when I ran at the TCK, it shows it as an error:

[ERROR]   MyEntityTests>EntityTests.testContainsInString:419 » UnsupportedOperation Contains is not supported in Eclipse JNoSQL method query
[ERROR]   MyEntityTests>EntityTests.testFindFirst3:617 » UnsupportedOperation EndsWith is not supported in Eclipse JNoSQL method query

Additional information

No response

@otaviojava otaviojava added the bug Something isn't working label Jun 13, 2024
@otaviojava otaviojava changed the title [Bug]: Contains should not be mandatory, but the TCK makes it mandatory. [Bug]: Contains and EndsWith should not be mandatory, but the TCK makes it mandatory. Jun 13, 2024
@njr-11
Copy link
Contributor

njr-11 commented Jun 13, 2024

Did anyone ever give the TCK a way to input which type of database you are using so that it can properly make assertions on where it can expect UnsupportedOperationException vs where it must not be raised? That seems like the first step.

Aside from that, it looks to me like Contains and EndsWith aren't the only keywords on that list that it uses, I also see IgnoreCase and In and haven't checked for the others.

    AsciiCharacter findByHexadecimalIgnoreCase(String hex);

    Stream<AsciiCharacter> findByHexadecimalIgnoreCaseBetweenAndHexadecimalNotIn(String minHex,
                                                                                 String maxHex,
                                                                                 Set<String> excludeHex,
                                                                                 Order<AsciiCharacter> sorts);

Once the capability is in place, we will need to trap for UnsupportedOperationException in all of these places and consider it a passing result only if Document or Graph.

@njr-11 njr-11 added the test Something test-related label Jun 13, 2024
@njr-11
Copy link
Contributor

njr-11 commented Jun 13, 2024

@KyleAure we should include this as an issue to fix for the v1.0.1 TCK.

@KyleAure
Copy link
Contributor

@njr-11 @otaviojava
I can see a few different ways of handling this in the TCK:

  1. Filter tests based on Database type, similar to how we filter based on Entity type (Persistence/NoSQL)
  2. Introduce a heuristic method that can determine the Database type and allow you skip/assert based on that.
  3. Always catch UnsupportedOperationException and have that be a passing scenario.

Any preference?

@otaviojava
Copy link
Contributor Author

otaviojava commented Jun 13, 2024

@njr-11 @otaviojava I can see a few different ways of handling this in the TCK:

  1. Filter tests based on Database type, similar to how we filter based on Entity type (Persistence/NoSQL)
  2. Introduce a heuristic method that can determine the Database type and allow you skip/assert based on that.
  3. Always catch UnsupportedOperationException and have that be a passing scenario.

Any preference?

The state of the art, for me, is option 1.
But it requires time, mainly because I am still reading and fixing to make JNoSQL pass on the TCK.

But I am OK with option 3 for a while.

Can we include the issue between them in this scenario?

#757

I would create a special branch to pass only to the TCK, but once it generates a new version, we could also fix it on this 1.0.1 version.

P.s.: I still working on the TCK, if the release happen in two or three weeks I can check all the tests and see the ones to NoSQL.

@njr-11
Copy link
Contributor

njr-11 commented Jun 13, 2024

Option 3 is inaccurate because it assumes UnsupportedOperationException is always valid, but it is only valid for Graph and Document. This would allow other database types to pass the TCK despite violating specification requirements.

Option 2 is better because the test can make a correct assertion for UnsupportedOperationException (must be Graph or Document) and a correct assertion for other database types (repository method must succeed).

Option 1 is not as good as Option 2 because we lose out on asserting that Graph and Document will either raise UnsupportedOperationException or succeed.

@KyleAure
Copy link
Contributor

KyleAure commented Jun 14, 2024

Thanks for the feedback. I agree that Option 2 is likely the best middle ground.
I'll work on a prototype for that.

@KyleAure KyleAure self-assigned this Jun 14, 2024
KyleAure added a commit to KyleAure/open-liberty that referenced this issue Jun 14, 2024
@otaviojava
Copy link
Contributor Author

There is one issue with this approach:

NoSQL databases do not have standards as we have in relational databases.

The classic is with keywords; take, for example, MongoDB and Oracle NoSQL, where both are document databases.

Oracle NoSQL works to capture the most relational capabilities possible, including the between keywords. On the other hand, MongoDB and ArangoDB do not provide support for the same keyword.

@gavinking
Copy link
Contributor

Not sure I'm understanding exactly what you mean Otavio.

Do you mean there's no between keyword, or that it can't even be emulated with and and <?

KyleAure added a commit to KyleAure/open-liberty that referenced this issue Sep 19, 2024
KyleAure added a commit to KyleAure/open-liberty that referenced this issue Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test Something test-related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants