Do not overwrite overridden fields and associations

[elliothursh]

Description

First off, thank y'all for adding this new reflection feature. It's super useful! I ran into an issue with fields and categories that exist in the default view but are overridden in other views. Reflections seem to get overwritten by their definitions in the default view instead of the other view. This PR changes the behavior to not overwrite overridden fields and associations with definitions in the default view.

Checklist:

• I have updated the necessary documentation
• I have signed off all my commits as required by DCO
• My build is green

[elliothursh]

@jhollinger I also ran into another issue around duplicate field aliases, but I couldn't think of a good way to solve in the reflection code without a breaking change and have a workaround. I think it's a pretty big edge case, but wondering if you had any thoughts?

In a blueprint defined like:

class FooBlueprint < Blueprinter::Base
  field :name, name: :label
  field :name, name: :value
end

The field second field doesn't get defined in the reflection's fields:

FooBlueprint.reflections[:default].fields
=> {:name=>#<struct Blueprinter::Reflection::Field name=:name, display_name=:value, options={:name=>:value, :block=>nil}>}

Even though it's defined in the Reflection::View.

FooBlueprint.reflections[:default]
=>
#<Blueprinter::Reflection::View:0x000000010f673530
 @fields={:name=>#<struct Blueprinter::Reflection::Field name=:name, display_name=:value, options={:name=>:value, :block=>nil}>},
 @name=:default,
 @view_collection=
  #<Blueprinter::ViewCollection:0x000000010f674020
   @sort_by_definition=false,
   @views=
    {:identifier=>#<Blueprinter::View:0x000000010ffb1bd8 @definition_order=[], @excluded_field_names=[], @fields={}, @included_view_names=[], @name=:identifier, @sort_by_definition=false, @view_transformers=[]>,
     :default=>
      #<Blueprinter::View:0x000000010ffb1b88
       @definition_order=[],
       @excluded_field_names=[],
       @fields=
        {:value=>
          #<Blueprinter::Field:0x000000010ffb1ae8
           @blueprint=FooBlueprint,
           @extractor=...,
           @method=:name,
           @name=:value,
           @options={:name=>:value, :block=>nil}>,
         :label=>
          #<Blueprinter::Field:0x000000010ffb1a48
           @blueprint=FooBlueprint,
           @extractor=...,
           @method=:name,
           @name=:label,
           @options={:name=>:label, :block=>nil}>},
       @included_view_names=[],
       @name=:default,
       @sort_by_definition=false,
       @view_transformers=[]>}>>

I think in order to resolve, I'd have to change the fields/associations to be keyed off of field.name instead of field.method which would break existing usage for a lot of people.

[jhollinger]

Good catch, thanks! However, I'm surprised that ViewCollection#fields_for is returning both the default and overridden fields. Other uses of fields_for aren't doing this kind of check [1], [2]. Is there something different about those use cases I'm not seeing, or is the bug we need to fix actually in fields_for?

[jhollinger]

@elliothursh Your other comment reminds me of #281 (which is old, but there have been recent discussions around it). It would probably make more sense for the reflection hashes to be keyed off of name, not method. Agreed it's technically a breaking change, but it's also broken as-is, so...yeah.

[elliothursh]

I think fields_for is working correctly.

With a Blueprint defined like this:

class FooBlueprint < Blueprinter::Base
  field :name

  view :bar do
    field :name, name: :label
    field :name, name: :value
  end
end

It outputs this:

FooBlueprint.render_as_hash(OpenStruct.new(name: "NAME"), view: :bar)
=> {:label=>"NAME", :name=>"NAME", :value=>"NAME"}

I think the the Blueprinter::Views.fields_for behavior is correct to also return the default fields fields in other views. I think it works because in Blueprinter::Views fields are keyed off of the field name. So I kind of think the main issue is just that reflections are being keyed off of method.

Another example:

class  FooBlueprint < Blueprinter::Base
  field :name

  view :bar do
    field :baz, name: :name
    field :name, name: :value
  end
end

I'd expect that the render output json overrides the name key with the value in :baz, which does happen.

FooBlueprint.render_as_hash(OpenStruct.new(name: "NAME", baz: "BAZ"), view: :bar)
=> {:name=>"BAZ", :value=>"NAME"}

[elliothursh]

I made a new branch with the change use field.name, and added tests for aliases (first example above) and overrides (second example above).
https://github.com/procore-oss/blueprinter/compare/main...elliothursh:blueprinter:reflection-use-name?expand=1

Not sure how/if ya'll want to move forward with a breaking change like this, but let me know!

[jhollinger]

@elliothursh The reflection feature is new enough (a couple weeks) and currently undocumented enough, that I'd personally be okay with calling the "breaking change" in your reflection-use-name branch a "bugfix with notes." Curious how others feel.

Longer term, I do think the lib has some...challenges...in how fields are stored and fetched internally. But I don't think that should stand in the way of this or reflection-use-name.

[elliothursh]

If you're okay with calling it a 'bugfix with notes', so am I! I'll close this PR and open one for reflection-use-name instead.