[SAY-5]

All Submissions:

• I've checked to ensure there aren't other open Pull Requests for the same update/change.
• I've submitted a ticket for my issue if one did not already exist.
• My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
• I've used Github auto-closing keywords in the commit message or the description.
• I've added/updated tests for this change.

Bug fixes and Changes to Core Features:

• I've included an explanation of what the changes do and why I'd like you to include them.
• I've provided test(s) that fails without the change.

Test Plan:

rake test needs a database and currently errors out on a booted-app requirement in rails/test_help that's unrelated to this change (reproduces the same way on master with Rails 7.2 and 8.1, so it looks like a separate pre-existing gap in test_helper.rb's Rails 8 support). To verify this fix directly I installed actionpack 7.2.2.2 and 8.1.3.1 side by side and drew routes with jsonapi_resources through a real ActionDispatch::Routing::RouteSet#draw, which is what test/unit/routing_ext_test.rb does. That test errors with the exact ArgumentError: wrong number of arguments (given 4, expected 3) from the issue when run against master, and passes on both Rails versions with this change.

Reviewer Checklist:

• Maintains compliance with JSON:API
• Adequate test coverage exists to prevent regressions

Fixes #1488

Rails 8.1 changed ActionDispatch::Routing::Mapper::Resource#initialize (and SingletonResource) to take only:/except: as keywords instead of a trailing positional options hash (rails/rails#53689). jsonapi_resource(s) still calls both with a plain positional hash, so drawing any route on Rails 8.1 raises ArgumentError: wrong number of arguments (given 4, expected 3) before the app boots. Passing options with ** instead of positionally works on both the old and the new signature.

[kurko]

LGTM, but can you look at the failing test?

[SAY-5]

I checked the failed Ruby 2.7 / Rails 6.0.6 / PostgreSQL job. It ran 675 tests with 8,767 assertions and one failure: RequestTest#test_pagination_related_resources_data_includes, at the cache-warmup equality assertion in test_helper.rb:507 (seed 13850). The diff contains the same ten book_comments IDs in a different order inside the included book's relationship data.

Both that test and its helper are byte-identical between the PR head 96eef89 and base ca2eb53; the PR changes only the two routing constructor calls and their routing tests. The other 27 jobs passed. The base branch's corresponding job also passed, so these results do not establish that the failure is pre-existing or unrelated to this change.

I have not reproduced the failure with the matching Ruby/Rails/PostgreSQL setup, so the cause remains unverified. The next diagnostic is a base-versus-head run with seed 13850, tracing the relationship query ordering with and without caching. No assertion or test has been weakened, and no CI repair is claimed.

[kurko]

3 paragraphs to say "can't reproduce".

Re-running. Seems flakey.

[kurko]

Ok, I'm going to make an executive decision and merge this one because (1) the failing test is flakey, (2) I don't want to block a legit update, and (3) #1499 should fix them.

[jagthedrummer]

Thanks for this, @kurko! In case it will help anyone else here's a monkey patch based on this PR that seems to be working in my project on 0.10.7.

config/initializers/jsonapi-resources-monkey-patch.rb

# frozen_string_literal: true
# Monkey patch to fix this issues: https://github.com/JSONAPI-Resources/jsonapi-resources/issues/1488
# Based on this PR: https://github.com/JSONAPI-Resources/jsonapi-resources/pull/1496

module ActionDispatch
  module Routing
    class Mapper
      Resources.class_eval do
        def jsonapi_resource(*resources, &_block)
          @resource_type = resources.first
          res = JSONAPI::Resource.resource_klass_for(resource_type_with_module_prefix(@resource_type))

          res._routed = true

          unless res.singleton?
            warn "Singleton routes created for non singleton resource #{res}. Links may not be generated correctly."
          end

          options = resources.extract_options!.dup
          options[:controller] ||= @resource_type
          options.merge!(res.routing_resource_options)
          options[:path] = format_route(@resource_type)

          if options[:except]
            options[:except] << :new unless options[:except].include?(:new) || options[:except].include?('new')
            options[:except] << :edit unless options[:except].include?(:edit) || options[:except].include?('edit')
          else
            options[:except] = [:new, :edit]
          end

          if res._immutable
            options[:except] << :create unless options[:except].include?(:create) || options[:except].include?('create')
            options[:except] << :update unless options[:except].include?(:update) || options[:except].include?('update')
            options[:except] << :destroy unless options[:except].include?(:destroy) || options[:except].include?('destroy')
          end

          resource @resource_type, options do
            # :nocov:
            if @scope.respond_to? :[]=
              # Rails 4
              @scope[:jsonapi_resource] = @resource_type

              if block_given?
                yield
              else
                jsonapi_relationships
              end
            else
              # Rails 5
              jsonapi_resource_scope(SingletonResource.new(@resource_type, api_only?, @scope[:shallow], **options), @resource_type) do
                if block_given?
                  yield
                else
                  jsonapi_relationships
                end
              end
            end
            # :nocov:
          end
        end

        def jsonapi_resources(*resources, &_block)
          @resource_type = resources.first
          res = JSONAPI::Resource.resource_klass_for(resource_type_with_module_prefix(@resource_type))

          res._routed = true

          if res.singleton?
            warn "Singleton resource #{res} should use `jsonapi_resource` instead."
          end

          options = resources.extract_options!.dup
          options[:controller] ||= @resource_type
          options.merge!(res.routing_resource_options)

          options[:param] = :id

          options[:path] = format_route(@resource_type)

          if res.resource_key_type == :uuid
            options[:constraints] ||= {}
            options[:constraints][:id] ||= /[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/
          end

          if options[:except]
            options[:except] = Array(options[:except])
            options[:except] << :new unless options[:except].include?(:new) || options[:except].include?('new')
            options[:except] << :edit unless options[:except].include?(:edit) || options[:except].include?('edit')
          else
            options[:except] = [:new, :edit]
          end

          if res._immutable
            options[:except] << :create unless options[:except].include?(:create) || options[:except].include?('create')
            options[:except] << :update unless options[:except].include?(:update) || options[:except].include?('update')
            options[:except] << :destroy unless options[:except].include?(:destroy) || options[:except].include?('destroy')
          end

          resources @resource_type, options do
            # :nocov:
            if @scope.respond_to? :[]=
              # Rails 4
              @scope[:jsonapi_resource] = @resource_type
              if block_given?
                yield
              else
                jsonapi_relationships
              end
            else
              # Rails 5
              jsonapi_resource_scope(Resource.new(@resource_type, api_only?, @scope[:shallow], **options), @resource_type) do
                if block_given?
                  yield
                else
                  jsonapi_relationships
                end
              end
            end
            # :nocov:
          end
        end
      end
    end
  end
end