Skip to content
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

Unable to access namespaced policy with const_get #172

Closed
baldwindavid opened this issue Jul 16, 2014 · 7 comments
Closed

Unable to access namespaced policy with const_get #172

baldwindavid opened this issue Jul 16, 2014 · 7 comments

Comments

@baldwindavid
Copy link

There seems to be an issue with the namespace functionality for what I think is a fairly common use case. Suppose an application has a UsersController at the base and then another UsersController namespaced under "admin." Assume that each of these then have a policy; UserPolicy and Admin::UserPolicy.

If the UserPolicy is accessed first, it seems that Admin::UserPolicy can then not be accessed later via const_get.

If I open a Rails console, I see the following...

Object.const_get "UserPolicy"
# => UserPolicy

Admin.const_get "UserPolicy"
# => UserPolicy

However, if I exit out of the console, re-enter the console and ask for the namespaced policy first, it works...

Admin.const_get "UserPolicy"
# => Admin::UserPolicy
@ecbypi
Copy link
Contributor

ecbypi commented Jul 16, 2014

I think this is because of Rails' autoloading. I ran the following in a Rails console:

Object.const_get "UserPolicy"
# => UserPolicy

Admin.const_get "UserPolicy"
# => UserPolicy

Admin::UserPolicy
# => Admin::UserPolicy

Admin.const_get 'UserPolicy'
# => Admin::UserPolicy

If Admin::UserPolicy isn't loaded yet, const_get will look in Object for UserPolicy. When it doesn't find it there, it will call const_missing. I'm not sure if const_missing is being called on Object or it's an issue / limitation of Rails' autoloading that it finds objects in the global namespace first.

I'll look into it more when I have some time and see if there's a way to handle it in the pundit code.

@baldwindavid
Copy link
Author

My initial thought was that Rails autoloading has something to do with it too. Note that if you start with the namespaced call to const_get, it works appropriately...

Admin.const_get "UserPolicy"
# => Admin::UserPolicy

I was hoping that "qualified_const_get" might work, but that doesn't seem to make a difference.

In my fork of pundit, I just ended up brute-forcing it to use the namespaced policy no matter what...

# policy_finder.rb
klass = "#{namespace}::#{klass.demodulize}".constantize if klass.is_a?(String)

You lose the fallback to the non-namespaced policy functionality with this, but that is not a big deal for my usage.

@DanOlson
Copy link

I ran into this problem last night. It was definitely an autoload issue. If you require the policies explicitly in an initializer, it should fix the problem. To illustrate:

# config/require_policies.rb
Dir[Rails.root.join('app/policies/**/*.rb')].each &method(:require)
# Without explicit require:
$ rails c
> Admin.constants
=> []

# With explicit require:
$ rails c
> Admin.constants
=> [:UserPolicy]

I'd guess there are no constants under your namespace because of how autoload works. As a result, Admin.const_get "UserPolicy" will return nil and Pundit will just fall back on UserPolicy.

Also, how are you declaring your namespaced classes? If you embed the namespace in the class definition, you'll run into a NameError with the require because the namespace is not yet defined. For example:

$ irb
2.1.2 :001 > class Foo::Bar; end
NameError: uninitialized constant Foo

2.1.2 :002 > module Foo
2.1.2 :003?>   class Bar; end
2.1.2 :004?>     end
=> nil

I really doubt you need to patch Pundit to fix this issue.

@thomasklemm
Copy link
Collaborator

@jnicklas Do we need to address this before releasing 0.3.0?

@jnicklas
Copy link
Collaborator

See my comment in #178. IMO the namespacing thing is a failure and we should yank it entirely before 0.3.0. Thoughts?

@onemanstartup
Copy link

@jnicklas, @thomasklemm It's not a failure it's just how autoload works in rails rails/rails#14844 rails/rails#8726
Proper fix for loading namespaced files is that

# article_policy.rb
class ArticlePolicy < ApplicationPolicy
end
require_dependency "admin/article_policy"

# admin/article_policy.rb
class Admin::ArticlePolicy < ArticlePolicy
end

You don't need to remove feature, just write in readme that person need to requre dependency at the bottom.

However this is not so nice, so I'm thinking maybe pundit can use modules for inheritance? Every policy usually have user and record object, so if there was one class for policy and actual rules would be taken from corresponding modules. With modules as far as I'm thinking there will be no such troubles with autoload.

@jnicklas
Copy link
Collaborator

See my reply in: #178 (comment)

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

No branches or pull requests

6 participants