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

The model reference in add_belongs_to can lead to issues #709

Closed
jwoertink opened this issue Jul 18, 2021 · 2 comments · Fixed by #742
Closed

The model reference in add_belongs_to can lead to issues #709

jwoertink opened this issue Jul 18, 2021 · 2 comments · Fixed by #742
Labels
clarify api Rename/remove/add something to make the API easier to understand needs investigation A possible bug / better docs needed. Investigate further

Comments

@jwoertink
Copy link
Member

Ref: #685 (comment)

Right now, the type used in add_belongs_to is only used for generating a table reference name, and not for enforcing a type. If you use a model that's in a namespace, you won't get the reference that you're expecting in some cases.

Take this for example:

module MyApp
  class Post < BaseModel
    table :posts do
    end
  end
end

# then in your migration
def migrate
  create table_for(MyApp::Post) do
    # ...
    add_belongs_to user : MyApp::User, on_delete: :cascade
  end
end

This will create the user_id column, but tell postgres that the foreign key references the my_app_users table. To fix this, you would add the references option:

add_belongs_to user : MyApp::User, references: :users, on_delete: :cascade or don't use a namespaced type here.
add_belongs_to user : User, on_delete: :cascade.

We can either make this easier to know (no clue how), or we can look at trying to resolve the model to get its table name...

This might be a tricky one.

@jwoertink jwoertink added clarify api Rename/remove/add something to make the API easier to understand needs investigation A possible bug / better docs needed. Investigate further labels Jul 18, 2021
@jwoertink
Copy link
Member Author

Just tossing out ideas here, but what if we just make it an error that if you pass in a namespace, it blows up without the reference option? Even if your table is namespaced like my_app_users, then you can still be explicit in that reference.

add_belongs_to user : MyApp::User, references: :my_app_users, on_delete: :cascade

@robacarp
Copy link
Contributor

robacarp commented Oct 4, 2021

what if we just make it an error that if you pass in a namespace, it blows up without the reference option?

That might not be a perfect solution, but I think it would be an improvement to the current behavior. It's bitten me a couple times and I had to stumble through getting it to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarify api Rename/remove/add something to make the API easier to understand needs investigation A possible bug / better docs needed. Investigate further
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants