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

Hash Serialization #182

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 141 additions & 28 deletions lib/protoboeuf/codegen.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,139 @@ def resolve
end
end

# Generates #to_h, #as_json, and #to_json methods
class HashSerializationCompiler
include TypeHelper

attr_reader :message, :fields, :oneof_selection_fields, :generate_types

class << self
def result(message:, fields:, oneof_selection_fields:, generate_types:)
new(message:, fields:, oneof_selection_fields:, generate_types:).result
end
end

def initialize(message:, fields:, oneof_selection_fields:, generate_types:)
@message = message
@fields = fields.sort_by(&:number) # Serialize fields in their proto order
@oneof_selection_fields = oneof_selection_fields
@generate_types = generate_types
end

def result
<<~RUBY
#{type_signature(returns: "T::Hash[Symbol, T.untyped]")}
def to_h
result = {}

#{fields.map { |f| to_h_assign_statement_rb(f) }.join("\n")}

result
end

#{type_signature(params: { options: "T::Hash[T.untyped, T.untyped]" }, returns: "T::Hash[Symbol, T.untyped]")}
def as_json(options = {})
result = {}

#{fields.map { |f| as_json_assign_statement_rb(f) }.join("\n")}

result
end

def to_json(options = {})
require 'json'
JSON.dump(as_json(options))
end
RUBY
end

private

def to_h_assign_statement_rb(field)
key = to_h_hash_key_rb(field)
value = to_h_hash_value_rb(field)

if field.has_oneof_index? && !field.optional?
oneof_selection_field_name = oneof_selection_fields[field.oneof_index].name.dump
field_name = field.name.dump

"result[#{key}] = #{value} if send(:#{oneof_selection_field_name}) == :#{field_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we use send here?
I see it in the old code, but we (now) have (public) attribute readers for these. 🤔
Not sure if maybe we didn't at some point.
If there is a reason, we should put a comment above this line explaining it.
If we don't, should we switch to self.#{...} (to still avoid any possible name clashes with local vars)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. self.#{...} is probably better.

else
"result[#{key}] = #{value}"
end
end

def to_h_hash_key_rb(field)
":#{field.name.dump}"
end

def to_h_hash_value_rb(field)
return to_h_hash_value_for_map_rb(field) if field.map_field?

# For primitives or arrays of primitives we can just use the instance variable value
return field.iv_name unless field.type == :TYPE_MESSAGE

if field.repeated?
# repeated maps aren't possible so we don't have to worry about to_h arity or as_json not being defined
"#{field.iv_name}.map { |v| v.to_h }"
else
"#{field.iv_name}.to_h"
end
end

def to_h_hash_value_for_map_rb(field)
if field.map_type.value.type == :TYPE_MESSAGE
"#{field.iv_name}.transform_values { |value| value.to_h }"
else
field.iv_name
end
end

def as_json_assign_statement_rb(field)
key = as_json_hash_key_rb(field)
value = as_json_hash_value_rb(field)

if field.has_oneof_index? && !field.optional?
oneof_selection_field_name = oneof_selection_fields[field.oneof_index].name.dump
field_name = field.name.dump

"result[#{key}] = #{value} if send(:#{oneof_selection_field_name}) == :#{field_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can see it in the generated code, sometimes there are multiple of these in a row, so we're calling send(key) several times, I wonder if there is a way to consolidate that.

elsif field.repeated?
"#{value}.tap { |v| result[#{key}] = v if !options[:compact] || v.any? }"
Copy link
Contributor

Choose a reason for hiding this comment

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

I often write my ruby code this way (and I see an instance of this in the old code), but I wonder if, for performance, the generated code should not have unnecessary blocks.
It's probably fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is #{value} could evaluate to a recursive method call, so I used tap to avoid duplicate calls. I suppose we could use a local variable instead. tap seemed more like Idiomatic Ruby but the performance concern is definitely worth considering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I was thinking maybe we could use something like tmp_#{field.lvar} = #{value} and then check the tmp var 🤷

elsif field.optional?
"result[#{key}] = #{value} if !options[:compact] || has_#{field.name}?"
Copy link
Contributor

Choose a reason for hiding this comment

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

This interpolation for the predicate name triggers my spidey-sense but this is how we generate the name when defining the predicate so I guess it's fine.
Might be nice to have predicate_name or something on the Field class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of putting predicate_name on the Field class.

else
"result[#{key}] = #{value}"
end
end

def as_json_hash_key_rb(field)
field.json_name.dump
end

def as_json_hash_value_rb(field)
return as_json_hash_value_for_map_rb(field) if field.map_field?

# For primitives or arrays of primitives we can just use the instance variable value
return field.iv_name unless field.type == :TYPE_MESSAGE

if field.repeated?
# repeated maps aren't possible so we don't have to worry about to_h arity or as_json not being defined
"#{field.iv_name}.map { |v| v.as_json(options) }"
else
"#{field.iv_name}.nil? ? {} : #{field.iv_name}.as_json(options)"
end
end

def as_json_hash_value_for_map_rb(field)
if field.map_type.value.type == :TYPE_MESSAGE
"#{field.iv_name}.transform_values { |value| value.as_json(options) }"
else
field.iv_name
end
end
end

class MessageCompiler
attr_reader :generate_types, :requires

Expand All @@ -88,8 +221,13 @@ def result(message, toplevel_enums, generate_types:, requires:, syntax:, options
end
end

attr_reader :message, :fields, :oneof_fields, :syntax
attr_reader :optional_fields, :enum_field_types
attr_reader :message,
:fields,
:oneof_fields,
:syntax,
:optional_fields,
:enum_field_types,
:oneof_selection_fields

def initialize(message, toplevel_enums, generate_types:, requires:, syntax:, options:)
@message = message
Expand Down Expand Up @@ -160,32 +298,7 @@ def class_body
end

def conversion
fields = self.fields.reject do |field|
field.has_oneof_index? && !field.optional?
end

oneofs = @oneof_selection_fields.map do |field|
"send(#{field.name.dump}).tap { |f| result[f.to_sym] = send(f) if f }"
end

<<~RUBY
#{type_signature(returns: "T::Hash[Symbol, T.untyped]")}
def to_h
result = {}
#{(oneofs + fields.map { |field| convert_field(field) }).join("\n")}
result
end
RUBY
end

def convert_field(field)
if field.repeated?
"result['#{field.name}'.to_sym] = #{field.iv_name}"
elsif field.type == :TYPE_MESSAGE
"result['#{field.name}'.to_sym] = #{field.iv_name}.to_h"
else
"result['#{field.name}'.to_sym] = #{field.iv_name}"
end
HashSerializationCompiler.result(message:, fields:, oneof_selection_fields:, generate_types:)
end

def encode
Expand Down
11 changes: 10 additions & 1 deletion lib/protoboeuf/decorated_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,16 @@ class DecoratedField

extend Forwardable

def_delegators :@original_field, :name, :label, :type_name, :type, :number, :options, :oneof_index, :has_oneof_index?
def_delegators :@original_field,
:name,
:label,
:type_name,
:type,
:number,
:options,
:oneof_index,
:has_oneof_index?,
:json_name
davebenvenuti marked this conversation as resolved.
Show resolved Hide resolved

def initialize(field:, message:, syntax:)
@original_field = field
Expand Down
20 changes: 18 additions & 2 deletions lib/protoboeuf/google/protobuf/any.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion lib/protoboeuf/google/protobuf/boolvalue.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion lib/protoboeuf/google/protobuf/bytesvalue.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading