Skip to content

Replace methods by singleton_methods when overriding them. #2585

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

Closed
wants to merge 1 commit into from

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented Jul 19, 2025

Overview

This PR refines the mechanism for overriding class methods on the API's base instance to ensure proper setup recording for remountable APIs in Grape.

Key Change

  • Replaced methods with singleton_methods:
    The logic that dynamically overrides methods on the API class now uses singleton_methods instead of methods. This ensures that only singleton methods defined directly on the base instance are considered for overriding, rather than all methods (including inherited ones).

Motivation

Previously, using methods could result in unintended methods being overridden, including those inherited from parent classes or modules. By switching to singleton_methods, we ensure that only the relevant, directly-defined methods are overridden for setup recording. This makes the remounting and inheritance behavior of Grape APIs more predictable and robust.

Impact

  • No breaking changes to the public API.
  • Internal API setup and remounting are now more precise and maintainable.
  • Lower memory foot print

Testing

  • All existing specs pass.
  • Manual testing confirms that remounting and inheritance behave as expected.

@ericproulx ericproulx marked this pull request as ready for review July 19, 2025 13:02
@grape-bot
Copy link

grape-bot commented Jul 19, 2025

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@ericproulx ericproulx force-pushed the api_override_singleton_methods branch from 43b1fb8 to a817425 Compare July 20, 2025 09:24
@dblock
Copy link
Member

dblock commented Jul 20, 2025

This does change behavior for (accidentally) overridden methods. Add a test for this?

@ericproulx
Copy link
Contributor Author

This does change behavior for (accidentally) overridden methods. Add a test for this?

AI says it does but it doesn't. Its equal methods. I ran it on grape-on-rack and its the same method count.

@dblock
Copy link
Member

dblock commented Jul 21, 2025

This does change behavior for (accidentally) overridden methods. Add a test for this?

AI says it does but it doesn't. Its equal methods. I ran it on grape-on-rack and its the same method count.

I mean this guards against overridden methods being used, so you can inherit the class and end up with more, no? So you can write a test that would fail with the old code. Or am I missing the point of this?

@ericproulx
Copy link
Contributor Author

This does change behavior for (accidentally) overridden methods. Add a test for this?

AI says it does but it doesn't. Its equal methods. I ran it on grape-on-rack and its the same method count.

I mean this guards against overridden methods being used, so you can inherit the class and end up with more, no? So you can write a test that would fail with the old code. Or am I missing the point of this?

I found something interesting. I made a simple test

it 'is equivalent' do
  expect(Grape::API::Instance.methods - Class.methods - Grape::API::NON_OVERRIDABLE).to match_array(Grape::API::Instance.singleton_methods(true) - Grape::API::NON_OVERRIDABLE)
end

Just running the test, it passes but when running the whole suite, it fails

1) Grape::API.inherited is equivalent
     Failure/Error: expect(Grape::API::Instance.methods - Class.methods - Grape::API::NON_OVERRIDABLE).to match_array(Grape::API::Instance.singleton_methods(true) - Grape::API::NON_OVERRIDABLE)
     
       expected collection contained:  [:after, :after_validation, :auth, :base, :base=, :base_instance?, :before, :before_validation, :buil..._route, :unset, :unset_namespace_stackable, :use, :version, :versions, :within_namespace, :yaml_tag]
       actual collection contained:    [:after, :after_validation, :auth, :base, :base=, :base_instance?, :before, :before_validation, :buil..., :uniqe_id_route, :unset, :unset_namespace_stackable, :use, :version, :versions, :within_namespace]
       the missing elements were:      [:yaml_tag]

Somehow, psych adds the yaml_tag while running the tests and its part of the singleton methods.
I don't know if my change is worth it but we need to keep the - Class.methods which removes the yaml_tag function and probably more tin the wild

@ericproulx
Copy link
Contributor Author

ericproulx commented Jul 27, 2025

All this work lead to another thing. Lots of internal functions are part of the public scope which are overriden for nothing:

:after,
 :after_validation,
 :auth,
 :base,
 :base=,
 :base_instance?,
 :before,
 :before_validation,
 :build_with,
 :cascade,
 :change!,
 :compile,
 :configuration=,
 :content_type,
 :content_types,
 :contract,
 :default_error_formatter,
 :default_error_status,
 :default_format,
 :define_boolean_in_mod,  #internal
 :delete,
 :desc,
 :do_not_document!,
 :do_not_route_head!,
 :do_not_route_options!,
 :endpoints,
 :error_formatter,
 :evaluate_as_instance_with_configuration,  #internal
 :finally,
 :format,
 :formatter,
 :get,
 :get_or_set, #internal
 :given,
 :global_setting,
 :group,
 :head,
 :helpers,
 :http_basic,
 :http_digest,
 :include_all_in_scope,  #internal
 :include_block,  #internal
 :include_new_modules,  #internal
 :inherit_settings,
 :inheritable_setting,
 :inheritable_setting=,
 :inject_api_helpers_to_mod,  #internal
 :insert,
 :insert_after,
 :insert_before,
 :instance,
 :lint!,
 :logger,
 :make_inclusion,  #internal
 :middleware,
 :mount,
 :mounted,
 :namespace,
 :namespace_end,  #internal
 :namespace_inheritable,  #internal
 :namespace_inheritable_to_nil,  #internal
 :namespace_reverse_stackable,  #internal
 :namespace_reverse_stackable_with_hash,  #internal
 :namespace_setting,  #internal
 :namespace_stackable, #internal
 :namespace_stackable_with_hash,  #internal
 :namespace_start,  #internal
 :nest, # internal
 :options,
 :params,
 :parser,
 :patch,
 :post,
 :prefix,
 :prepare_routes,  #internal
 :put,
 :represent,
 :rescue_from,
 :reset!,  #internal
 :reset_endpoints!,  #internal
 :reset_routes!,  #internal
 :reset_validations!,  #internal
 :resource,
 :resources,
 :route,
 :route_end,  #internal
 :route_param,
 :route_setting,
 :routes,
 :scope,
 :segment,
 :top_level_setting,
 :top_level_setting=,  #internal
 :unset,  #internal
 :unset_namespace_stackable,  #internal
 :use,
 :version,
 :versions,
 :within_namespace

@ericproulx ericproulx closed this Jul 27, 2025
@ericproulx ericproulx deleted the api_override_singleton_methods branch July 27, 2025 14:29
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

Successfully merging this pull request may close these issues.

3 participants