Gate belongs_to :owner on enable_application_owner? at include time

[55728]

Problem

Since #1830 (ab58c37), Doorkeeper::Models::Ownership is included unconditionally from Mixins::Application's included block. This means belongs_to :owner is always declared — even when enable_application_owner is not set and the schema lacks the owner_id/owner_type columns. The :owner reflection is visible, but calling owner_type raises NoMethodError, which is confusing.

Root cause

The original commit chose unconditional inclusion to avoid "freezing" the gate at first-load time. In practice, the real constraint was that the test suite exercised the enabled behavior by reconfiguring Doorkeeper.configure { enable_application_owner } at runtime and calling Doorkeeper.run_orm_hooks — which is now a no-op. Since ActiveSupport::Concern's included block runs only once, the association could never be retro-fitted onto an already-loaded model, so the tests relied on it being always present.

Note: gating the :owner association on the flag also restores the pre-#1830 behavior — the old on_load(:active_record) path included Ownership conditionally (if enable_application_owner?), so OFF-by-default never declared the association. #1830's unconditional include was the regression; this PR is not a new user-facing behavior change.

Fix

Gate the association and its presence validation on enable_application_owner? inside Ownership's included block:

included do
  if Doorkeeper.config.enable_application_owner?
    belongs_to :owner, polymorphic: true, optional: true
    validates :owner, presence: true, if: :validate_owner?
  end
end

This mirrors how PolymorphicResourceOwner::ForAccessGrant / ForAccessToken gate belongs_to :resource_owner — same pattern, same timing.

enable_application_owner? reads option_set? (an instance variable check), so this introduces no constantize call (#1703 safe) and no ActiveSupport.on_load(:active_record) (#1828 safe).

Also synced the now-stale comment above include ::Doorkeeper::Models::Ownership in Mixins::Application, which still argued for unconditional inclusion.

Test changes

enable_application_owner is now formally a load-time switch: its effect is evaluated once when the model class is defined. Tests that exercise the enabled path can no longer reconfigure an already-loaded singleton.

Introduced build_application_model helper (spec/support/helpers/application_model_helper.rb) that builds a fresh ActiveRecord::Base subclass with Mixins::Application included after the feature is configured. This keeps examples order-independent and avoids mutating the global Doorkeeper::Application.

The two #1830 regression specs that asserted ":owner association is present even when the feature is off" are inverted to assert the opposite — the intended behavioral change of this PR.

Also fixed a pre-existing label typo: duplicate "without confirmation" → "with confirmation".

Closes #1831

[nbulaj]

Since we already in the included block here which is lazy as well - can we move if Doorkeeper.config.enable_application_owner? here and include conditionally ? 👀 Or I missed smth?

[55728]

Good call 👀✨ — the include site is just as lazy (both run in the same included block at parent-class autoload), so I moved the gate up:

include ::Doorkeeper::Models::Ownership if Doorkeeper.config.enable_application_owner?

Doorkeeper::Models::Ownership is now untouched and the whole change lives at the include site. One observable difference: with the feature off, the model no longer carries the Ownership module in its ancestors nor defines validate_owner? — I updated the specs that asserted those.

[houndci-bot]

Some files could not be reviewed due to errors:

Error: unrecognized cop RSpec/AnyInstance found in .rubocop.yml, unrecognized...

Error: unrecognized cop RSpec/AnyInstance found in .rubocop.yml, unrecognized cop RSpec/ExpectInHook found in .rubocop.yml, unrecognized cop RSpec/IndexedLet found in .rubocop.yml, unrecognized cop RSpec/InstanceVariable found in .rubocop.yml, unrecognized cop RSpec/LeakyConstantDeclaration found in .rubocop.yml, unrecognized cop RSpec/MessageChain found in .rubocop.yml, unrecognized cop RSpec/MessageSpies found in .rubocop.yml, unrecognized cop RSpec/RepeatedExampleGroupDescription found in .rubocop.yml, unrecognized cop RSpec/SubjectStub found in .rubocop.yml, unrecognized cop RSpec/VerifiedDoubles found in .rubocop.yml, unrecognized cop plugins found in .rubocop.yml, unrecognized cop Rails/DynamicFindBy found in .rubocop.yml, unrecognized cop Rails/HttpPositionalArguments found in .rubocop.yml, unrecognized cop Rails/HttpStatus found in .rubocop.yml, unrecognized cop Rails/RakeEnvironment found in .rubocop.yml, unrecognized cop Rails/ReflectionClassName found in .rubocop.yml, unrecognized cop Rails/SkipsModelValidations found in .rubocop.yml, unrecognized cop RSpec/BeforeAfterAll found in .rubocop.yml, unrecognized cop RSpec/ContextWording found in .rubocop.yml, unrecognized cop RSpec/DescribeClass found in .rubocop.yml, unrecognized cop RSpec/ExampleLength found in .rubocop.yml, unrecognized cop RSpec/ExpectChange found in .rubocop.yml, unrecognized cop RSpec/MultipleMemoizedHelpers found in .rubocop.yml, unrecognized cop RSpec/SpecFilePathFormat found in .rubocop.yml, unrecognized cop RSpec/MultipleExpectations found in .rubocop.yml, unrecognized cop RSpec/NestedGroups found in .rubocop.yml, unrecognized cop RSpec/NoExpectationExample found in .rubocop.yml

[Copilot]

Pull request overview

This PR fixes a regression introduced by #1830 where Doorkeeper::Models::Ownership (and therefore belongs_to :owner) was being applied to the Application model unconditionally, exposing a misleading :owner association even when enable_application_owner is disabled and the schema lacks owner_id/owner_type.

Changes:

• Conditionally include Doorkeeper::Models::Ownership from the ActiveRecord Application mixin only when Doorkeeper.config.enable_application_owner? is enabled at include time.
• Update specs to treat enable_application_owner as a load-time switch by building fresh AR model classes after configuring Doorkeeper.
• Add a spec helper (build_application_model) and update changelog/documentation to reflect the intended behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
spec/support/helpers/application_model_helper.rb	Adds a helper to build a fresh AR Application model after configuring enable_application_owner.
spec/models/doorkeeper/application_spec.rb	Updates regression expectations (no :owner association by default) and rewrites enabled-owner tests to use fresh model classes.
spec/lib/doorkeeper/orm/active_record_spec.rb	Adjusts ORM hook/mixin inclusion expectations for Ownership under default vs enabled configuration.
spec/lib/config_spec.rb	Updates config specs to avoid relying on retrofitting associations onto already-loaded classes.
lib/doorkeeper/orm/active_record/mixins/application.rb	Gates Ownership inclusion on enable_application_owner? at include time.
CHANGELOG.md	Documents the user-visible behavior change for disabled application-owner feature.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[houndci-bot]

Some files could not be reviewed due to errors:

Error: unrecognized cop RSpec/AnyInstance found in .rubocop.yml, unrecognized...

Error: unrecognized cop RSpec/AnyInstance found in .rubocop.yml, unrecognized cop RSpec/ExpectInHook found in .rubocop.yml, unrecognized cop RSpec/IndexedLet found in .rubocop.yml, unrecognized cop RSpec/InstanceVariable found in .rubocop.yml, unrecognized cop RSpec/LeakyConstantDeclaration found in .rubocop.yml, unrecognized cop RSpec/MessageChain found in .rubocop.yml, unrecognized cop RSpec/MessageSpies found in .rubocop.yml, unrecognized cop RSpec/RepeatedExampleGroupDescription found in .rubocop.yml, unrecognized cop RSpec/SubjectStub found in .rubocop.yml, unrecognized cop RSpec/VerifiedDoubles found in .rubocop.yml, unrecognized cop plugins found in .rubocop.yml, unrecognized cop Rails/DynamicFindBy found in .rubocop.yml, unrecognized cop Rails/HttpPositionalArguments found in .rubocop.yml, unrecognized cop Rails/HttpStatus found in .rubocop.yml, unrecognized cop Rails/RakeEnvironment found in .rubocop.yml, unrecognized cop Rails/ReflectionClassName found in .rubocop.yml, unrecognized cop Rails/SkipsModelValidations found in .rubocop.yml, unrecognized cop RSpec/BeforeAfterAll found in .rubocop.yml, unrecognized cop RSpec/ContextWording found in .rubocop.yml, unrecognized cop RSpec/DescribeClass found in .rubocop.yml, unrecognized cop RSpec/ExampleLength found in .rubocop.yml, unrecognized cop RSpec/ExpectChange found in .rubocop.yml, unrecognized cop RSpec/MultipleMemoizedHelpers found in .rubocop.yml, unrecognized cop RSpec/SpecFilePathFormat found in .rubocop.yml, unrecognized cop RSpec/MultipleExpectations found in .rubocop.yml, unrecognized cop RSpec/NestedGroups found in .rubocop.yml, unrecognized cop RSpec/NoExpectationExample found in .rubocop.yml

[leoarnold]

@55728 Looks good to me (see also here)! 👏

[55728]

Both Copilot's points are valid — updated in the next force-push:

1. CHANGELOG: dropped the owner_type / NoMethodError claim; the fix only removes the misleading :owner reflection.
2. Helper comment: reworded to "As of Gate belongs_to :owner on enable_application_owner? at include time #1832 (fixes Unconditional inclusion ::Doorkeeper::Models::Ownership has confusing side effect #1831)" since the behavior change is introduced by this PR, not Unconditional inclusion ::Doorkeeper::Models::Ownership has confusing side effect #1831.

[55728]

Hound skipped his walk today 🐕💤

[55728]

@leoarnold Thank you for verifying against your app 😊 — that's exactly the behavior this PR restores 🎉