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

Updated the KafkaEvent header(s) value to use the correct array type of sbyte values, so that negative values could be handled. #1476

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

ashishdhingra
Copy link
Contributor

Issue #, if available: Issue reported internally.

Description of changes:
Updated the KafkaEvent header(s) value to use the correct array type of sbyte values, so that negative values could be handled.

This is a breaking change, hence, major version bump.

Context:
The POCO KafkaEvent class was contributed from external user, and was most probably ported from https://github.com/aws/aws-lambda-java-libs/blob/main/aws-lambda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/KafkaEvent.java.
In C#, byte type is unsigned and to use signed type, sbyte type needs to be used.
In Java, it's the other was around, byte is signed. Java doesn't have unsigned bytes.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…of sbyte values, so that negative values could be handled.
@kevinstapleton
Copy link
Contributor

The header value represents binary data and shouldn't contain negative numbers. My opinion is that the proper place to fix this issue is on the side generating the JSON payload.

A Kafka message has three places where it handles binary data (the key, the value, and header values), and for some reason they are all handled differently:

  • Message Key: A JSON string of base-64 encoded bytes represented as a string
  • Message Value: A JSON string of base-64 encoded bytes represented as a MemoryStream
  • Header Value: A JSON array of signed (should be unsigned) bytes represented as a byte[]

My recommendation would be to fix the side generating the JSON payload to emit the header values as JSON strings of base-64 encoded bytes, just as it does for the message key and value. Then, on the C# contract side, byte[] is used as the type for all three, as both Newtonsoft and System.Text.Json will automatically deserialize base-64 strings into byte[] out of the box.

@ozziepeeps
Copy link

I agree with @kevinstapleton , I wonder if it might be better to fix the JSON payload generation side instead of what is proposed in this PR @96malhar ? /CC @normj

@normj
Copy link
Member

normj commented Apr 5, 2023

@ozziepeeps I admit I'm not very experienced with Kafka events but it sounds like what you and @kevinstapleton would like to see is for the service side to change the format of the events they emit out. That is more then we control in this repository. Here we create the POCO for how the service document the event format. Actually a lot of the POCOs are community contributed.

We did pass along your feedback to the service team. It wouldn't be a simple change for them because it would have to be an opt-in behavior to receive messages in a different format while still maintaining the current format as the default.

If the service team does create a new event format we would create a new V2 version of the POCO to match the new format similar to how API Gateway was done.

@normj
Copy link
Member

normj commented Apr 5, 2023

We could also add or accept a PR that adds utility methods to the POCO that make it easier to work binary data. We have done this in several other event POCOs.

@ashishdhingra ashishdhingra merged commit b3fd72c into dev Apr 6, 2023
@ashishdhingra ashishdhingra deleted the user-ashdhin-KafkaEvent-NegativeByteInHeaderFix branch April 6, 2023 16:19
@normj
Copy link
Member

normj commented Apr 7, 2023

Version 2.0.0 of the Amazon.Lambda.KafkaEvents package is out with this PR. The major version bump is for the breaking change from byte to sbyte.

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.

5 participants