jakartaee / data

Data-API
Apache License 2.0
105 stars 29 forks source link

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

Closed otaviojava closed 4 months ago

otaviojava commented 5 months ago

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

njr-11 commented 5 months ago

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 commented 5 months ago

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

KyleAure commented 5 months ago

@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 commented 5 months ago

@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?

https://github.com/jakartaee/data/issues/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 commented 5 months ago

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 commented 5 months ago

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

otaviojava commented 5 months ago

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 commented 5 months ago

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 <?