Hash Serialization #182
Hash Serialization #182
Conversation
davebenvenuti
force-pushed
the
davebenvenuti/hash-serialization
branch
4 times, most recently
from
14e30fa to
4d9a090
Compare
davebenvenuti
added
the
#gsd:43107
davebenvenuti
force-pushed
the
davebenvenuti/hash-serialization
branch
from
4d9a090 to
161ec23
Compare
There was a problem hiding this comment.
Left a few questions.
Overall seems good, but there may be some improvements we want to make (now or later) for performance.
One thing I see missing for compatibility is that fields of type bytes should be base64 encoded in the JSON output, but that could be added later.
|
|
||
| "result[#{key}] = #{value} if send(:#{oneof_selection_field_name}) == :#{field_name}" | ||
| elsif field.repeated? | ||
| "#{value}.tap { |v| result[#{key}] = v if !options[:compact] || v.any? }" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Indeed. I was thinking maybe we could use something like tmp_#{field.lvar} = #{value} and then check the tmp var 🤷
| elsif field.repeated? | ||
| "#{value}.tap { |v| result[#{key}] = v if !options[:compact] || v.any? }" | ||
| elsif field.optional? | ||
| "result[#{key}] = #{value} if !options[:compact] || has_#{field.name}?" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like the idea of putting predicate_name on the Field class.
| result[:"enum_1"] = @enum_1 if send(:"oneof_field") == :"enum_1" | ||
| result[:"enum_2"] = @enum_2 if send(:"oneof_field") == :"enum_2" |
There was a problem hiding this comment.
This feels a bit wasteful (calling send(key) multiple times) compared to the old version
| 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}" |
There was a problem hiding this comment.
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.
| 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}" |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Makes sense. self.#{...} is probably better.
Co-authored-by: Randy Stauner <[email protected]>
| result["value".to_sym] = @value | ||
|
|
||
| result[:"type_url"] = @type_url | ||
| result[:"value"] = @value |
There was a problem hiding this comment.
I don't know why that to_sym call was there to begin with, this looks a little funny to me. result[:type_url] and result[:value] would be much more natural.
There was a problem hiding this comment.
I left the quotes just in case the field names have anything funky in them that might produce a syntax error. But I'm not sure if that's even possible given the constraints on proto field names.
|
|
||
| def to_json(options = {}) | ||
| require "json" | ||
| JSON.dump(as_json(options)) |
There was a problem hiding this comment.
Can we rename options to as_json_options please? I would otherwise think options applies to formatting the JSON output.
| result = {} | ||
|
|
||
| result["intField"] = @int_field | ||
| result["stringField"] = @string_field if !options[:compact] || |
There was a problem hiding this comment.
Maybe it's worth adding a helper method:
# fallback is probably the wrong name, but you get the idea
def should_compact?(options, fallback)
options[:compact] || fallback
# or maybe:
# @compact_export_keys ||= options[:compact]
# @compact_export_keys || fallback
endI'm generally a fan of duplicating small fragments if it makes the code easier to read. But, in this case I think the lines are getting a bit long.
|
I got bit by this 😭 isaacs/github#361 I'll reopen |
This PR introduces a new
HashSerializationCompilerwhich generatesto_h,as_json, andto_jsonmethods for our generated code.to_hworks mostly as it did before except it now recursively callsto_hon Message values.as_jsonworks similarly, except it uses thejson_nameas the Hash key for a givenFieldinstead of the field name. This value can be overridden in proto definitions andprotocprovides a default value if none is specified. This functionality is also required to pass the conformance suite one day. Additionally, it supports acompactoption, which was requested for an internal use case, which will excludeoptionalvalues which aren't set as well as empty arrays (repeated) or maps from the output if set to true.to_jsonbasically just callsas_jsonand then uses thejsonlibrary to stringify the result, passing along and supporting the samecompactoption.