Feature proposal: Honor all conditional checks config - path for sparse fields #308

alejandroereyes · 2023-07-18T20:50:02Z

Background

At my company we make liberal use of views. But there are times that I find them constricting when trying to optimize payload sizes. I would like to have more granular control over what is returned. I propose adding a configuration to honor all conditional checks. This would allow for a lightweight implementation for sparse fields support over our API.

Example:

Blueprinter.configure do |config|
  config.roll_up_conditions = true
  config.if = ->(field_name, object, local_opts) {
    if local_opts[:sparse_fields]
      local_opts[:sparse_fields].any? { |sfield| sfield.to_s == field_name.to_s }
    else
      true
    end
  }
end

class SlothBlueprint < Blueprinter::Base
  identifier :uuid

  field :name
  field :favorite_food
  field :hair_color
  field :sunglasses, name: :has_sunglasses
end

class Sloth
  attr_accessor :name, :favorite_food, :hair_color, :sunglasses
  def initialize(name, food, color, sg)
    @name = name
    @favorite_food = food
    @hair_color = color
    @sunglasses = sg
  end

  def uuid
    Random.new_seed.to_s
  end
end

my_sloth = Sloth.new('Mirabel', 'bananas', 'brown', true)

SlothBlueprint.render_as_hash(my_sloth, sparse_fields: [:has_sunglasses, :favorite_food])
# => {:favorite_food => 'bananas', :has_sunglasses => true }

SlothBlueprint.render_as_hash(my_sloth)
# => {:uuid => '21342134...', :favorite_food => 'bananas', :hair_color => 'brown', :has_sunglasses => true, :name => 'Mirabel'}

Checklist:

I have updated the necessary documentation
I have updated the changelog, if necessary (CHANGELOG.md)
I have signed off all my commits as required by DCO
My build is green

njbbaer

Looks good from my perspective. I agree we should be able to combine the conditionals, and roll_up_conditions makes sense.

lib/blueprinter/field.rb

README.md

njbbaer · 2023-09-19T23:18:55Z

