WIP #1105 fix bug with inclusion and linkage of has_one polymorphic

[senid231]

WIP Fix #1105
assignment and inclusion of has_one polymorphic relationships works incorrectly

tested on bug report

begin
  require 'bundler/inline'
  require 'bundler'
rescue LoadError => e
  STDERR.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true, ui: ENV['SILENT'] ? Bundler::UI::Silent.new : Bundler::UI::Shell.new) do
  source 'https://rubygems.org'

  gem 'rails', require: false
  gem 'sqlite3', platform: :mri

  gem 'activerecord-jdbcsqlite3-adapter',
      git: 'https://github.com/jruby/activerecord-jdbc-adapter',
      branch: 'rails-5',
      platform: :jruby

  if ENV['JSONAPI_RESOURCES_PATH']
    gem 'jsonapi-resources', path: ENV['JSONAPI_RESOURCES_PATH'], require: false
  else
    gem 'jsonapi-resources', git: 'https://github.com/cerebris/jsonapi-resources', require: false
  end

end

# prepare active_record database
require 'active_record'

class NullLogger < Logger
  def initialize(*_args)
  end

  def add(*_args, &_block)
  end
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = ENV['SILENT'] ? NullLogger.new : Logger.new(STDOUT)
ActiveRecord::Migration.verbose = !ENV['SILENT']

ActiveRecord::Schema.define do
  # Add your schema here
  create_table :options, force: true do |t|
    t.integer :optionable_id
    t.string :optionable_type
    t.integer :maintainer_id
    t.boolean :enabled, default: false, null: false
  end

  create_table :androids, force: true do |t|
    t.string :version_name
  end

  create_table :users, force: true do |t|
    t.string :name
  end
end

# create models
class Option < ActiveRecord::Base
  belongs_to :optionable, polymorphic: true, required: false
  belongs_to :maintainer, class_name: 'User', required: false
end

class Android < ActiveRecord::Base
  has_one :option, as: :optionable
end

class User < ActiveRecord::Base
  has_one :maintained_option
end

# prepare rails app
require 'action_controller/railtie'
# require 'action_view/railtie'
require 'jsonapi-resources'

class ApplicationController < ActionController::Base
end

# prepare jsonapi resources and controllers
class OptionsController < ApplicationController
  include JSONAPI::ActsAsResourceController
end

class OptionResource < JSONAPI::Resource
  attribute :enabled
  has_one :optionable, polymorphic: true, class_name: 'Android'
  has_one :maintainer, class_name: 'User'
end

class AndroidResource < JSONAPI::Resource
  attribute :version_name
end

class UserResource < JSONAPI::Resource
  attribute :name
end

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.logger = ENV['SILENT'] ? NullLogger.new : Logger.new(STDOUT)
  Rails.logger = config.logger

  secrets.secret_token = 'secret_token'
  secrets.secret_key_base = 'secret_key_base'

  config.eager_load = false
end

# initialize app
Rails.application.initialize!

JSONAPI.configure do |config|
  config.json_key_format = :underscored_key
  config.route_format = :underscored_key
end

# draw routes
Rails.application.routes.draw do
  jsonapi_resources :options, only: [:show, :update]
end

# prepare tests
require 'minitest/autorun'
require 'rack/test'

