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

problems with 0.10.1 changes to plain old resources #45

Open
bf4 opened this issue Nov 15, 2019 · 5 comments
Open

problems with 0.10.1 changes to plain old resources #45

bf4 opened this issue Nov 15, 2019 · 5 comments

Comments

@bf4
Copy link
Contributor

bf4 commented Nov 15, 2019

We're upgrading from 0.9.10 to 0.10.1

in one of our acceptance specs we saw a failure

     {"exception"=>"undefined method `where' for []:Array",
      "backtrace"=>
       ["/root/server/vendor/bundle/ruby/2.6.0/gems/jsonapi-resources-0.10.1/lib/jsonapi/active_relation_resource.rb:843:in `apply_filter'",

it turns out, it's added a filter[:id] = "57630ec8-3332-4be3-8421-6de45545f294" where that uuid is coming from the resource identifier.

the reason there's an [] is that this is a plain old resource where we, naturally, cannot return an active record relation, but instead are returning an empty collection.

I've tried fiddling with class methods self.records , self.records_base, self.apply_filter, self.apply_join and instance methodsrecords_for, records, records_base and stepped through quite a bunch of code, but haven't been able to figure out what exactly changed or how to handle it

e.g.

     90:       # @return [Hash{ResourceIdentity => {identity: => ResourceIdentity, cache: cache_field, attributes: => {name => value}}}]
     91:       #    the ResourceInstances matching the filters, sorting, and pagination rules along with any request
     92:       #    additional_field values
     93:       def find_fragments(filters, options = {})

     filters
=> {:id=>"e57ed230-84e5-429c-806f-5e4e13af81ea"}
options
=> {:context=>
  {:current_user=>
    #<User:0x00007fba8499eb20
     id: 1,
      :fields=>{},
 :filters=>{:id=>"e57ed230-84e5-429c-806f-5e4e13af81ea"},
 :include_directives=>
  #<JSONAPI::IncludeDirectives:0x00007fba849dd348
   @include_directives_hash={:include_related=>{}},
   @resource_klass=V1::SomeResource>,
 :cache=>false}

probably this is related to cerebris/jsonapi-resources#1221 ?

The 0.9.x resource in question is defined something like below (we only interact with it by POST)

module PlainOldResource
  module Extensions
    # @override
    def records(*)
      []
    end
  end
  extend ActiveSupport::Concern

  class_methods do
    # @override
    def updatable_fields(_context)
      []
    end
  end

  included do |base|
    base.extend ::PlainOldResource::Extensions
  end
  # @override
  def id
    if _model.respond_to?(:id)
      _model.id
    else
      @id ||= SecureRandom.uuid
    end
  end

  # @override
  def created_at
    @created_at ||= Time.now
  end

  # @override
  alias_method :updated_at, :created_at

  # @override
  def records_for(association_name, _options = {})
    _model.public_send(association_name)
  end

  # @override
  # @see https://github.com/cerebris/jsonapi-resources/blob/v0.9.10/lib/jsonapi/resource.rb#L395-L425
  def replace_attributes(data)
    attributes = attributes_from(data)
    assign_attributes attributes
  end

  def assign_attributes(attributes)
    attributes.each do |key, value|
      attribute_setter = "#{key}="
      _model.public_send(attribute_setter, value)
    end
  end

  # Finds attributes in the POST/PUT request to assign to the model.
  #
  # Override to add relationships or remove attributes
  # @example
  #    def attributes_from(data)
  #      super.merge!(
  #        some_thing_ids: data[:to_many][:some_things],
  #      )
  #    end
  def attributes_from(data)
    attributes = data[:attributes].dup
    data[:to_many].each do |key, value|
      attributes["#{key}_ids"] = value
    end
    data[:to_one].each do |key, value|
      attributes["#{key}_id"] = value
    end
    attributes
  end
end
class ThingActionResource < BaseResource
  include PlainOldResource
  abstract

  has_one :thing
  attribute :comment

  def fetchable_fields
    [:id, :thing, :comment]
  end

  def replace_fields(data)
    attributes = replace_attributes(data)
    ensure_model_exists!(attributes)
    authorize_action!(:create?)
    perform
  end

  private

  def assign_attributes(attributes)
    super
    _model.changed_by = current_user
  end

  def ensure_model_exists!(attributes)
    return unless _model.thing.nil?
    fail JSONAPI::Exceptions::RecordNotFound.new(attributes[:thing_id])
  end

  def perform
    if _model.perform
      :completed
    else
      fail JSONAPI::Exceptions::ValidationErrors.new(self)
    end
  end

  def current_user
    context[:current_user]
  end
end
@scottgonzalez
Copy link
Collaborator

You won't want to use self.records as that is now specific to resources that are backed by ActiveRelation (records_for is also gone completely).

I believe you'll want to return the empty array from self.find now. If you actually do have models to return, you can use resources_for(array_of_models, context) to generate the correct response from self.find.

That should at least you get further along. If you continue having problems, please just let us know. If that alone solves the problem, please close this issue once your app is working again. We'll hopefully have documentation for these changes posted soon.

@bf4
Copy link
Contributor Author

bf4 commented Nov 25, 2019

sounds like part of what we need to do is have two base resources

  1. class BaseResource < Resource
  2. class PlainOldBaseResource < BasicResource

and mix in any common stuff we want

'cause right now I guess we need to patch the JR::Resource to handle where it assumes the resource is active-record based?

@bf4
Copy link
Contributor Author

bf4 commented Nov 25, 2019

find didn't help (yet?). Doesn't seem to be called

(the guides for 0.10.x need all the removed stuff removed from it.. better to be incomplete and prs accepted than wrong :)

@bf4
Copy link
Contributor Author

bf4 commented Nov 26, 2019

Looks like PORO support is generally pretty broken in 0.10.x as well as association overrides. e.g. cerebris/jsonapi-resources#1045 (comment)
and cerebris/jsonapi-resources#1244

I'm going to stop working on the upgrade now.

I ended up with something like

  class PoroResource < JSONAPI::BasicResource
    root_resource

    # see https://github.com/cerebris/jsonapi-resources/blob/cc9679302b7989018aad47cc04b9112f9422ffff/test/fixtures/active_record.rb
    class << self
      def find_records(filters, options)
        fail NotImplementedError, <<~EOF
        Should be something like
        def find_records(filters, options)
          breeds = []
          id_filter = filters[:id]
          id_filter = [id_filter] unless id_filter.nil? || id_filter.is_a?(Array)
          $breed_data.breeds.values.each do |breed|
            breeds.push(breed) unless id_filter && !id_filter.include?(breed.id)
          end
          breeds
        end
        EOF
      end

      def find_record_by_key(key, options = {})
        fail NotImplementedError, <<~EOF
        Should be something like
        def find_record_by_key(key, options = {})
          $breed_data.breeds[key.to_i]
        end
        EOF
      end

      def find_records_by_keys(keys, options = {})
        fail NotImplementedError, <<~EOF
        Should be something like
        def find_records_by_keys(keys, options = {})
          breeds = []
          keys.each do |key|
            breeds.push($breed_data.breeds[key.to_i])
          end
          breeds
        end
        EOF
      end

      # Records
      # NOTE(BF) this seems to be the source of all our pain
      def find_fragments(filters, options = {})
        fragments = {}
        find_records(filters, options).each do |record|
          rid = JSONAPI::ResourceIdentity.new(resource_klass, record.id)
          fragments[rid] = JSONAPI::ResourceFragment.new(rid)
        end
        fragments
      end

      def resource_klass
        self
      end

      # Finds Resources using the `filters`. Pagination and sort options are used when provided
      #
      # @param filters [Hash] the filters hash
      # @option options [Hash] :context The context of the request, set in the controller
      # @option options [Hash] :sort_criteria The `sort criteria`
      # @option options [Hash] :include_directives The `include_directives`
      #
      # @return [Array<Resource>] the Resource instances matching the filters, sorting and pagination rules.
      def find(filters, options = {})
        records = find_breeds(filters, options)
        resources_for(records, options[:context])
      end

      # # Counts Resources found using the `filters`
      # #
      # # @param filters [Hash] the filters hash
      # # @option options [Hash] :context The context of the request, set in the controller
      # #
      # # @return [Integer] the count
      # def count(filters, options = {})
      #   count_records(records)
      # end

      # Returns the single Resource identified by `key`
      #
      # @param key the primary key of the resource to find
      # @option options [Hash] :context The context of the request, set in the controller
      def find_by_key(key, options = {})
        record = find_record_by_key(key, options)
        resource_for(record, options[:context])
      end

      def find_to_populate_by_keys(keys, options = {})
        find_by_keys(keys, options)
      end

      # Returns an array of Resources identified by the `keys` array
      #
      # @param keys [Array<key>] Array of primary keys to find resources for
      # @option options [Hash] :context The context of the request, set in the controller
      def find_by_keys(keys, options = {})
        records = find_records_by_keys(keys, options)
        resources_for(records, options[:context])
      end

      # This resource class (ActiveRelationResource) uses an `ActiveRecord::Relation` as the starting point for
      # retrieving models. From this relation filters, sorts and joins are applied as needed.
      # Depending on which phase of the request processing different `records` methods will be called, giving the user
      # the opportunity to override them differently for performance and security reasons.

      # begin `records`methods

      # Base for the `records` methods that follow and is not directly used for accessing model data by this class.
      # Overriding this method gives a single place to affect the `ActiveRecord::Relation` used for the resource.
      #
      # @option options [Hash] :context The context of the request, set in the controller
      #
      # @return [ActiveRecord::Relation]
      def records_base(_options = {})
        raise 'not used'
        # _model_class.all
      end

      # The `ActiveRecord::Relation` used for finding user requested models. This may be overridden to enforce
      # permissions checks on the request.
      #
      # @option options [Hash] :context The context of the request, set in the controller
      #
      # @return [ActiveRecord::Relation]
      def records(options = {})
        raise 'not used'
        # records_base(options)
      end

      # The `ActiveRecord::Relation` used for populating the ResourceSet. Only resources that have been previously
      # identified through the `records` method will be accessed. Thus it should not be necessary to reapply permissions
      # checks. However if the model needs to include other models adding `includes` is appropriate
      #
      # @option options [Hash] :context The context of the request, set in the controller
      #
      # @return [ActiveRecord::Relation]
      def records_for_populate(options = {})
        raise 'not used'
        # records_base(options)
      end

      # The `ActiveRecord::Relation` used for the finding related resources. Only resources that have been previously
      # identified through the `records` method will be accessed and used as the basis to find related resources. Thus
      # it should not be necessary to reapply permissions checks.
      #
      # @option options [Hash] :context The context of the request, set in the controller
      #
      # @return [ActiveRecord::Relation]
      def records_for_source_to_related(options = {})
        raise 'not used'
        # records_base(options)
      end

      # end `records` methods

      # def apply_join(records:, relationship:, resource_type:, join_type:, options:)
      # def relationship_records(relationship:, join_type: :inner, resource_type: nil, options: {})
      # def join_relationship(records:, relationship:, resource_type: nil, join_type: :inner, options: {})
    end
  end
  class BasePlainResource < PoroResource
    include PlainOldResource
  end

and

module PlainOldResource
  module Extensions
# snip
+    def find_records(filters, options)
+      []
+    end
# snip
  end
  extend ActiveSupport::Concern
end

getting a 201 on the POST but an empty data attribute in the response. Seems to have to do with find_fragments needing to 'find' the _model we're 'creating', but just not feeling any progress at this point.

@scottgonzalez
Copy link
Collaborator

In v0.10, the operation processing and the response generation are two distinct processes. This is why you need to find the created model now. Since you've explicitly defined find_records to always return an empty set, it's not surprising that you're getting no data in your response.

If the resources are actually findable, you should implement a proper find_records method that will work for all requests. If the resources are not findable (perhaps they do not actually persist), you can store the created model in context in an after_create callback and then return that model in the finder (you can access context via the options argument).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants