Add support to render with a root key and meta attributes #135

ritikesh · 2019-02-07T21:09:27Z

Since Most JSON specs allow a root key to be configurable, we should allow that as well.
Added option to support passing meta information along with the root element

ritikesh · 2019-02-07T21:38:42Z

@mcclayton Hey, I've got another one for you guys! Let me know your thoughts. 😄

lib/blueprinter/base.rb

AllPurposeName · 2019-02-07T22:05:19Z

lib/blueprinter/base.rb

@@ -167,6 +167,11 @@ def self.association(method, options = {}, &block)
    # @option options [Symbol] :view Defaults to :default.
    #   The view name that corresponds to the group of
    #   fields to be serialized.
+    # @option options [Symbol|String] :root Defaults to nil.


This is not enforced, currently [Any] :root is more accurate.

Wouldn't [Any] refer to any object whereas, we would only allow for a symbol / string as a hash key? Documentation should reflect on what is the expected input, I wouldn't want to add a is_a check in the method for that sake.

I'd rather deliberately decide to be open to different input or not. A type check is completely reasonable if we want to restrict to symbol/string. Is there a reason you don't want to enforce typing here?

A type coercion does happens regardless in .render and .render_as_json, but not in .render_as_hash. This is because the underlying .as_json and .jsonify turn { 1 => "hello" } into { "1" => "hello"}. I think it's confusing and dishonest to allow that to happen while .render_as_hash's underlying .prepare_for_render leaves us with the original { 1 => "hello" }.

I'm torn here because I don't think it would be wise for anyone to pass in an array, hash, or any random object, but technically now they can and it will not blow up. In those cases I would prefer to either document and fully support any input or restrict the input and treat it as an error and handle it gracefully by returning an instructive error message. One of my biggest pains migrating to AMS v0.10 was the lack of helpful error messages and confusion about the breaking changes surrounding root behavior specifically...

So let's look at some of the behavior this introduces:

# Hash as root with render_as_hash # Expected? UserBlueprint.render_as_hash( user.where(id: 1, name: "wat"), root: {"My object as a root" => "yeah it's crazy"} ) { {"My object as a root" => "yeah it's crazy"} => { "id": 1, "name": "wat" } }

# Hash as root with vanilla render # Unexpected behavior of object becoming a string and then a key in the response. UserBlueprint.render( User.where(id: 1, name: "wat"), root: {"My object as a root" => "yeah it's crazy"} ) { "{\"My object as a root\"=>\"yeah it's crazy\"}": { "id": 1, "name": "wat" } }

# Integer as root with render_as_hash # Expected because it's a hash and maintains type UserBlueprint.render_as_hash( User.where(id: 1, name: "wat"), root: 1 ) { 1: { "id": 1, "name": "wat" } }

# Integer as root with vanilla render # Possibly unexpected coercion rather than an exception being thrown UserBlueprint.render( User.where(id: 1, name: "wat"), root: 1 ) { "1": { "id": 1, "name": "wat" } }

I think it's confusing and dishonest to allow that to happen

Overall, I understand where you are coming from, but I would disagree here, because the method definitions/documentations both suggest a json or json like output - which is pretty much self-explanatory to have strings as keys in the output.

Is there a reason you don't want to enforce typing here?

Adding extra .is_a?(Symbol) || .is_a?(String) for every render call does seem like an unnecessary overhead for something that should be the implementers' job. I'm however, okay with changing the type to Any in the comments.

I'm torn here because I don't think it would be wise for anyone to pass in an array, hash, or any random object

However, I still feel strongly about being indicative and not strict in this case - that you need to pass a symbol/string - This would cover most use-cases anyway.

On the contrary, I agree with you, we need to have proper documentation and exceptions being raised when those are not followed. Have made the changes along with some refactoring. Please let me know if you have any other concerns.

@AllPurposeName I think its ok to have it as [Symbol|String] because that is what we want to support. We don't want to support [Any] even though it is technically possible in this implementation.

Users can of course pass in Array or Hash or some other crazy type, and they would do so at their own risk. If we don't document passing in those types, we will not be responsible for breaking it in future updates.

@AllPurposeName, @philipqnguyen, I've retained the Symbol | String documentation and have added a check to raise exception if anything else is passed for root along with a test case for the same.

I agree with @AllPurposeName that it'd be counter intuitive not raising an exception when calling out that limited types are supported.

Along with that, I've done some other refactoring as well. The base class was becoming heavier with lot of helper methods and active record helper has not grown as it was anticipated. Hence, I have combined them into one and have moved all private helper methods there. You can refer to my most recent commit.

Let me know your thoughts.

lib/blueprinter/base.rb

CHANGELOG.md

philipqnguyen · 2019-02-16T00:59:50Z

@ritikesh thank you for another PR!

I wonder if we can specify root key and/or meta data in the blueprint class itself?

Maybe root key would be specified like this?

class UserBlueprint
  root :users
  fields :first_name, last_name, :email
end

Do you agree?

As for meta, I'm not quite sure I like the meta being passed in to render as is as well.

philipqnguyen · 2019-02-16T01:21:03Z

On second thought, passing in root and meta to render provides better control... Scratch my previous comment =D

ritikesh · 2019-02-16T07:00:39Z

@philipqnguyen I agree. That was the intention behind passing them to render. Better control and easier manipulation. Apart from plural differences (user for singular, users for plural), for us, root is different in different use-cases as well.

ritikesh · 2019-02-25T07:43:24Z

Hi guys, any updates on this?

AllPurposeName · 2019-02-27T22:07:01Z

lib/blueprinter/helpers/base_helpers.rb

@@ -0,0 +1,101 @@
+module Blueprinter


💯 this is a great step in organization!

AllPurposeName · 2019-02-27T22:09:23Z

lib/blueprinter/helpers/base_helpers.rb

+      def prepare_for_render(object, options)
+        view_name = options.delete(:view) || :default
+        root = options.delete(:root)
+        symbol_or_string_or_nil?(root, :root)


Not blocking: Not a fan of this method name, but I like what it does!

AllPurposeName

Thanks for hashing it out to get to an even better solution.

spec/integrations/shared/base_render_examples.rb

mcclayton

Awesome, looks great to me -- thanks so much for your work on this @ritikesh!
Just want one more re-review from @AllPurposeName or @philipqnguyen and I'll merge / release the this new Blueprinter version 🎉

ritikesh added 4 commits February 8, 2019 02:24

add support for root elements

188839b

make root optional - prepare is used from association_extractor as well

63244e6

bump version and changelog

f2825d7

add support for meta attributes

2b7a4ec

ritikesh changed the title ~~Optionally, render with a root key~~ Add support to render with a root key and meta attributes Feb 7, 2019

update comments

30fdf2f

AllPurposeName reviewed Feb 7, 2019

View reviewed changes

lib/blueprinter/base.rb Outdated Show resolved Hide resolved

AllPurposeName reviewed Feb 7, 2019

View reviewed changes

lib/blueprinter/base.rb Outdated Show resolved Hide resolved

mcclayton reviewed Feb 7, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mcclayton requested review from philipqnguyen and wai-procore February 7, 2019 22:12

ritikesh added 2 commits February 8, 2019 03:46

fix changelog, hoist data variable

2cac094

cleanup prepare method and separate concerns

4f60b74

philipqnguyen mentioned this pull request Feb 16, 2019

Set root node for output JSON #134

Closed

refactoring

92d3258

AllPurposeName reviewed Feb 27, 2019

View reviewed changes

lib/blueprinter/helpers/base_helpers.rb

@@ -0,0 +1,101 @@

module Blueprinter

Copy link

Contributor

AllPurposeName Feb 27, 2019

Choose a reason for hiding this comment

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

💯 this is a great step in organization!

AllPurposeName reviewed Feb 27, 2019

View reviewed changes

AllPurposeName approved these changes Feb 27, 2019

View reviewed changes

Merge branch 'master' into fork_master

e27a82f

philipqnguyen approved these changes Feb 28, 2019

View reviewed changes

mcclayton reviewed Mar 1, 2019

View reviewed changes

spec/integrations/shared/base_render_examples.rb Outdated Show resolved Hide resolved

raise error for meta+root

b75b0a2

mcclayton approved these changes Mar 1, 2019

View reviewed changes

AllPurposeName approved these changes Mar 1, 2019

View reviewed changes

mcclayton merged commit 1ff82d0 into procore-oss:master Mar 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to render with a root key and meta attributes #135

Add support to render with a root key and meta attributes #135

ritikesh commented Feb 7, 2019 •

edited

Loading

ritikesh commented Feb 7, 2019 •

edited

Loading

AllPurposeName Feb 7, 2019

ritikesh Feb 7, 2019 •

edited

Loading

AllPurposeName Feb 20, 2019

ritikesh Feb 21, 2019

ritikesh Feb 21, 2019

philipqnguyen Feb 21, 2019 •

edited

Loading

ritikesh Feb 22, 2019

philipqnguyen commented Feb 16, 2019 •

edited

Loading

philipqnguyen commented Feb 16, 2019

ritikesh commented Feb 16, 2019 •

edited

Loading

ritikesh commented Feb 25, 2019

AllPurposeName Feb 27, 2019

AllPurposeName Feb 27, 2019

AllPurposeName left a comment

mcclayton left a comment

Add support to render with a root key and meta attributes #135

Add support to render with a root key and meta attributes #135

Conversation

ritikesh commented Feb 7, 2019 • edited Loading

ritikesh commented Feb 7, 2019 • edited Loading

AllPurposeName Feb 7, 2019

Choose a reason for hiding this comment

ritikesh Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

AllPurposeName Feb 20, 2019

Choose a reason for hiding this comment

ritikesh Feb 21, 2019

Choose a reason for hiding this comment

ritikesh Feb 21, 2019

Choose a reason for hiding this comment

philipqnguyen Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

ritikesh Feb 22, 2019

Choose a reason for hiding this comment

philipqnguyen commented Feb 16, 2019 • edited Loading

philipqnguyen commented Feb 16, 2019

ritikesh commented Feb 16, 2019 • edited Loading

ritikesh commented Feb 25, 2019

AllPurposeName Feb 27, 2019

Choose a reason for hiding this comment

AllPurposeName Feb 27, 2019

Choose a reason for hiding this comment

AllPurposeName left a comment

Choose a reason for hiding this comment

mcclayton left a comment

Choose a reason for hiding this comment

ritikesh commented Feb 7, 2019 •

edited

Loading

ritikesh commented Feb 7, 2019 •

edited

Loading

ritikesh Feb 7, 2019 •

edited

Loading

philipqnguyen Feb 21, 2019 •

edited

Loading

philipqnguyen commented Feb 16, 2019 •

edited

Loading

ritikesh commented Feb 16, 2019 •

edited

Loading