-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: configurable extra allowable metadata field attributes #73
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,12 @@ def self.build_schema(str = nil, filename: nil, &block) | |
def self.add_load_path(*paths) | ||
Avro::Builder::DSL.load_paths.merge(paths) | ||
end | ||
|
||
# Define extra allowable metadata attributes for fields | ||
def self.extra_metadata_attributes(*attrs) | ||
Avro::Builder::Field.extra_metadata_attributes(attrs) | ||
Avro::Builder::Record.extra_metadata_attributes(attrs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the Avro spec supports metadata on all types rather than just record types. If that's correct, then should this be on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jturkel sorry I haven’t got back to this because I hit some code here that could benefit from a refactor. Any possibility we could merge this as is and then circle back later? |
||
end | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# frozen_string_literal: true | ||
|
||
module Avro | ||
module Builder | ||
module Metadata | ||
module ClassMethods | ||
def extra_metadata_attributes(attrs) | ||
dsl_attributes *attrs | ||
end | ||
end | ||
|
||
def self.included(base) | ||
base.extend ClassMethods | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
# frozen_string_literal: true | ||
|
||
describe Avro::Builder, ".extra_metadata_attributes" do | ||
before do | ||
Avro::Builder.extra_metadata_attributes(:reference, :deprecated_by, :documentation_url) | ||
end | ||
|
||
context "applying attributes to fields in a record" do | ||
subject(:schema_json) do | ||
described_class.build do | ||
record :r do | ||
required :n, :null | ||
required :b, :boolean, reference: 'com.example.bool', deprecated_by: 'com.example.bool_v2', other: 'value' | ||
required :s, :string | ||
required :i, :int | ||
optional :l, :long do | ||
doc 'A long value' | ||
order 'ascending' | ||
reference 'com.example.long' | ||
deprecated_by 'com.example.long_v2' | ||
end | ||
required :f, :float | ||
optional :d, :double | ||
required :many_bits, :bytes | ||
end | ||
end | ||
end | ||
|
||
let(:expected) do | ||
{ | ||
type: :record, | ||
name: :r, | ||
fields: [ | ||
{ name: :n, type: :null }, | ||
{ name: :b, type: :boolean, reference: 'com.example.bool', deprecated_by: 'com.example.bool_v2' }, | ||
{ name: :s, type: :string }, | ||
{ name: :i, type: :int }, | ||
{ | ||
name: :l, | ||
type: [:null, :long], | ||
default: nil, | ||
doc: 'A long value', | ||
order: 'ascending', | ||
reference: 'com.example.long', | ||
deprecated_by: 'com.example.long_v2' | ||
}, | ||
{ name: :f, type: :float }, | ||
{ name: :d, type: [:null, :double], default: nil }, | ||
{ name: :many_bits, type: :bytes } | ||
] | ||
} | ||
end | ||
|
||
it { is_expected.to be_json_eql(expected.to_json) } | ||
end | ||
|
||
context "applying attributes to the record" do | ||
subject(:schema_json) do | ||
described_class.build do | ||
record :r do | ||
documentation_url 'https://example.com/docs' | ||
reference 'internal-reference' | ||
required :b, :boolean | ||
optional :d, :double | ||
end | ||
end | ||
end | ||
|
||
let(:expected) do | ||
{ | ||
type: :record, | ||
name: :r, | ||
fields: [ | ||
{ name: :b, type: :boolean }, | ||
{ name: :d, type: [:null, :double], default: nil } | ||
], | ||
documentation_url: 'https://example.com/docs', | ||
reference: 'internal-reference' | ||
} | ||
end | ||
|
||
it { is_expected.to be_json_eql(expected.to_json) } | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the spec supports metadata attributes on all JSON object in the serialized Avro schema. Is there a generalized version of this change that supports attributes on constructs other than just fields e.g. records?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jturkel Yes, I can make that change no problem. Would you rather seperate config blocks for declaring the attributes for for each object type? Or is all together ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with how people are using metadata on Avro schemas but my instinct is to keep it simple for now with a single set of attributes that applies to all types/fields. We can always extend this in the future to support both global and type specific metadata attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jturkel Fantastic. I’ve pushed that change. There is the possibility still to have individual setters for records vs fields in the future if you want too.