Add dynamic field support #164

amalarayfreshworks · 2019-07-04T08:19:37Z

We have a use case in our product where the key names of the result hash are defined at run time, so in order to avoid introducing an unnecessary extra level/key, I have made some changes.

I have introduced one custom field option called "merge". If this option is present we can add the value returned by the "method/name" directly to the obj hash through a merge.

Eg :
let(:blueprint) do
Class.new(Blueprinter::Base) do
identifier :id
field :dynamic_fields, merge: true
end
end
def dynamic_fields
{"field1" => "val1"}
end
blueprint.render(obj) would return '{"id":' + obj_id + ', "field1": "val1"}'
instead of
'{"id":' + obj_id + ', "dynamic_fields": {"field1": "val1"}}'

AllPurposeName · 2019-07-08T17:33:11Z

spec/factories/model_factories.rb

@@ -3,7 +3,7 @@
 FactoryBot.define do
  factory :user do
    first_name { 'Meg' }
-    last_name  { 'Jones' }
+    last_name  { 'Ryan' }


philipqnguyen

I'm not sure if I like this merge option. I don't think it explains itself very well what merge is doing without context.

I think the second proposal of what @ritikesh mentioned in the gitter here https://gitter.im/blueprinter-gem/community?at=5d1c81b2f0f22f66450825c4 would fall more in line with current blueprinter's APIs.

Considering that we already have existing features like this:

field :something, if: -> { ... }

so doing something like

field :something, name: -> { ... }

would be pretty self explanatory because it sticks with current patterns.

amalarayfreshworks · 2019-07-09T11:10:26Z

@philipqnguyen But aren't we giving additional responsibility to the attribute name, by giving a proc which will return name and value. If at all in future, if we have to support a feature where you can dynamically define the name/key of the hash, won't this approach be ideal for that scenario, where you return the dynamic name through the proc?

philipqnguyen · 2019-07-09T18:42:06Z

@amalarayfreshworks I'm not sure if I fully understand.

In this case:

field :something, name: -> { |obj, _opt| obj.dynamic_field_name }

The field :something would be responsible for accessing the value, where as name: -> { |obj, _opt| obj.dynamic_field_name } would evaluate to a new name. I don't see the name proc ever evaluating for both name and value.

Or am I missing something or not understanding you?

ritikesh · 2019-07-09T20:02:55Z

@philipqnguyen can you pls check Gitter? She has elaborated the use-case there.

amalarayfreshworks · 2019-07-10T05:50:28Z

I have elaborated the usecase in gitter https://gitter.im/blueprinter-gem/community?at=5d24b57317cc7b05ca9c8249. Please do let me know how can we implement the use case by being in line with current blueprinter's APIs.

mcclayton · 2019-07-10T18:25:01Z

The team has been deliberating on how we’d like to address something like this.
We’ve come up with a proposal that we think is generic enough to handle your use case, as well as some others that we’ve been thinking through (such as #158). Would the following suffice your use case?

We are thinking of adding a new Transformer class
i.e.

module Blueprinter
  class Transformer
    attr_reader :primary_obj, :hash, :options

    def initialize(hash, primary_obj, options={})
      @primary_obj = primary_obj
      @hash = hash
      @options = options
    end

    def transform
      fail NotImplementedError, "A Transformer must implement #transform"
    end
  end
end

Users can then implement transformer classes and declare them in a Blueprint as follows:

class UserBlueprint < Blueprinter::Base
  fields :first_name, :last_name

  transform DynamicFieldTransformer
end

class DynamicFieldTransformer < Blueprinter::Transformer
  def transform
    hash.merge!(primary_obj.dynamic_fields)
  end
end

Whatever is returned by the set transform will be the resulting hash passed to serialization (to_json).

amalarayfreshworks · 2019-07-15T05:40:08Z

Can anyone please re trigger the build for ruby 2_5 and ruby 2_4 as the build failed at installing ruby dependencies.

@philipqnguyen

philipqnguyen

Thanks for doing this!
Some comments!

lib/blueprinter/transformer.rb

philipqnguyen · 2019-07-17T00:13:52Z

lib/blueprinter/view.rb

@@ -38,6 +43,10 @@ def exclude_fields(field_names)
      end
    end

+    def *(custom_transformer)
+      transformers << custom_transformer
+    end


I've never seen * used for anything but multiplication. Is this common style?

Otherwise, can we come up with a method name that makes clear that it is appending transformer here?
I'm worried we will run out of symbols to define new methods 🤣

I am usually not into operator overloading, but then I was inspired by the method written for adding fields into the view
def <<(field)
fields[field.name] = field
end
I was under the impression that its a style blueprinter follows for adding setters. I agree to all the reviewers @philipqnguyen and @mcclayton when you feel that we can run out of operators to overload. For now I will change the method name to transform :)

can you rename to add_transformer? Otherwise it seems like view.transform is already invoking the transformer, which is not entirely accurate since it's just storing it to be called later.

philipqnguyen

thanks @amalarayfreshworks a couple more comments, but overall looking great.

philipqnguyen · 2019-07-23T17:21:35Z

lib/blueprinter/view.rb

@@ -22,6 +23,10 @@ def inherit(view)
      view.excluded_field_names.each do |field_name|
        exclude_field(field_name)
      end
+
+      view.transformers.each do |transformer|
+        self * transformer


you'll want to change this * to the new method name.

philipqnguyen · 2019-07-23T17:23:01Z

lib/blueprinter/view.rb

@@ -38,6 +43,10 @@ def exclude_fields(field_names)
      end
    end

+    def *(custom_transformer)
+      transformers << custom_transformer
+    end


can you rename to add_transformer? Otherwise it seems like view.transform is already invoking the transformer, which is not entirely accurate since it's just storing it to be called later.

philipqnguyen · 2019-07-23T17:50:25Z

spec/integrations/shared/base_render_examples.rb

+      end
+    end
+    it('returns json with values derived from options') { should eq(result) }
+  end


Would you also add a test to ensure that the transformers are also inherited? There's some inheritance specs here that you can add to: https://github.com/procore/blueprinter/blame/4ba12dd14edf340f9a4a95f1668785adc8c21ca6/spec/integrations/base_spec.rb#L326-L348

Test case has been added.

1. Changed setter of transformer from “transform” to “add_transformer” 2. Invoked add_transformer instead of * in view inherit. 3. Added test cases to verify transformer inheritance

philipqnguyen

LGTM

mcclayton

🚀

tjwallace · 2019-08-09T16:23:46Z

lib/blueprinter/base.rb

+    #     # other code
+    #   end
+    #
+    #   class DynamicFieldTransformer < Blueprinter::Transformer


Should this be DynamicTransformer? (or transform DynamicFieldTransformer above)?

Yup, example mismatch. would appreciate if you can make a PR.

Done, see #174

amalarayfreshworks added 2 commits July 4, 2019 13:41

Add dynamic field support

b95e423

Changed the option as merge instead of flatten

15ff212

AllPurposeName reviewed Jul 8, 2019

View reviewed changes

philipqnguyen reviewed Jul 8, 2019

View reviewed changes

Instead of merge at field level - transformer at view level

95ab65a

philipqnguyen mentioned this pull request Jul 17, 2019

More possibilities with collections for easier composing of complex JSON #166

Closed

philipqnguyen reviewed Jul 17, 2019

View reviewed changes

amalarayfreshworks added 3 commits July 17, 2019 11:18

changing * to transform

8554022

version change

cf96ba1

reverted the version change

3bfe5c6

philipqnguyen suggested changes Jul 23, 2019

View reviewed changes

philipqnguyen reviewed Jul 23, 2019

View reviewed changes

Transformer changes

97a97c9

1. Changed setter of transformer from “transform” to “add_transformer” 2. Invoked add_transformer instead of * in view inherit. 3. Added test cases to verify transformer inheritance

philipqnguyen approved these changes Jul 24, 2019

View reviewed changes

mcclayton approved these changes Jul 24, 2019

View reviewed changes

mcclayton merged commit 6cb2784 into procore-oss:master Jul 24, 2019

mcclayton mentioned this pull request Jul 24, 2019

Draft release for v0.19.0 #169

Merged

tjwallace reviewed Aug 9, 2019

View reviewed changes

supremebeing7 mentioned this pull request Jan 30, 2020

Any way to configure/perform something like key_transform in AMS? #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic field support #164

Add dynamic field support #164

amalarayfreshworks commented Jul 4, 2019 •

edited

Loading

AllPurposeName Jul 8, 2019

philipqnguyen left a comment

amalarayfreshworks commented Jul 9, 2019 •

edited

Loading

philipqnguyen commented Jul 9, 2019 •

edited

Loading

ritikesh commented Jul 9, 2019

amalarayfreshworks commented Jul 10, 2019

mcclayton commented Jul 10, 2019 •

edited

Loading

amalarayfreshworks commented Jul 15, 2019 •

edited

Loading

philipqnguyen left a comment

philipqnguyen Jul 17, 2019

amalarayfreshworks Jul 17, 2019

philipqnguyen Jul 23, 2019

philipqnguyen left a comment

philipqnguyen Jul 23, 2019

philipqnguyen Jul 23, 2019

philipqnguyen Jul 23, 2019 •

edited

Loading

amalarayfreshworks Jul 23, 2019

philipqnguyen left a comment

mcclayton left a comment

tjwallace Aug 9, 2019

philipqnguyen Aug 9, 2019

tjwallace Aug 9, 2019

Add dynamic field support #164

Add dynamic field support #164

Conversation

amalarayfreshworks commented Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

philipqnguyen left a comment

Choose a reason for hiding this comment

amalarayfreshworks commented Jul 9, 2019 • edited Loading

philipqnguyen commented Jul 9, 2019 • edited Loading

ritikesh commented Jul 9, 2019

amalarayfreshworks commented Jul 10, 2019

mcclayton commented Jul 10, 2019 • edited Loading

amalarayfreshworks commented Jul 15, 2019 • edited Loading

philipqnguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipqnguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipqnguyen Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipqnguyen left a comment

Choose a reason for hiding this comment

mcclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amalarayfreshworks commented Jul 4, 2019 •

edited

Loading

amalarayfreshworks commented Jul 9, 2019 •

edited

Loading

philipqnguyen commented Jul 9, 2019 •

edited

Loading

mcclayton commented Jul 10, 2019 •

edited

Loading

amalarayfreshworks commented Jul 15, 2019 •

edited

Loading

philipqnguyen Jul 23, 2019 •

edited

Loading