Namespaced models count as excluded if same prefix (ie: User + User::Stuff)

MyklClason commented 4 years ago

Steps to reproduce

Create a User model
Create a User::Stuff model
Exclude User model.

Expected behavior

User model is excluded, but User::Stuff is not.

Actual behavior

User and User::Stuff models are excluded resulting in User::Stuff being publicly scoped.

Edit: Actually in some cases, this may be a desired behavior (See #633) in terms of what can be done through apartment, but the point remains that the above Expected Behavior is not possible to achieve.

However, models not namespaced under any of the excluded model names work as expected, so fairly sure this it's accepting anything under the User namespace as being part of the user model.

I'm likely going to end up having to change the namespace those models are under, but figured I'd report the issue in case there is a quick fix I'm missing. Thankfully the app isn't actually in production yet, so rebuilding the database isn't a major issue, but would be nice to avoid it.

System configuration

AWS Cloud9 Ubuntu

Database: (Tell us what database and its version you use.)
```
Postgres 10.10
```

Apartment version:

$ gem list | grep apartment
apartment (2.2.1)

Apartment config (in config/initializers/apartment.rb or so):

# Apartment initializer
# frozen_string_literal: true

require "apartment/elevators/generic"

Apartment.configure do |config|
  config.excluded_models = ["Tenant", "User"]

  config.tenant_names = -> { Tenant.pluck :tenant_name }
end

Rails.application.config.middleware.use Apartment::Elevators::Custom

Original configuration was a bit more complicated as it was using UUIDs for a while, then decided to move away from them in favor of friendly IDs, thought that was the issue, but it looks like it wasn't.

use_schemas: (true or false) Postgres, so yes I believe.

Rails (or ActiveRecord) version: Rails 5.2.3
Ruby version: ruby 2.6.5

MyklClason commented 4 years ago

Thought of a workaround for this issue that didn't require a ton of refactoring. Looks like this might be more of a rails issue, but it is related to the gem so perhaps just an official approach to this problem is needed.

Seems that Apartment is excessively rewriting the tables. So something like User::Stuff turns into public.user_stuffs instead of user_stuffs. Work around here is to use remove the public., but only for models where the first module is is also the name of an excluded model. This will ensure it is still applied to models which are actually excluded.

Also doesn't handle the case where you want to exclude User::Stuff and have a User::Stuff::Other model, but wouldn't be too difficult to handle if one needed it by adjusting this.

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  def self.inherited(subclass)
    super

    # Hack to fix issue with Apartment excluded models being overly broad
    # Given that the first module is A,
    #  remove the public schema if that A is an excluded model

    if Apartment.excluded_models.include?(subclass.to_s.deconstantize)
      subclass.table_name = subclass.table_name.gsub('public.', '')
    end
  end
end

lcjury commented 4 years ago

You can look for all the constants who inherit from ActiveRecord::Base defined inside your modules and add them to your tenants array

Namespace.constants.map{|c| Namespace.const_get(c) }.select do |const|
    const.class == Class && const < ActiveRecord::Base
end

but, if you want to put that on apartment configuration, you need to lazyload it

class LazyTenants
  def method_missing(method, *args, &block)
    tenants.send(method, *args, &block)
  end

  def tenants
    @tenants ||= Namespace.constants.map{|c| Namespace.const_get(c) }.select do |const|
        const.class == Class && const < ApplicationRecord
    end.map(&:to_s) + ["Tenant"]
  end
end

Apartment.configure do |config|
  config.excluded_models = LazyTenants.new
end

MyklClason commented 4 years ago

@lcjury I don't think that is what I was aiming for. That seems like it would be extremely broad in terms of exclusion. It might work in some cases, but not others. Though the idea of using a method to determine what should be excluded instead of models would be nice.

I actually wanted about 95% of the models to be tenanted. However I had a User::Stuff and also Tenant::Stuff (And Tenant::Stuff::MoreStuff) type models that were supposed to be tenant specific, but the User and Tenant models were cross tenant.

MyklClason commented 4 years ago

Basically issue is with this line:

https://github.com/influitive/apartment/blob/f92b459c933fccb9a11c085944307ea8750d607a/lib/apartment/adapters/postgresql_adapter.rb#L49-L56

Specifically:

table_name = klass.table_name.split('.', 2).last

If I recall right, this causes a cascading issue where User::Stuff is basically uses User.table_name + ".stuff" and so if User is public, then it cascade down. causing anything that starts with User:: to be public if User is excluded. There are certainly times when this is a desired behavior as #633 points out. However, it should be clear in the documentation (and code) how to achieve both results or a combination of the two (IE: Exclude everything User:: but don't exclude User and vise versa), thus making for 3 scenarios that need to be handled, of which only one can be handled at the moment.

MyklClason commented 4 years ago

Edited the actual behavior to concede that the current behavior is certainly a desirable behavior in terms of what apartment can do, but it prevents a behavior I'd like to do.

So perhaps this is more of a feature request rather than a bug report.

bjensen commented 4 years ago

Maybe we should make a check on wether the input is a class or module? Seams like it would make everyone happy :-)

influitive / apartment