README.md


 ### Global Config Setting - if and unless

 ```ruby
 Blueprinter.configure do |config|
  config.if = ->(field_name, obj, _options) { !obj[field_name].nil? }
  config.unless = ->(field_name, obj, _options) { obj[field_name].nil? }
+  config.roll_up_conditions = true


@lessthanjacob suggested roll_up_conditions is a bit unclear. How about enforce_all_conditions?

Thanks for the ping! You caught me in the middle of writing a quick comment 😆

The desired behavior makes sense to me, but the name of the configuration setting gives me slight pause. Ideally it's something that you can set and forget, but I'd challenge us to think of a name that describes the functionality a bit more explicitly!

I could be on board with enforce_all_conditions. We could also codify the default behavior in the configuration, and expect that we set it to false to get the behavior detailed in this PR (e.g. field_level_condition_overrides_global).

lessthanjacob · 2023-09-19T23:58:26Z

Looking back at this again, I think it could be worth implementing explicit Sparse Field functionality here as a follow up!

- add roll_up_conditions configuration - when enabled serialization honors all conditionals when deciding to render a field - this allows for a global conditional to be set, opening the possibility for sparse_fields to be implemented Signed-off-by: alejandroereyes <[email protected]>

Co-authored-by: Nate Baer <[email protected]> Signed-off-by: Alejandro Reyes <[email protected]>

Signed-off-by: alejandroereyes <[email protected]>

alejandroereyes · 2023-09-20T15:54:03Z

@lessthanjacob , @njbbaer Goood call on the name change to enforce_all_conditions, it's much clearer. I went ahead and changed it. There were some merge conflicts that I had to resolve as well.

kkohrt

@alejandroereyes -- thanks for adding this feature!
Throwing in some ideas here.

I could go with enforce_all_conditions; or I might suggest something using the word merge; e.g. merge_field_options_with_global_options
Ideally there would be more complete test coverage of the different branches:
- field unless == true but global if == true: merge_options? ? passing : skipping
- field unless == false, but global unless == true: merge_options? ? skipping : passing
- field if == true, but global unless == false: merge_options? ? passing : passing
- invalid if or unless values for either global or field options should raise specific errors
I do like the break up of methods into more Clean Code; but I might DRY things up some more. Suggestions in-line, below

kkohrt · 2023-09-28T09:35:29Z

lib/blueprinter/field.rb

    end

    private

+    def any_global_or_field_condition_failed?(field_name, object, local_options)
+      return true if if_callables.any? { |if_call| !if_call.call(field_name, object, local_options) }


This is not very DRY. It is repeating the same logic as the original skip? method, but against a set of up to two items, rather than just a single item. I think it would be cleaner if you use the new config setting to help compose the set of conditions to be tested, and only have one method doing the actual testing.
Suggestions below. Feel free to use them, modify them, DRY them up further, or do something else.

kkohrt · 2023-09-28T10:06:56Z

lib/blueprinter/field.rb

      else
-        raise ArgumentError, "#{tmp.class} is passed to :#{condition}"
+        raise ArgumentError, "#{tmp_callable.class} is passed to :#{condition}"


I think you missed adding a test for this error case, because condition is not defined for this method--that is the argument passed to the original callable_from method.

This extraction method is nice, though. It would be nice to keep it.
A slightly clunky solution that would provide a valid error message is to pass in a 2nd argument: condition, where condition ∈ { 'field option', 'Blueprint configuration' } as provided by one of the private calling methods, above.

A better solution, I think, would be to

add explicit if and unless setter methods on Blueprinter::Configuration and raise appropriate argument errors there if not a Proc or a Symbol

add validation of the options param in the Field initializer to ensure that any :if or :unless attributes are also either a Proc or Symbol

That way you get errors sooner and with a more meaningful stack trace.

kkohrt · 2023-09-28T10:24:12Z

lib/blueprinter/field.rb

+    def unless_callables
+      [
+        extract_callable_from(Blueprinter.configuration.unless),
+        extract_callable_from(options[:unless])
+      ].select { |callable| callable }
+    end


The following modification should allow this method to be used for both the legacy conditions, and the case where enforce_all_conditions (or whatever it is called) is set to true

Suggested change

def unless_callables

[

extract_callable_from(Blueprinter.configuration.unless),

extract_callable_from(options[:unless])

].select { |callable| callable }

end

def unless_callables

callables = [extract_callable_from(options[:unless])]

if options[:unless].nil? || enforce_all_conditions

callables << extract_callable_from(Blueprinter.configuration.unless),

end

callables.compact

end

assumes extract_callable_from returns nil instead of false when passed a nil argument

and something like:

delegate :enforce_all_conditions, to: Blueprinter.configuration

kkohrt · 2023-09-28T10:37:49Z

lib/blueprinter/field.rb

+    def if_callables
+      [
+        extract_callable_from(Blueprinter.configuration.if),
+        extract_callable_from(options[:if])
+      ].select { |callable| callable }
+    end
+


The following modification should allow this method to be used for both the legacy conditions, and the case where enforce_all_conditions (or whatever it is called) is set to true

Suggested change

def if_callables

[

extract_callable_from(Blueprinter.configuration.if),

extract_callable_from(options[:if])

].select { |callable| callable }

end

def if_callables

callables = [extract_callable_from(options[:if])]

if options[:if].nil? || enforce_all_conditions

callables << extract_callable_from(Blueprinter.configuration.if),

end

callables.compact

end

assumes extract_callable_from returns nil instead of false when passed a nil argument

and something like:

delegate :enforce_all_conditions, to: Blueprinter.configuration

kkohrt · 2023-09-28T10:39:58Z

lib/blueprinter/field.rb

@@ -47,13 +71,17 @@ def callable_from(condition)
              config.public_send(condition)
            end

-      return false unless tmp
+      extract_callable_from(tmp)


This whole method can go away if all condition gathering is moved to unless_callables and if_callables

Or, you could refactor it to really DRY things up by pulling in the suggested logic from those two methods, and make this one the the do-everything method. That does have the potential to add too much complexity here, though, which is why I leaned in on the explicitly named methods that you created.

kkohrt · 2023-09-28T10:52:54Z

lib/blueprinter/field.rb

+      if Blueprinter.configuration.enforce_all_conditions
+        any_global_or_field_condition_failed?(field_name, object, local_options)
+      else
+        return true if if_callable && !if_callable.call(field_name, object, local_options)


"true if this and not this" just spins my head a little. The following suggestion is to replace the entire skip? method, though:

Basically, I might make skip? work a little more like Rails blank? and present? for readability (but in this case, skip? is !include?)

Suggested change

if Blueprinter.configuration.enforce_all_conditions

any_global_or_field_condition_failed?(field_name, object, local_options)

else

return true if if_callable && !if_callable.call(field_name, object, local_options)

def skip?(field_name, object, local_options)

!inclusion_conditions_met?(field_name, object, local_options)

end

def inclusion_conditions_met?(field, obj, locals)

if_callables.any? { |callable| callable.call(field, obj, locals) } &&

unless_callables.none? { |callable| callable.call(field, obj, locals) }

end

kkohrt · 2023-09-28T11:10:59Z

spec/integrations/shared/base_render_examples.rb

+              let(:local_options) { {x: 1, y: 2, v1: value, v2: other_value} }
+
+              before do
+                Blueprinter.configuration.enforce_all_conditions = true


Setting and resetting global configurations for testing is not an ideal practice.
Can this be done instead with

Suggested change

Blueprinter.configuration.enforce_all_conditions = true

allow(Blueprinter).to receive(:configuration.enforce_all_conditions).and_return(true)

and then, as a bonus, you don't need the after block

kkohrt · 2023-09-28T11:11:41Z

spec/integrations/shared/base_render_examples.rb

+
+              before do
+                Blueprinter.configuration.enforce_all_conditions = true
+                Blueprinter.configuration.if = ->(field_name, object, local_opts) {


Ideally this could also be stubbed with an allow condition

kkohrt · 2023-09-28T11:11:49Z

spec/integrations/shared/base_render_examples.rb

+                    value
+                  end
+                }
+                Blueprinter.configuration.unless = ->(_a,_b,_c) { other_value }


github-actions · 2024-01-18T01:50:15Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

alejandroereyes force-pushed the add_conditional_rollup branch from a87270c to 0fd0158 Compare August 22, 2023 14:26

alejandroereyes requested a review from a team as a code owner August 22, 2023 14:26

alejandroereyes force-pushed the add_conditional_rollup branch from 0fd0158 to 9eb8c9e Compare August 22, 2023 14:30

jmeridth assigned njbbaer Sep 5, 2023

njbbaer reviewed Sep 13, 2023

View reviewed changes

lib/blueprinter/field.rb Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

njbbaer previously approved these changes Sep 15, 2023

View reviewed changes

njbbaer added approved and removed approved labels Sep 19, 2023

njbbaer reviewed Sep 19, 2023

View reviewed changes

alejandroereyes dismissed njbbaer’s stale review via 1d0caac September 20, 2023 15:03

alejandroereyes and others added 4 commits September 20, 2023 10:42

remove extra line

4d1209a

Co-authored-by: Nate Baer <[email protected]> Signed-off-by: Alejandro Reyes <[email protected]>

fix grammer

75a5b12

Co-authored-by: Nate Baer <[email protected]> Signed-off-by: Alejandro Reyes <[email protected]>

change config to enforce_all_conditions

f96ac5e

Signed-off-by: alejandroereyes <[email protected]>

alejandroereyes force-pushed the add_conditional_rollup branch from 1d0caac to f96ac5e Compare September 20, 2023 15:50

alejandroereyes requested review from njbbaer and lessthanjacob September 20, 2023 15:55

kkohrt reviewed Sep 28, 2023

View reviewed changes

github-actions bot added the no-pr-activity label Jan 18, 2024

github-actions bot closed this Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature proposal: Honor all conditional checks config - path for sparse fields #308

Feature proposal: Honor all conditional checks config - path for sparse fields #308

alejandroereyes commented Jul 18, 2023 •

edited

Loading

njbbaer left a comment

njbbaer Sep 19, 2023

lessthanjacob Sep 19, 2023

lessthanjacob commented Sep 19, 2023

alejandroereyes commented Sep 20, 2023

kkohrt left a comment •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023

kkohrt Sep 28, 2023

kkohrt Sep 28, 2023

github-actions bot commented Jan 18, 2024

-      if Blueprinter.configuration.enforce_all_conditions
-        any_global_or_field_condition_failed?(field_name, object, local_options)
-      else
-        return true if if_callable && !if_callable.call(field_name, object, local_options)
+    def skip?(field_name, object, local_options)
+      !inclusion_conditions_met?(field_name, object, local_options)
+    end
+    def inclusion_conditions_met?(field, obj, locals)
+      if_callables.any? { |callable| callable.call(field, obj, locals) } &&
+        unless_callables.none? { |callable| callable.call(field, obj, locals) }
+    end

	Blueprinter.configuration.enforce_all_conditions = true
	allow(Blueprinter).to receive(:configuration.enforce_all_conditions).and_return(true)

Feature proposal: Honor all conditional checks config - path for sparse fields #308

Feature proposal: Honor all conditional checks config - path for sparse fields #308

Conversation

alejandroereyes commented Jul 18, 2023 • edited Loading

Background

njbbaer left a comment

Choose a reason for hiding this comment

njbbaer Sep 19, 2023

Choose a reason for hiding this comment

lessthanjacob Sep 19, 2023

Choose a reason for hiding this comment

lessthanjacob commented Sep 19, 2023

alejandroereyes commented Sep 20, 2023

kkohrt left a comment • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kkohrt Sep 28, 2023

Choose a reason for hiding this comment

kkohrt Sep 28, 2023

Choose a reason for hiding this comment

kkohrt Sep 28, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2024

alejandroereyes commented Jul 18, 2023 •

edited

Loading

kkohrt left a comment •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading

kkohrt Sep 28, 2023 •

edited

Loading