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

Extractor configurability #404

Open
1 task done
sandstrom opened this issue Mar 21, 2024 · 12 comments
Open
1 task done

Extractor configurability #404

sandstrom opened this issue Mar 21, 2024 · 12 comments
Labels
help wanted Anyone is welcome to open a pull request to fix this issue refactor opportunity

Comments

@sandstrom
Copy link

Is there an existing issue for this?

  • 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 sandstrom mentioned this issue Mar 21, 2024
1 task
@sandstrom
Copy link
Author

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

@sandstrom
Copy link
Author

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

@lessthanjacob
Copy link
Contributor

I think this would be a reasonable refactor.

@njbbaer @ritikesh any thoughts/concerns here?

@ritikesh
Copy link
Collaborator

Agreed, we are open to contributions @sandstrom.

@sandstrom
Copy link
Author

@lessthanjacob @ritikesh Thanks!

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

Copy link

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
Copy link
Author

Still relevant!

Copy link

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
Copy link
Author

Not stale

@lessthanjacob lessthanjacob added the help wanted Anyone is welcome to open a pull request to fix this issue label Sep 17, 2024
@jhollinger
Copy link
Contributor

jhollinger commented Nov 11, 2024

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
Copy link
Author

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
Copy link
Contributor

jhollinger commented Nov 12, 2024

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Anyone is welcome to open a pull request to fix this issue refactor opportunity
Projects
None yet
Development

No branches or pull requests

4 participants