Skip to content

HHH-19694 Enhanced support for older Informix #10706

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VladoKuruc
Copy link
Contributor

https://hibernate.atlassian.net/browse/HHH-19694


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Aug 7, 2025

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@beikov
Copy link
Member

beikov commented Aug 7, 2025

Can you please prefix all commits with the Jira key HHH-19694 and maybe also squash the commits?

Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

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

I guess the changes to the Dialect itself are OK but I don't see why there should be any changes to tests here.

The test suite was already all passing on the current release of Informix, and we don't need to be able to test on obsolete versions of Informix. (We can't, actually, since we don't have access to any.)

So unless there's some other good reason for these test changes (other than to make the tests work on obsolete versions), please back out those changes. Thanks.

@@ -52,7 +52,7 @@
// make sure all 3 are still physically there
session.doWork( (connection) -> {
final Statement statement = connection.createStatement();
final ResultSet resultSet = statement.executeQuery( "select count(1) from implicit_entities" );
final ResultSet resultSet = statement.executeQuery( "select count(*) from implicit_entities" );

Check warning

Code scanning / CodeQL

Potential database resource leak

This ResultSet is not always closed on method exit.
@gavinking
Copy link
Member

pfffffff, oh gawd I just noticed we're already back up to 26 failures (from zero) on main, including some stuff that really should not have been able to break, after relatively few changes. I guess we should add an Informix CI job.

@VladoKuruc
Copy link
Contributor Author

I guess the changes to the Dialect itself are OK but I don't see why there should be any changes to tests here.

The test suite was already all passing on the current release of Informix, and we don't need to be able to test on obsolete versions of Informix. (We can't, actually, since we don't have access to any.)

So unless there's some other good reason for these test changes (other than to make the tests work on obsolete versions), please back out those changes. Thanks.

The tests with the annotation @RequiresDialectFeature(DialectChecks.SupportsDmlTargetColumnQualifier.class) relate to the bug at https://hibernate.atlassian.net/browse/HHH-18473. This issue was fixed for a newer version of Informix by switching to TABLE_ALIAS. I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I'm happy to revert the other set of tests where I changed count(1) to count(*), as it seems only the old Informix version has a problem with it.

@gavinking
Copy link
Member

I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I don't understand what you're saying. DynamicFIlterTest passes for me. Why would we skip it?

@VladoKuruc
Copy link
Contributor Author

I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I don't understand what you're saying. DynamicFIlterTest passes for me. Why would we skip it?

Only skip for old Informix:

public static class SupportsDmlTargetColumnQualifier implements DialectCheck {
    @Override
    public boolean isMatch(Dialect dialect) {
       return dialect.getDmlTargetColumnQualifierSupport() != DmlTargetColumnQualifierSupport.NONE;
    }
}
...
@Override
public DmlTargetColumnQualifierSupport getDmlTargetColumnQualifierSupport() {
    return getVersion().isSameOrAfter( 12,10 ) ? DmlTargetColumnQualifierSupport.TABLE_ALIAS : DmlTargetColumnQualifierSupport.NONE;
}

@gavinking
Copy link
Member

Only skip for old Informix:

Ok, so what I'm saying is we don't want anything in the test suite which is specific to obsolete versions of Informix.

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.

3 participants