Skip to content
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

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -670,13 +670,15 @@ Output:


Both the `field` and the global Blueprinter Configuration supports `:if` and `:unless` options that can be used to serialize fields conditionally.
By default, field-level conditions override global conditions. By enabling `enforce_all_conditions`, BOTH field-level and global conditions must be met in order for the field to be serialized.

### 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.enforce_all_conditions = true
end
```

Expand All @@ -690,8 +692,7 @@ class UserBlueprint < Blueprinter::Base
end
```

_NOTE:_ The field-level setting overrides the global config setting (for the field) if both are set.

---
</details>

<details>
Expand Down
4 changes: 3 additions & 1 deletion lib/blueprinter/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
module Blueprinter
class Configuration
attr_accessor :association_default, :datetime_format, :deprecations, :field_default, :generator, :if, :method,
:sort_fields_by, :unless, :extractor_default, :default_transformers, :custom_array_like_classes
:sort_fields_by, :unless, :extractor_default, :default_transformers, :custom_array_like_classes,
:enforce_all_conditions

VALID_CALLABLES = %i[if unless].freeze

Expand All @@ -20,6 +21,7 @@ def initialize
@extractor_default = AutoExtractor
@default_transformers = []
@custom_array_like_classes = []
@enforce_all_conditions = false
end

def array_like_classes
Expand Down
42 changes: 35 additions & 7 deletions lib/blueprinter/field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,49 @@ def extract(object, local_options)
end

def skip?(field_name, object, local_options)
return true if if_callable && !if_callable.call(field_name, object, local_options)
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)
Comment on lines +21 to +24
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

"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


unless_callable && unless_callable.call(field_name, object, local_options)
unless_callable && unless_callable.call(field_name, object, local_options)
end
end

private

alejandroereyes marked this conversation as resolved.
Show resolved Hide resolved
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) }
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

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.


unless_callables.any? { |unless_call| unless_call.call(field_name, object, local_options) }
end

def if_callable
return @if_callable if defined?(@if_callable)

@if_callable = callable_from(:if)
end

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

Comment on lines +44 to +50
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

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

def unless_callable
return @unless_callable if defined?(@unless_callable)

@unless_callable = callable_from(:unless)
end

def unless_callables
[
extract_callable_from(Blueprinter.configuration.unless),
extract_callable_from(options[:unless])
].select { |callable| callable }
end
Comment on lines +57 to +62
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

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


def callable_from(condition)
config = Blueprinter.configuration

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

return false unless tmp
extract_callable_from(tmp)
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

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.

end

def extract_callable_from(tmp_callable)
return false unless tmp_callable

case tmp
when Proc then tmp
when Symbol then blueprint.method(tmp)
case tmp_callable
when Proc then tmp_callable
when Symbol then blueprint.method(tmp_callable)
else
raise ArgumentError, "#{tmp.class} is passed to :#{condition}"
raise ArgumentError, "#{tmp_callable.class} is passed to :#{condition}"
Copy link

@kkohrt kkohrt Sep 28, 2023

Choose a reason for hiding this comment

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

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.

end
end
end
Expand Down
37 changes: 37 additions & 0 deletions spec/integrations/shared/base_render_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,43 @@ def self.unless_method(_field_name, _object, _options)
include_examples 'does not serialize the conditional field'
end
end

context 'enforce_all_conditions is enabled' do
let(:local_options) { {x: 1, y: 2, v1: value, v2: other_value} }

before do
Blueprinter.configuration.enforce_all_conditions = true
Copy link

Choose a reason for hiding this comment

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

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

Blueprinter.configuration.if = ->(field_name, object, local_opts) {
Copy link

Choose a reason for hiding this comment

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

Ideally this could also be stubbed with an allow condition

if local_opts[:sparse_fields]
local_opts[:sparse_fields].include?(field_name.to_s)
else
value
end
}
Blueprinter.configuration.unless = ->(_a,_b,_c) { other_value }
Copy link

Choose a reason for hiding this comment

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

ditto

end
after do
Blueprinter.configuration.enforce_all_conditions = false
Blueprinter.configuration.if = nil
Blueprinter.configuration.unless = nil
end

if value && !other_value
include_examples 'serializes the conditional field'

context 'and top level conditional triggered by local options' do
let(:local_options) { {sparse_fields: ['first_name']} }

it 'honors all configuration conditionals - only serializes specified field' do
should eq(%({"first_name":"Meg"}))
end
end
else
it 'nothing is serialized' do
should eq('{}')
end
end
end
end
end

Expand Down