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

fix: hierarchy model with namespace should inherit from the superclass of basic model #384

Merged

Conversation

shawndodo
Copy link
Contributor

@shawndodo shawndodo commented Sep 15, 2021

#382
FYI: In my case, I have a namespace AAA for billing, and billing model inherits from AAA::ApplicationRecord.
Like the following code:

module AAA
  class Billing < ApplicationRecord
    connects_to database: { writing: :foo_service, reading: :foo_service}
    has_closure_tree
  end
end

module AAA
  class ApplicationRecord < ActiveRecord::Base
    connects_to database: { writing: :aaa, reading: :aaa }

    self.abstract_class = true
  end
end

Therefore if I follow the unmodified code to execute, AAA::BillingHierarchy will inherit from ApplicationRecord, not AAA::ApplicationRecord, then it will connect to primary database, not aaa database.

PTAL.Thank you.

@seuros
Copy link
Member

seuros commented Sep 15, 2021

While this code works, the implementation is not correct. We need to inherit just the connection. This PR take in consideration that everyone will use ApplicationRecord inside a namespace.

What if i have 1 model inheriting from PostgresqlRecord and Another From MysqlRecord for example ?

What we need to do is to inherit still from Base, use the connection of the model with the tree.

@shawndodo shawndodo changed the title fix: hierarchy class with namespace should inherit from the Namespace::ApplicationRecord fix: hierarchy class with namespace should connect to the database of parent class Sep 16, 2021
@shawndodo shawndodo marked this pull request as draft September 16, 2021 11:48
@shawndodo shawndodo marked this pull request as ready for review September 16, 2021 18:31
@shawndodo
Copy link
Contributor Author

@seuros I have made some changes for inheriting the connection. PTAL. Thank you.

While this code works, the implementation is not correct. We need to inherit just the connection. This PR take in consideration that everyone will use ApplicationRecord inside a namespace.

What if i have 1 model inheriting from PostgresqlRecord and Another From MysqlRecord for example ?

What we need to do is to inherit still from Base, use the connection of the model with the tree.

I think if our goal is to use the connection of the model with the tree, maybe we need to reproduce the logic of rails to make the models inherit from the same superclass use the same one database connection. Shall we use model_class.superclass to replace ActiveRecord::Base? Then we do not need to mind whether the model inherits from PostgresqlRecord or MysqlRecord.

@seuros
Copy link
Member

seuros commented Sep 16, 2021

I was thinking at that solution at first.
But since we can override the connection at the model level , we impose this on other devs.

We should update the README to state that the hierarchy is going to inherit the connection from the model. If a Dev has some custom setup that need special setting, there is always the ability to create the Hierarchy model manually and assign it.

@seuros
Copy link
Member

seuros commented Sep 16, 2021

Let use your fix for now. We can inherit the connection once we drop support for old AR version. Good job

@shawndodo
Copy link
Contributor Author

Thank you!
I did some attempts to override the connection at the model level, then I found that I can override with different connections easily but cannot make the hierarchy model and the basic model use the same connection within a short time. So I think superclass is a not bad choice for me.

@shawndodo shawndodo changed the title fix: hierarchy class with namespace should connect to the database of parent class fix: hierarchy model with namespace should inherit from the superclass of basic model Sep 16, 2021
@seuros seuros merged commit f3b33f8 into ClosureTree:master Sep 17, 2021
@mbandrewfoster
Copy link

It would be nice if the class being used was configurable (rather than model_class.superclass or even ActiveRecord::Base) in the same way the hierarchy_class_name is configurable right now.
This change breaks our code base because of how we use Rails engines. The model_class.superclass is not what we want as that causes other problems for us.

@seuros
Copy link
Member

seuros commented Nov 9, 2021

@mbandrewfoster can you explain your use case ?

@AmShaegar13
Copy link

@mbandrewfoster can you explain your use case ?

Same problem here. See my issue regarding this: #395

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.

5 participants