Skip to content

Implement BinaryStreamReader.EnumValue.toString() - #2209

Merged
chernser merged 1 commit into
ClickHouse:mainfrom
jjtt:enum-value-tostring
Mar 14, 2025
Merged

Implement BinaryStreamReader.EnumValue.toString()#2209
chernser merged 1 commit into
ClickHouse:mainfrom
jjtt:enum-value-tostring

Conversation

@jjtt

@jjtt jjtt commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

Summary

In a Spring Boot application using org.springframework.jdbc.core.JdbcTemplate#queryForList(java.lang.String), doing a simple SELECT * from table from a table containing Enum columns returns the enum values as strings as interpreted by Object.toString(). These strings are not meaningful to the caller, since they are of the form "com.clickhouse.client.api.data_formats.internal.BinaryStreamReader$EnumValue@7331ed36"

name feels to me like the obvious choice to represent an enum value as string.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@CLAassistant

CLAassistant commented Mar 10, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@chernser

Copy link
Copy Markdown
Contributor

@jjtt would you please add a simple test to check that name is present?

}

@Override
public String toString() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think, getName would be more correct here.
toString usually tells more about object.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jjtt would you please add getName method also?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem in the case mentioned in the PR description is that something ends up calling toString() to convert the Enum8/Enum16 in the db into a string returned by a query. So that something doesn't know that it should call getName().

I don't know which part decides that the column type is a String in Java. Maybe that should be changed to default to using an integer type for enums and then it wouldn't matter what the toString() of an EnumValue is?

In my use case I don't care about the integer value of the enum, but I'm sure I could change the query to explicitly return a string. This SELECT * is actually in integration test code that verifies the db content. IMO, production code shouldn't use SELECT * anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess this also needs to be considered:

return (T) new EnumValue(name == null ? "<unknown>" : name, enum8Val);

so there are situations where the string value of the enum can be unknown to the driver?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In some cases with Dynamic types it can be unknown. Theoretically.

As for the rest - It should be fixed may be in another place to make it aware about getName. We will look at it later.

@jjtt

jjtt commented Mar 14, 2025

Copy link
Copy Markdown
Contributor Author

@jjtt would you please add a simple test to check that name is present?

Test as in adding a null check before returning the name value?

Or test as in adding a unit test for the case when name is null?

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