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

Fix: Parsing timestamps fails when producing messages #803

Merged
merged 2 commits into from
Sep 7, 2021

Conversation

xarx00
Copy link
Contributor

@xarx00 xarx00 commented Sep 6, 2021

The commit fixes two related problems that prevent producing messages if AVRO contains a timestamp field. Production of such messages wasn't possible at all in AKHQ version 0.18.0, but the major problems have been fixed since then. However, some problems still remain.

Let's have an AVRO like this:

{
  "type": "record",
  "name": "myrecord",
  "fields": [
    {
      "name": "cas",
      "type": {
        "type": "long",
        "logicalType": "timestamp-millis"
      }
    }
  ]
}
  1. The message
{
  "cas": 12345
}

fails to be produced because Integer cannot be converted to Instant.

  1. The message
{
  "cas": "2021-09-05T20:31:53.582+02"
}

cannot be produced because the parser cannot parse short time-zones.

This patch fixes both problems. Timestamp strings without time-zone can still be parsed, Even negative numbers (Integers and Longs) can be parsed.

Fix: ZonedDateTime didn't allow zone othen than GMT
@tchiotludo
Copy link
Owner

Thanks @xarx00 , maybe you can add a unit test to validate the behavior please ?

@xarx00
Copy link
Contributor Author

xarx00 commented Sep 6, 2021

I added unit tests for short form of TZ offset. AvroSerializer methods like timestampMillisSerializer() need testing too (in order to test my other changes), but I would have to change their visibility (they are private), which I didn't dare to do.

@tchiotludo tchiotludo merged commit 7f476bc into tchiotludo:dev Sep 7, 2021
tchiotludo pushed a commit that referenced this pull request Oct 24, 2021
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.

2 participants