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

Byte array converter + converters cleanup #388

Merged
merged 3 commits into from
Jul 17, 2022

Conversation

roimenashe
Copy link
Member

  1. Byte arrays should not be converted or waste time on unnecessary convert collection flow (read flow only, write flow treat Byte arrays as simple fields).
  2. Aerospike converters cleanup, remove unnecessary/unused converters.

@roimenashe roimenashe requested a review from reugn May 29, 2022 16:45
@roimenashe roimenashe self-assigned this May 29, 2022
@roimenashe roimenashe requested a review from Aloren May 29, 2022 16:45
Copy link
Member

@reugn reugn left a comment

Choose a reason for hiding this comment

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

We may need to add the same check to the MappingAerospikeWriteConverter.

@roimenashe
Copy link
Member Author

roimenashe commented May 30, 2022

We may need to add the same check to the MappingAerospikeWriteConverter.

@reugn
Byte[] fields are written directly (without conversions), method MappingAerospikeWriteConverter.getValueToWrite() checks whether the field value is a simple value in line 128 - conversions.isSimpleType(value.getClass())) is true.

Also test MappingAerospikeConverterTypesTest.ObjectWithByteArrayFieldWithOneValueInData() covers write and read of byte[] fields.

@roimenashe roimenashe requested a review from reugn May 30, 2022 14:39
@@ -132,6 +132,10 @@ private <T> T readValue(Object source, TypeInformation<?> propertyType) {
if (conversions.hasCustomReadTarget(source.getClass(), targetClass)) {
return (T) conversionService.convert(source, targetClass);
} else if (propertyType.isCollectionLike()) {
/* Byte arrays should not be converted or waste time on unnecessary convert collection flow */
if (source instanceof byte[]) {
Copy link

Choose a reason for hiding this comment

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

Please prepare a test where:

  1. you write data using document:
class Doc {
byte[] array;
}
  1. you read data using document:
class Doc {
List<Byte> array;
}

Existing implementation should not fail, but introduced change will fail with exception. This is because you need to check not only for source, but also for target type.

Copy link

Choose a reason for hiding this comment

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

@roimenashe
Copy link
Member Author

@Aloren
Added a check for the target type - convert as it is only in case the source is byte[] and the target type is also byte[] ("[B") otherwise convert as collection.
Also added the tests that you suggested.

@roimenashe roimenashe requested a review from Aloren July 6, 2022 08:43
@roimenashe roimenashe merged commit f289e5c into main Jul 17, 2022
@roimenashe roimenashe deleted the byte-array-default-converter branch July 17, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants