-
Notifications
You must be signed in to change notification settings - Fork 362
Implement Base#encode in terms of format.encode
#417
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
Implement Base#encode in terms of format.encode
#417
Conversation
test/fixtures/person.rb
Outdated
| def load(attributes, *args) | ||
| attributes = attributes.deep_transform_keys { |key| key.to_s.underscore } | ||
|
|
||
| super | ||
| end | ||
|
|
||
| def serializable_hash(options = {}) | ||
| super.deep_transform_keys! { |key| key.camelcase(:lower) } | ||
| 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.
Is this the best way to achieve this outcome? When learning about the library, my first instinct was to tackle name casing at the JsonFormat level.
Unfortunately, since #encode is implemented in terms of #to_json, and not JsonFormat.encode, a custom format that inherited from JsonFormat had no effect.
Is there a more canonical way to extend or configure the pre-existing JsonFormat module to more elegantly handle this?
Could it be worthwhile to explore changing the Base#encode implementation to incorporate self.class.format.encode?
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.
Could it be worthwhile to explore changing the Base#encode implementation to incorporate self.class.format.encode?
I think so. I'd just make possible to have a custom format and deal with it instead of documenting to add two overrides.
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.
@rafaelfranca I've re-purposed this commit to create an opportunity for consumers to provide a custom format.
test/fixtures/person.rb
Outdated
| def load(attributes, *args) | ||
| attributes = attributes.deep_transform_keys { |key| key.to_s.underscore } | ||
|
|
||
| super | ||
| end | ||
|
|
||
| def serializable_hash(options = {}) | ||
| super.deep_transform_keys! { |key| key.camelcase(:lower) } | ||
| 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.
Could it be worthwhile to explore changing the Base#encode implementation to incorporate self.class.format.encode?
I think so. I'd just make possible to have a custom format and deal with it instead of documenting to add two overrides.
1fc2f60 to
5a94309
Compare
Base#encode in terms of format.encode
The problem
---
There isn't a conventional way to transform a resource's attributes
before they're sent to a remote server. For example, consider a resource
that needs to transform snake_case attribute keys (which are idiomatic
to Ruby's hash keys) into camelCase keys for a service's JSON API prior
to sending them as a request payload.
Similarly, consider transforming a response payload's camelCase keys
back into snake_case Hash instances to be loaded as attributes.
The proposal
---
Prior to this commit, the `Base#encode` method *did not* utilize the
resource class' configured format's `encode` method. Instead, it
relied on the format's `#extension` to invoke the appropriate method
(for example, `"xml"` would invoke `#to_xml`, `"json"` would invoke
`#to_json`, a hypothetical `"msgpack"` custom format would invoke a
hypothetical `#to_msgpack` method, etc.)
Since `#to_json` and `#to_xml` (and presumable `#to_msgpack`) result in
already-encoded String values, there isn't an opportunity for consumers
to transform keys. This means they're responsible for being familiar
with the underlying method invocation's interface (for example, that
`to_json` calls `as_json`, and that `as_json` calls
`serializable_hash`), or they're responsible for decoding and
re-encoding after the modifications.
To resolve that issue, this commit modifies the `Base#encode` method to
delegate to the configured format's `#encode` method. To preserve
backwards compatibility and to continue to support out-of-the-box
behavior, this commit ensures that those formats invoke the appropriate
method on the resource instance (`to_xml` for `XmlFormat`, `to_json` for
`JsonFormat`). This change both simplifies the implementation (by
removing the `send("to_#{format.extension}", ...)` metaprogramming)
*and* introduces a seam for consumers to override behavior.
For example, consumers can now declare customized formatters to serve
their encoding and decoding needs (like a
snake_case->camelCase->snake_case chain):
```ruby
module CamelcaseJsonFormat
extend ActiveResource::Formats[:json]
def self.encode(resource, options = nil)
hash = resource.as_json(options)
hash = hash.deep_transform_keys! { |key| key.camelcase(:lower) }
super(hash)
end
def decode(json)
hash = super
hash.deep_transform_keys! { |key| key.underscore }
end
end
Person.format = CamelcaseJsonFormat
person = Person.new(first_name: "First", last_name: "Last")
person.encode
# => "{\"person\":{\"firstName\":\"First\",\"lastName\":\"Last\"}}"
Person.format.decode(person.encode)
# => {"first_name"=>"First", "last_name"=>"Last"}
```
5a94309 to
0342b20
Compare
| # module CamelcaseJsonFormat | ||
| # extend ActiveResource::Formats[:json] | ||
| # | ||
| # def self.encode(resource, options = nil) | ||
| # hash = resource.as_json(options) | ||
| # hash = hash.deep_transform_keys! { |key| key.camelcase(:lower) } | ||
| # super(hash) | ||
| # end | ||
| # | ||
| # def decode(json) | ||
| # hash = super | ||
| # hash.deep_transform_keys! { |key| key.underscore } | ||
| # 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.
This PR makes this possible, but I still think this use case (snake_case to camelCase to snake_case) is common enough that it deserves a conventional solution.
I'm happy to explore this further in the future, but I just wanted to share my thoughts here for later.
|
|
||
| def encode(hash, options = nil) | ||
| ActiveSupport::JSON.encode(hash, options) | ||
| def encode(resource, options = nil) |
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.
Funny. This method wasn't used since 228fb73.
The problem
There isn't a conventional way to transform a resource's attributes
before they're sent to a remote server. For example, consider a resource
that needs to transform snake_case attribute keys (which are idiomatic
to Ruby's hash keys) into camelCase keys for a service's JSON API prior
to sending them as a request payload.
Similarly, consider transforming a response payload's camelCase keys
back into snake_case Hash instances to be loaded as attributes.
The proposal
Prior to this commit, the
Base#encodemethod did not utilize theresource class' configured format's
encodemethod. Instead, itrelied on the format's
#extensionto invoke the appropriate method(for example,
"xml"would invoke#to_xml,"json"would invoke#to_json, a hypothetical"msgpack"custom format would invoke ahypothetical
#to_msgpackmethod, etc.)Since
#to_jsonand#to_xml(and presumable#to_msgpack) result inalready-encoded String values, there isn't an opportunity for consumers
to transform keys. This means they're responsible for being familiar
with the underlying method invocation's interface (for example, that
to_jsoncallsas_json, and thatas_jsoncallsserializable_hash), or they're responsible for decoding andre-encoding after the modifications.
To resolve that issue, this commit modifies the
Base#encodemethod todelegate to the configured format's
#encodemethod. To preservebackwards compatibility and to continue to support out-of-the-box
behavior, this commit ensures that those formats invoke the appropriate
method on the resource instance (
to_xmlforXmlFormat,to_jsonforJsonFormat). This change both simplifies the implementation (byremoving the
send("to_#{format.extension}", ...)metaprogramming)and introduces a seam for consumers to override behavior.
For example, consumers can now declare customized formatters to serve
their encoding and decoding needs (like a
snake_case->camelCase->snake_case chain):