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

HHH-17246 - Guard against Sybase being configured for truncating trailing zeros #9079

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

Conversation

jrenaat
Copy link
Contributor

@jrenaat jrenaat commented Oct 10, 2024


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.


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

@jrenaat jrenaat requested a review from beikov October 10, 2024 19:26
@gavinking
Copy link
Member

@jrenaat It should be possible to detect that Sybase is configured like this, right? Just like we detect certain things about how MySQL or Oracle is configured, when we instantiate their dialects.

I think we should start logging warnings for such "bad" config, i.e. this thing, and also non-ANSI nulls.

@jrenaat
Copy link
Contributor Author

jrenaat commented Oct 11, 2024

@jrenaat It should be possible to detect that Sybase is configured like this, right? Just like we detect certain things about how MySQL or Oracle is configured, when we instantiate their dialects.

I think we should start logging warnings for such "bad" config, i.e. this thing, and also non-ANSI nulls.

I tried to find out how to do this, but failed miserably ...

Comment on lines +36 to +38
final Book book = new Book(uuid, "John Doe");
session.persist( book );
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to test what happens when 15 bytes are written, so let's write the first 15 bytes only.

Suggested change
final Book book = new Book(uuid, "John Doe");
session.persist( book );
session.createMutationQuery( "insert into Book(id, author) values (?,'John Doe')" ).setParameter(1, Arrays.copyOf( UUIDJavaType.ToBytesTransformer.transform( uuid ), 15 ) ).executeUpdate();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do explicitely want to write 16 bytes to the db, and i check if the value recovered is 15 bytes, to ensure that there is effectively some truncation going on; without that i wouldn't have seen that this truncation doesn't happen if in the UuidAsBinaryJdbcType we indicate BINARY as the ddl type code. Truncation only happens if that is VARBINARY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, the UuidAsBinaryJdbcType seems to cause issues for Oracle, irrespective of what ddl type code is used (happens with binary, as well as varbinary)

public UUID fromString(CharSequence string) {
return ToStringTransformer.INSTANCE.parse( string.toString() );
public UUID fromString(CharSequence string) {
return '-' == string.charAt( 8 ) ? ToStringTransformer.INSTANCE.parse( string.toString() ) : NoDashesStringTransformer.INSTANCE.parse( string.toString() );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind this change, but is it necessary to get some test working?

Copy link
Contributor Author

@jrenaat jrenaat Oct 15, 2024

Choose a reason for hiding this comment

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

Yes, the new UUID type is causing some issues with json in oracle; this change fixes a number of tests in JsonEmbeddableTest, but there is still one remaining (testSelectionItems), that as far as i can tell has to with the function hexToRaw not liking that the UUID hex String contains '-'.

@@ -41,7 +41,7 @@
return ToStringTransformer.INSTANCE.transform( value );
}

public UUID fromString(CharSequence string) {
public UUID fromString(CharSequence string) {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
BasicJavaType.fromString
; it is advisable to add an Override annotation.
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