Skip to content

Make avro serde accept clj maps with underscores, add option to skip mangling names.#126

Closed
sathyavijayan wants to merge 0 commit into
FundingCircle:masterfrom
sathyavijayan:master
Closed

Make avro serde accept clj maps with underscores, add option to skip mangling names.#126
sathyavijayan wants to merge 0 commit into
FundingCircle:masterfrom
sathyavijayan:master

Conversation

@sathyavijayan

Copy link
Copy Markdown

Ive made a quick fix/change to the avro serde to make it accept non-idiomatic, but perfectly valid maps as input. For eg.

{"string_field": "hello", :enum_field "a_1"}

I also added an option to skip mangling names like in the abracad library. This will be useful when we need to use the original message.

@sathyavijayan sathyavijayan requested a review from a team as a code owner April 18, 2019 14:54

@cddr cddr left a comment

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 this would be a useful change. If I had our time again, *mangle-names* == false would be the default but I think the ship has sailed on that.

@creese do you accept this in principle? If so, we might consider adding it as an option here

so that end-users don't need to fiddle with dynamic vars.

Comment thread test/jackdaw/serdes/avro_test.clj Outdated
schema-str (json/write-str schema)
serde (->serde schema-str)]

(is (= (round-trip serde "bananas"

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.

It took me a while before I realized that the difference between the 1st and 2nd round-trip tests here was that the enum field was expressed as a string in the first, and a keyword in the second. Would you mind putting each one inside a testing block so that they can more easily be distinguished?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks. I have restructured the tests and added documentation to make them more readable.

I used a dynamic var for *mangle-names* as it made more sense for me to have it as an 'application-level configuration'. It will lead to a consistent low level design within an application - either use idiomised values or not. (although, I agree the degree of ambiguity wouldn't be much as there will always be a default setting).

I noticed that the mangling is done only for the serde and not for json etc., So, as you mentioned, it would make sense to drop it altogether and push the responsibility to the apps if they need such a feature. The dynamic var will be a nice way to provide backward compatibility during transition if you decide to drop mangling in a future release.

Let me know your thoughts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @cddr, @creese any thoughts on this PR ? Let me know if you still like the dyn var to be moved to the topic-config map (last arg to the serde function) instead. The map is named topic-config which makes it a weird place for the mangle-names key - what do you think ?

@codecov-io

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@4bbb6de). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #126   +/-   ##
=========================================
  Coverage          ?   80.86%           
=========================================
  Files             ?       39           
  Lines             ?     2315           
  Branches          ?      147           
=========================================
  Hits              ?     1872           
  Misses            ?      296           
  Partials          ?      147
Impacted Files Coverage Δ
src/jackdaw/serdes/avro.clj 86.91% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bbb6de...8b7a894. Read the comment docs.

@peey

peey commented Jul 29, 2020

Copy link
Copy Markdown

Anything blocking this from being merged?

@obohrer

obohrer commented Jan 26, 2021

Copy link
Copy Markdown

Can this be merged? This caused us issues as well as the data mangling was a surprise to us

@dakra

dakra commented Oct 24, 2021

Copy link
Copy Markdown

+1 for merging this. Not be able to use maps with snake_case is a bit of a showstopper.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@4bbb6de). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/jackdaw/serdes/avro.clj 87.50% 0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #126   +/-   ##
=========================================
  Coverage          ?   80.99%           
=========================================
  Files             ?       39           
  Lines             ?     2315           
  Branches          ?      147           
=========================================
  Hits              ?     1875           
  Misses            ?      293           
  Partials          ?      147           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

8 participants