Extractor configurability

[sandstrom]

Is there an existing issue for this?

• [X] I have searched the existing issues

Is your feature request related to a problem? Please describe

Association extractor is useful to modify to e.g. add a circuit-breaker, implement caching and for a few other things. Right now it's somewhat complicated to override/modify.

Describe the feature you'd like to see implemented

Right now, there are two types of extractors: AssociationExtractor and AutoExtractor (along with some sub-extractors).

Their roles are somewhat intertwined (AssociationExtractor calls the AutoExtractor, for example), and only one of them can be modified globally.

I'd propose a stricter separation, splitting it up into (probably) three extractors:

• FieldExtractor
• AssociationExtractor
• ValueExtractor (or DataExtractor)

The default ValueExtractor would encapsulate the logic currently existing in AutoExtractor, and I'd move the logic of deferring to block to field/association extractors. ValueExtractor could also be named something like DataExtractor instead.

Example

This is a rough sketch, in reality it would probably be slightly different.

class ValueExtractor < Extractor
  def extract(field_name, object, local_options, options = {})
    if object.is_a?(Hash)
      value = extract_hash(field_name, object, local_options, options)
    else
      value = extract_send(field_name, object, local_options, options)
    end
  end

  private

  def extract_hash(field_name, object, _local_options, _options = {})
    object[field_name]
  end

  def extract_send(field_name, object, _local_options, _options = {})
    object.public_send(field_name)
  end
end

class FieldExtractor < Extractor
  include EmptyTypes

  def initialize
    @extractor = Blueprinter.configuration.field_extractor_default.new # `ValueExtractor.new` by default
  end

  def extract(field_name, object, local_options, options = {})
    if options[:block]
      value = options[:block].call(object, local_options)
    else
      value = @value_extractor.extract(field_name, object, local_options, options)
    end

    value = @datetime_formatter.format(value, options)
    use_default_value?(value, options[:default_if]) ? default_value(options) : value
  end

  private

  def default_value(field_options)
    field_options.key?(:default) ? field_options.fetch(:default) : Blueprinter.configuration.field_default
  end
end

class AssociationExtractor < Extractor
  include EmptyTypes

  def initialize
    @extractor = Blueprinter.configuration.field_extractor_default.new # `ValueExtractor.new` by default
  end

  def extract(association_name, object, local_options, options = {})
    options_without_default = options.except(:default, :default_if)
    
    # Merge in assocation options hash
    local_options = local_options.merge(options[:options]) if options[:options].is_a?(Hash)

    if options[:block]
      value = options[:block].call(object, local_options)
    else
      value = @value_extractor.extract(association_name, object, local_options, options)
    end

    return default_value(options) if use_default_value?(value, options[:default_if])

    view = options[:view] || :default
    blueprint = association_blueprint(options[:blueprint], value)
    blueprint.prepare(value, view_name: view, local_options: local_options)
  end

  private

  def default_value(association_options)
    return association_options.fetch(:default) if association_options.key?(:default)

    Blueprinter.configuration.association_default
  end

  def association_blueprint(blueprint, value)
    blueprint.is_a?(Proc) ? blueprint.call(value) : blueprint
  end
end

Describe alternatives you've considered

No response

Additional context

No response

[sandstrom]

Happy to discuss this in more detail, and elaborate more.

[sandstrom]

Friendly ping @lessthanjacob @njbbaer @ritikesh

If you think this is a reasonable suggestion, let me know! If so, I'd propose these steps:

• I'll flesh out the proposed change in some more detail (written, in this issue)
• You get another chance to provide more feedback, and if you're happy we'll open a PR

For reference, there are also a few other issues that I've opened, where I'd also be happy to get your input.

Full list

• https://github.com/procore-oss/blueprinter/issues/403
• https://github.com/procore-oss/blueprinter/issues/404
• https://github.com/procore-oss/blueprinter/issues/405
• https://github.com/procore-oss/blueprinter/issues/406
• https://github.com/procore-oss/blueprinter/issues/407

[lessthanjacob]

I think this would be a reasonable refactor.

@njbbaer @ritikesh any thoughts/concerns here?

[ritikesh]

Agreed, we are open to contributions @sandstrom.

[sandstrom]

@lessthanjacob @ritikesh Thanks!

I'll have a chat with a colleague of mine, and we'll try to get back with a proposal.

[github-actions[bot]]

This issue 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.

[sandstrom]

Still relevant!

[github-actions[bot]]

This issue 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.

[sandstrom]

Not stale

[jhollinger]

I've been experimenting with this V2 extractor in #479. I think it covers the concerns in this issue.

Note that V2 (currently - nothing is set in stone) makes a distinction between a has_one/belongs_to association (aka object) and a has_many (aka collection). Those terms may change, but the distinction is proving useful in several areas.

module Blueprinter
  module V2
    # The default extractor and base class for custom extractors
    class Extractor
      #
      # Extract a field value.
      #
      # @param ctx [Blueprinter::V2::Context]
      #
      def field(ctx)
        if ctx.field.value_proc
          ctx.blueprint.instance_exec(ctx.object, ctx.options, &ctx.field.value_proc)
        elsif ctx.object.is_a? Hash
          ctx.object[ctx.field.from]
        else
          ctx.object.public_send(ctx.field.from)
        end
      end

      #
      # Extract an object field (singular association) value.
      #
      # @param ctx [Blueprinter::V2::Context]
      #
      def object(ctx)
        field ctx
      end

      #
      # Extract a collection field (array of objects) value.
      #
      # @param ctx [Blueprinter::V2::Context]
      #
      def collection(ctx)
        field ctx
      end
    end
  end
end

[sandstrom]

I think it looks good!

I would consider a name other than object for the singular association. Maybe has_one/has_many, single_item/multiple_items, item/collection, element/collection.

The things we've done in our extractors are:

• Circuit breaker (to avoid infinite recursion)
• Caching layer, to avoid doing some costly lookups multiple times
• Formatting

I think those would be covered here.

Any code example of what context would be?

[jhollinger]

@sandstrom Again, this is very much a WIP, but context is currently defined in #479 as:

#
# The Context struct is used for most extension hooks and all extractor methods.
# Some fields are always present, others are context-dependant. Each hook and extractor
# method will separately document which fields to expect and any special meanings.
#
# blueprint = Instance of the current Blueprint class (always)
# field = Field | ObjectField | Collection (optional)
# value = The current value of `field` or the Blueprint output (optional)
# object = The object currently being evaluated (e.g. passed to `render` or from an association) (optional)
# options = Arbitrary options passed to `render` (always)
# instances = Allows this entire render to share instances of Blueprints and Extractors (always)
# data = A Hash to for extensions, etc to cache render data in (always)
#
Context = Struct.new(:blueprint, :field, :value, :object, :options, :instances, :data)

[github-actions[bot]]

This issue 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.

[sandstrom]

Not stale

[filipkis]

Caching layer, to avoid doing some costly lookups multiple times

@sandstrom I would be curious to hear more about this. How did you achieve caching with current V1? I was looking into doing exactly the same, but as you pointed out current AssociationExtractor is hard to work with. Could you share example of how you've done it?