search

cerebris / jsonapi-resources

A resource-focused Rails library for developing JSON:API compliant servers.
http://jsonapi-resources.com
MIT License
2.32k stars 533 forks source link

Class caching is too aggressive in dev #1439

Closed adamkiczula closed 8 months ago

adamkiczula commented 10 months ago

This issue is a (choose one):

Checklist before submitting:

Description

Choose one section below and delete the other:

Bug reports:

I'm using the v0-11-dev branch, and recently upgraded from commit 1bdacf1 to 311b1fe. If I start the server I can make a request for a resource just fine, but if I modify any resource I get empty results returned until I restart my dev server. I stepped through the commits on the v0-11-dev branch between the range above and narrowed it down to 2668b6b that seems to have introduced the bug. The resource queries after making code changes have a AND (TRUE=FALSE) condition appended to them which causes nothing to be returned.

Steps to reproduce:

  1. Start dev server.
    • Note, I have eager_load = false, cache_classes = false in development.rb, but do separately eager load my models if that might make a difference.
  2. Request a resource index, in my case "users"
    • I see queries like this: User Count (0.5ms) SELECT COUNT(*) FROM "users"
  3. Make any change to a resource, even an unrelated one. In my case, I removed the updated_at field from the list of fields
  4. Make the same request from step 2 again
    • I now see queries like this: User Count (0.2ms) SELECT COUNT(*) FROM "users" WHERE (TRUE=FALSE)

UPDATE Feb 7, 2024

The WHERE (TRUE=FALSE) bit isn't specifically coming from jsonapi-resources, but is coming from CanCanCan, it's their default scope if no rules apply and you use accessible_by which I am using, and no rules apply due to class caching (more on that below).

The underlying problem is that the new class caching introduced in 2668b6b doesn't get reset when Rails reloads your application. I believe this is because JSONAPI::Resource doesn't live within the app, so modifications to the app code don't get picked up. So if you make a change to your app, then the following is true:

Here's the bug report template that reproduces this issue. Warning, it will write to ./app/resources/your_model_resource.rb because the file needs to be there so the autoloader can reload it.

# ensure the resource exists in the "unedited state"
MY_RESOURCE_FILE_PATH = File.dirname(__FILE__)
puts "creating resource file in #{MY_RESOURCE_FILE_PATH}"
`mkdir -p #{MY_RESOURCE_FILE_PATH}/app/resources`
`echo 'class YourModelResource < JSONAPI::Resource; attribute :name; end' > #{MY_RESOURCE_FILE_PATH}/app/resources/your_model_resource.rb`

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

  if ENV['JSONAPI_RESOURCES_PATH']
    puts "installing jsonapi-resources @ #{ENV['JSONAPI_RESOURCES_PATH']}"
    gem 'jsonapi-resources', path: ENV['JSONAPI_RESOURCES_PATH'], require: false
  elsif ENV['BEFORE_BUG']
    puts "installing jsonapi-resources @ 0bbbc0bc9d3dbe547fc9a7e34aab2185d0cef682"
    gem 'jsonapi-resources', git: 'https://github.com/cerebris/jsonapi-resources', ref: '0bbbc0bc9d3dbe547fc9a7e34aab2185d0cef682', require: false
  else
    puts "installing jsonapi-resources @ v0-11-dev"
    gem 'jsonapi-resources', git: 'https://github.com/cerebris/jsonapi-resources', branch: 'v0-11-dev', 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 :your_models, force: true do |t|
    t.string :name
  end
end

class YourModel < ActiveRecord::Base
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 YourModelsController < ApplicationController
  include JSONAPI::ActsAsResourceController
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.cache_classes = false
  config.eager_load = false
  config.hosts = ["example.org"]
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 :your_models, only: [:index, :create]
end

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

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

  def test_resource_klass_cache_refreshes
    assert_equal(
      YourModelResource.object_id,
      JSONAPI::Resource.resource_klass_for('your_models').object_id,
      "Expected resource to be returned from resource_klass_for"
    )
    assert_equal(
      %i[id name],
      YourModelResource.fields,
      "Expected YourModelResource definition to be reset to default fields"
    )
    assert_equal(
      %i[id name],
      JSONAPI::Resource.resource_klass_for('your_models').fields,
      "Expected JSONAPI::Resource.resource_klass_for definition to be reset to default fields"
    )

    # "update" the resource class to make `display_name` the new `name` attribute
    `echo 'class YourModelResource < JSONAPI::Resource; attribute :display_name, delegate: :name; end' > #{Rails.root}/app/resources/your_model_resource.rb`
    Rails.application.reloader.reload!

    assert_equal(
      %i[id display_name],
      YourModelResource.fields,
      "Expected YourModelResource definition to be reloaded"
    )

    assert_equal(
      %i[id display_name],
      JSONAPI::Resource.resource_klass_for('your_models').fields,
      "Expected JSONAPI::Resource.resource_klass_for definition to be reloaded"
    )
    assert_equal(
      YourModelResource.object_id,
      JSONAPI::Resource.resource_klass_for('your_models').object_id,
      "Expected updated resource to be returned from resource_klass_for after reload"
    )
  end
end

Run BEFORE_BUG=true ruby test_template.rb to see it pass Run ruby test_template.rb to see it fail against v0-11-dev

lgebhardt commented 8 months ago

Fixed with #1448