# Replace this with the code necessary to make your test fail.
class BugTest < Minitest::Test
  include Rack::Test::Methods

  def json_api_headers
    {'Accept' => JSONAPI::MEDIA_TYPE, 'CONTENT_TYPE' => JSONAPI::MEDIA_TYPE}
  end

  def test_patch_link_option_with_android_include_optionable
    android = Android.create! version_name: '1.0'
    option = Option.create!
    json_request = {
        data: {
            id: option.id.to_s,
            type: 'options',
            relationships: {
                optionable: {
                    data: {
                        id: android.id.to_s,
                        type: 'androids'
                    }
                }
            }
        }
    }
    patch "/options/#{option.id}?include=optionable", json_request.to_json, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    },
                    data: {
                        id: android.id.to_s,
                        type: 'androids'
                    }
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    }
                }
            }
        },
        included: [
            {
                id: android.id.to_s,
                type: 'androids',
                links: {
                    self: "http://example.org/androids/#{android.id}"
                },
                attributes: {
                    version_name: android.version_name
                }
            }
        ]
    }
    assert_equal expected_response, last_response_json
    assert_equal android, option.reload.optionable
  end

  def test_patch_unlink_option_from_android_include_optionable
    android = Android.create! version_name: '1.0'
    option = Option.create! optionable: android
    json_request = {
        data: {
            id: option.id.to_s,
            type: 'options',
            relationships: {
                optionable: {
                    data: nil
                }
            }
        }
    }
    patch "/options/#{option.id}?include=optionable", json_request.to_json, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    },
                    data: nil
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    }
                }
            }
        }
    }
    assert_equal expected_response, last_response_json
    assert_equal nil, option.reload.optionable
  end

  def test_fetch_option_linked_with_android_include_optionable
    android = Android.create! version_name: '1.0'
    option = Option.create! optionable: android
    get "/options/#{option.id}?include=optionable", nil, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    },
                    data: {
                        id: android.id.to_s,
                        type: 'androids'
                    }
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    }
                }
            }
        },
        included: [
            {
                id: android.id.to_s,
                type: 'androids',
                links: {
                    self: "http://example.org/androids/#{android.id}"
                },
                attributes: {
                    version_name: android.version_name
                }
            }
        ]
    }
    assert_equal expected_response, last_response_json
  end

  def test_fetch_option_not_linked_with_android_include_optionable
    option = Option.create!
    get "/options/#{option.id}?include=optionable", nil, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    },
                    data: nil
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    }
                }
            }
        }
    }
    assert_equal expected_response, last_response_json
  end

  def test_fetch_option_not_linked_with_maintainer_include_maintainer
    option = Option.create!
    get "/options/#{option.id}?include=maintainer", nil, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    }
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    },
                    data: nil
                }
            }
        }
    }
    assert_equal expected_response, last_response_json
  end

  def test_fetch_option_linked_with_maintainer_include_maintainer
    user = User.create! name: 'John Doe'
    option = Option.create! maintainer: user
    get "/options/#{option.id}?include=maintainer", nil, json_api_headers
    assert last_response.ok?
    expected_response = {
        data: {
            id: option.id.to_s,
            type: 'options',
            links: {
                self: "http://example.org/options/#{option.id}"
            },
            attributes: {
                enabled: false
            },
            relationships: {
                optionable: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/optionable",
                        related: "http://example.org/options/#{option.id}/optionable"
                    }
                },
                maintainer: {
                    links: {
                        self: "http://example.org/options/#{option.id}/relationships/maintainer",
                        related: "http://example.org/options/#{option.id}/maintainer"
                    },
                    data: {
                        id: user.id.to_s,
                        type: 'users'
                    }
                }
            }
        },
        included: [
            {
                id: user.id.to_s,
                type: 'users',
                links: {
                    self: "http://example.org/users/#{user.id}"
                },
                attributes: {
                    name: user.name
                }
            }
        ]
    }
    assert_equal expected_response, last_response_json
  end

  private

  def last_response_json
    JSON.parse(last_response.body).deep_symbolize_keys
  end

  def app
    Rails.application
  end
end

[senid231]

three failures left:

BugTest#test_patch_unlink_option_from_android_include_optionable [/home/senid/projects/forked/jsonapi-resources/lib/bug_report_templates/rails_5_master_polymorphic_linkage_bug.rb:246]:
--- expected
+++ actual
@@ -1 +1 @@
-{:data=>{:id=>"6", :type=>"options", :links=>{:self=>"http://example.org/options/6"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/6/relationships/optionable", :related=>"http://example.org/options/6/optionable"}, :data=>nil}, :maintainer=>{:links=>{:self=>"http://example.org/options/6/relationships/maintainer", :related=>"http://example.org/options/6/maintainer"}}}}}
+{:data=>{:id=>"6", :type=>"options", :links=>{:self=>"http://example.org/options/6"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/6/relationships/optionable", :related=>"http://example.org/options/6/optionable"}}, :maintainer=>{:links=>{:self=>"http://example.org/options/6/relationships/maintainer", :related=>"http://example.org/options/6/maintainer"}}}}}

BugTest#test_fetch_option_not_linked_with_maintainer_include_maintainer [/home/senid/projects/forked/jsonapi-resources/lib/bug_report_templates/rails_5_master_polymorphic_linkage_bug.rb:365]:
--- expected
+++ actual
@@ -1 +1 @@
-{:data=>{:id=>"4", :type=>"options", :links=>{:self=>"http://example.org/options/4"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/4/relationships/optionable", :related=>"http://example.org/options/4/optionable"}}, :maintainer=>{:links=>{:self=>"http://example.org/options/4/relationships/maintainer", :related=>"http://example.org/options/4/maintainer"}, :data=>nil}}}}
+{:data=>{:id=>"4", :type=>"options", :links=>{:self=>"http://example.org/options/4"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/4/relationships/optionable", :related=>"http://example.org/options/4/optionable"}}, :maintainer=>{:links=>{:self=>"http://example.org/options/4/relationships/maintainer", :related=>"http://example.org/options/4/maintainer"}}}}}

BugTest#test_fetch_option_not_linked_with_android_include_optionable [/home/senid/projects/forked/jsonapi-resources/lib/bug_report_templates/rails_5_master_polymorphic_linkage_bug.rb:331]:
--- expected
+++ actual
@@ -1 +1 @@
-{:data=>{:id=>"1", :type=>"options", :links=>{:self=>"http://example.org/options/1"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/1/relationships/optionable", :related=>"http://example.org/options/1/optionable"}, :data=>nil}, :maintainer=>{:links=>{:self=>"http://example.org/options/1/relationships/maintainer", :related=>"http://example.org/options/1/maintainer"}}}}}
+{:data=>{:id=>"1", :type=>"options", :links=>{:self=>"http://example.org/options/1"}, :attributes=>{:enabled=>false}, :relationships=>{:optionable=>{:links=>{:self=>"http://example.org/options/1/relationships/optionable", :related=>"http://example.org/options/1/optionable"}}, :maintainer=>{:links=>{:self=>"http://example.org/options/1/relationships/maintainer", :related=>"http://example.org/options/1/maintainer"}}}}}

[senid231]

@lgebhardt, I end up with an understanding that JSONAPI::Processor#flatten_resource_id_tree works incorrect after refactoring at bdcbf61.

when we include relationship and it is not linked response does not have data in relationship object

BTW this behaviour is not tested at all, so all tests are green, which is completely wrong

[lgebhardt]

@senid231 Thanks for the detailed error report. With the refactor I was worried about behaviors that were not explicitly tested, like this case.

[senid231]

@lgebhardt many cases aren't tested yet, and it's hard to add tests into the project, and hard to understand.
I think it should be refactored so that everyone could understand them and understand, add, and change them easily.
For now, I don't know how to do it. Have you any ideas and/or plans about tests refactoring?

[senid231]

BTW, what should I do with this PR?

should I remove WIP from the title so it could be merged?

@lgebhardt, I'm not familiar with the latest refactoring that you made, so finish it now. I can continue when I will have free time, or maybe you want to fix the issue with response serialization?

[lgebhardt]

@senid231 I agree that the tests could be refactored, but it will be a large undertaking (and I don't have a lot of time for it). Probably the best way would be to add new tests as unit and integration tests and leave the existing tests in place. Eventually we can remove them after we ensure all the relevant functionality is tested.

[lgebhardt]

You can leave the WIP in the title. I'd like to get some tests added to demonstrate the problem before I merge this PR. I can try to make some time in the coming week to work on that, and hopefully get a fix in there.

I'm not sure what you mean by "finish it now"? Are you referring to #1045? It is finished, but may still have bugs as you have found. I plan to get a related resource pagination support added before we do an alpha release where hopefully the bugs can be shaken out.

[senid231]

@lgebhardt what do you think about testing with RSpec?
It has its own downsides (like much bigger time for setup and runtime) but I think it will allow writing tests drier and more readable at the same time.

[senid231]

By "finish it now" I meant that I can't finish work on this PR and fix all bugs that I found. There are many changes in project, that I don't understand, yet

[lgebhardt]

I don't have an opinion on RSpec. At this point it would need to be drastically better to justify the expense of switching. To me the problem with the test suite in JR isn't the testing framework, but rather that things are tested at too high of a level (controller/integration instead of unit).

I tent agree with @tenderlove here: https://tenderlovemaking.com/2015/01/23/my-experience-with-minitest-and-rspec.html

[lgebhardt]

@senid231 I understand if you don't have time to fix all the bugs you find. If you can create an issue to describe them, even if it doesn't contain a fully reproducible bug report like you provided here, it will be appreciated.

[senid231]

@lgebhardt I have added tests that cover fixes that I made and bug with missing data: null in response for when not linked relationships are included

[senid231]

@lgebhardt I rebased this PR
seems issue still relevant

[gordonbisnor]

If I understand this issue, I think that 0.10 fixes it? – I was having an issue where I had a model with a has_one relationship, and then specified has_one in my resource, and then included that record in my request and received a 500 error where it was using the belongs_to builder instead of the has_one builder.... Updated to 0.10 and no longer having any trouble.

[senid231]

@gordonbisnor @lgebhardt but 0.9 still have an issue and 0.10 is not stable yet

I can rebase this PR to 0.9

what do you think about it?