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

remove a new test from TCK #626

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Conversation

gavinking
Copy link
Contributor

This test, which was added yesterday, requires dynamically sorting by fields which are not selected. As far as I know, the specification does not explicitly require this, and in traditional SQL, sorting by unselected fields is something that is not usually considered allowed, though it's true that many databases are quite forgiving here.

Now, it's true that I didn't disallow this in JDQL, but there's a big difference between allowing it statically and allowing it for dynamically-added sorting.

Actually I probably should have disallowed it in JDQL, because in fact JPQL disallows it in section 4.10 of the latest Persistence spec. (Again, actually-existing JPQL implementations do allow it.) I will follow up with a PR that adds a caveat about this to section 5.5.5.

Folks, I think at this late stage we should try hard to avoid adding "weird" new corner-casey things like this to the TCK. Let's stick to bread-and-butter things which we know are supposed to work and leave stuff like this alone for now.

@gavinking
Copy link
Contributor Author

See #627.

This test requires sorting by fields which are not selected
but as for as I know, the specification does not explicitly
require this, and it is something that is not usually
considered allowed, though it's true that many databases are
quite forgiving here. Now, it's true that I didn't disallow
this in JDQL, but there's a big difference between allowing
it statically and allowing it for dynamically-added sorting.
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a new test that I added to the TCK for the purpose of testing unusual usage. It's from my attempt to correct an error in the TCK where it was requiring a type parameter on the repository super interface. The method that I was fixing already performed a find operation and had dynamic sorting for it, with these same asserted outputs. Unfortunately, the fix I made to restrict the return type to only the id in order to avoid the type parameter turned out to be in conflict with the dynamic sorting by other entity attributes. Instead of removing the test, we should correct it, otherwise we lose coverage. I added suggestions to switch the dynamic ordering to static.

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
@gavinking
Copy link
Contributor Author

Ah, OK, apologies, I thought it was new.

Applied your suggestions.

@otaviojava otaviojava merged commit 44d745b into jakartaee:main Apr 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants