flipp-oss / deimos

Framework to work with Kafka, Avro and ActiveRecord
Other
59 stars 22 forks source link

DB backend errors when using Avro-encoded key #95

Closed jmccance closed 3 years ago

jmccance commented 3 years ago

When using the DB backend and an Avro-encoded key, the DB backend errors out when trying to persist the KafkaMessage.

TypeError: can't quote ActiveSupport::HashWithIndifferentAccess
from /Users/jmccance/.asdf/installs/ruby/2.6.3/lib/ruby/gems/2.6.0/gems/activerecord-6.0.3.4/lib/active_record/connection_adapters/abstract/quoting.rb:231:in `_quote'

I think the issue is here in lib/deimos/backends/db.rb, when m.partition_key is nil and m.key is a Hash.

partition_key: m.partition_key || m.key
jmccance commented 3 years ago

I experimented with using m.encoded_key.to_s.b, but then it complains about the presence of a null byte. Happy to provide a PR, but will need direction on what you would like to use for a partition key in this instance. Base64-encoded form of m.encoded_key, maybe?

dorner commented 3 years ago

Hey @jmccance, thanks for the bug report! Am I correct that you're encoding the key manually before you pass it to the producer? The producer should be handling all the encoding, including the key itself. Do you have some sample code you can provide?

jmccance commented 3 years ago

Nope, no manual encoding. I can try to pull together a small example to reproduce, but here's some (lightly redacted) bits to start:

My producer's key_config:

key_config schema: 'my.company.RequestKey'

The schema:

{
  "type": "record",
  "name": "RequestKey",
  "namespace": "my.company",
  "fields": [
    { "name": "id", "type": "string" }
  ]
}

The publish:

publish({
  'payload_key' => { 'id' => reference_id },
  'request' => {
    'foo' => 'bar',
  }
})
jmccance commented 3 years ago

Oh, and to re-emphasize: key itself gets encoded fine. The issue seems to be that the partition_key does not.

dorner commented 3 years ago

Interesting... I'm almost positive we have this case in our flagship app and we didn't see any crashes. Let me try and reproduce it and maybe I can figure out what's going on.

dorner commented 3 years ago

OK, found it - so the difference is that we're on a much older version of Rails, and this error check in ActiveRecord seems to have been introduced afterwards. In old Rails, any unknown class would be quoted like this:

"'#{quote_string(value.to_yaml)}'"

So I think we can just add .to_yaml to the key and it should work! Want to whip up a PR?

jmccance commented 3 years ago

Ah, interesting! Yeah, we're on Rails 6.

I'll get a PR pulled together. Thanks!