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

rel:has-one on a relationship should not have foreign key creation #1115

Closed
K1DV5 opened this issue Jan 28, 2025 · 9 comments · Fixed by #1130
Closed

rel:has-one on a relationship should not have foreign key creation #1115

K1DV5 opened this issue Jan 28, 2025 · 9 comments · Fixed by #1130
Labels
help wanted Extra attention is needed

Comments

@K1DV5
Copy link

K1DV5 commented Jan 28, 2025

I was writing a function to generate the create table statements with the foreign keys and it was working fine...until I found out that I had a has-one relationship.

I was under the impression that has-one was meant to be used in place of has-many to deal with a one-to-one relationship. Example copied from the docs:

// Profile belongs to User.
type Profile struct {
	ID     int64 `bun:",pk"`
	Lang   string
	UserID int64
}

type User struct {
	ID      int64 `bun:",pk"`
	Name    string
	Profile *Profile `bun:"rel:has-one,join:id=user_id"`
}

It says right there that the Profile belongs to the user and that is what should refer to the User not the other way around. But currently, when trying to use the bun.CreateTableQuery.WithForeignKeys(), it is creating a foreign key like FOREIGN KEY ("id") REFERENCES "profiles" ("user_id") on the User table.

Also, as I have a belongs-to relationship on the Profile model, it is correctly creating the foreign key to reference the User table. The issue is that the other one should not be created.

@j2gg0s j2gg0s added the help wanted Extra attention is needed label Feb 5, 2025
@j2gg0s
Copy link
Collaborator

j2gg0s commented Feb 5, 2025

I’m not familiar with the code related to migration.
I need to find some free time to go through the overall logic.

😂

@dordieux
Copy link

I've created a PR for this.
Please review my PR 🙏

#1126

@j2gg0s
Copy link
Collaborator

j2gg0s commented Feb 14, 2025

The example in the documentation doesn’t seem quite appropriate.

If it’s a has-one relationship, we should store the Profile ID in the User table instead.

// Profile belongs to User.
type Profile struct {
	ID     int64 `bun:",pk"`
	Lang   string
}

type User struct {
	ID      int64 `bun:",pk"`
	Name    string
        ProfileID string
	Profile *Profile `bun:"rel:has-one,join:profile_id=id"`
}

In this example, we need to create a foreign key, like FOREIGN KEY ("profile_id") REFERENCES "profiles" ("id").
So maybe we can’t simply remove the logic that creates the foreign key for the has-one relationship.
@dordieux

If it's a belong-to relationship, we should store the user's id in the profile table.

// Profile belongs to User.
type Profile struct {
	ID     int64 `bun:",pk"`
	Lang   string
        UserID string 
        User *User `bun:"rel:belong-to,join:user_id=id"`
}

type User struct {
	ID      int64 `bun:",pk"`
	Name    string
}

In this example, we need to create a foreign key, like FOREIGN KEY ("user_id") REFERENCES "user" ("id")

@vmihailenco Is there anyone familiar with this area who can provide guidance?

@j2gg0s
Copy link
Collaborator

j2gg0s commented Feb 14, 2025

I've created a PR for this. Please review my PR 🙏

#1126

Thank you for your PR; it helped me understand the key logic.

@dordieux
Copy link

dordieux commented Feb 14, 2025

@j2gg0s
I see, that makes sense.
But when considering only migrations, it might be better to create foreign keys only for belongs-to relationships.

However, similar to has-many, it would be useful if has-one could also be referenced within the parent struct.
Is this not considered?

// Profile belongs to User.
type Profile struct {
	ID     int64  `bun:",pk"`
	Lang   string
	UserID string
	User   *User `bun:"rel:belongs-to,join:user_id=id"`
}

type User struct {
	ID      int64  `bun:",pk"`
	Name    string
	Profile *Profile `bun:"rel:has-one,join:user_id=id"`
}

@j2gg0s
Copy link
Collaborator

j2gg0s commented Feb 14, 2025

New Tag to disable FK?

@K1DV5
Copy link
Author

K1DV5 commented Feb 14, 2025

The example in the documentation doesn’t seem quite appropriate.

If it’s a has-one relationship, we should store the Profile ID in the User table instead.

I agree that the documentation needs to be clearer, but I wouldn't say it's not appropriate. Going by the doc's idea, we want to have one profile per user. And each profile should belong to a user. And for this, the columns are actually correct, it just doesn't have the relations. So the complete version would be:

type Profile struct {
	ID     int64 `bun:",pk"`
	Lang   string
	UserID int64
        User *User `bun:"belongs-to,join=user_id=id"`  // this should have been in the docs
}

type User struct {
	ID      int64 `bun:",pk"`
	Name    string
	Profile *Profile `bun:"rel:has-one,join:id=user_id"`
}

Otherwise, the docs are good, only the foreign key creation. The reason, from the profile's perspective, it is already satisfied that there can only be one user because it refers to the user model. And as such, it is correctly creating the foreign key for the user_id column. But from the user model, we have two choices: has-many and has-one, none of which should be concerned with the database logic because we can only have foreign keys in one direction, and we already have it on the profile.

In summary, the docs are good, just a little incomplete. Problem is the foreign key creation on the user.

@bevzzz
Copy link
Collaborator

bevzzz commented Feb 14, 2025

@j2gg0s thank you for starting the discussion on this one and @dordieux thank you for the PR.
I will take some time on the weekends to go through the changes and the comments here and write back asap

@bevzzz
Copy link
Collaborator

bevzzz commented Feb 15, 2025

Hi everyone! Finally had the time to dig into this and here's what I think.

TL;DR: As far as I understand, has-one and belongs-to are almost synonymous, so both should produce an FK relation where it makes sense. It doesn't make sense in the example from the docs, because Profile *Profile `bun:"rel:"` is only there to fetch Profile at query time. Our code should guard against such cases -- this PR outlines my proposed fix.

@dordieux I also added a short comment to your PR, but I'm writing a more detailed version here. I'd love to hear your thoughts on that!


The example in the documentation doesn’t seem quite appropriate.

So this has sent me reading the docs for ORM: Table relationships, and, interestingly enough, words "foreign key" aren't mentioned anywhere on that page. 😄

It looks to me like rel: tag was primarily intended for telling Bun how to join related entities at query time and creating FKs is rather a "bonus feature". If we read into the docs, has-one doesn't mean "this field references that field", but rather sth like "when fetching a user, I want to fetch their profile as well".

In that light the example doesn't seem inappropriate at all -- rel: tag may be used without foreign keys, but we can use the information therein to create them if needed.

But when considering only migrations, it might be better to create foreign keys only for belongs-to relationships.

Which is why I also disagree with this. The choice between has-one and belongs-to depends wholly on how the user reads their data, so a foreign key should be created in either case, as long as we can reasonably assume that it was intended. Consider the modified example @j2gg0s provides above:

type User struct {
	ID      int64 `bun:",pk"`
        ProfileID string
	Profile *Profile `bun:"rel:has-one,join:profile_id=id"`
}

Calling NewCreateTable().WithForeignKeys(), one would expect users table to have a foreign key users.profile_id -> profiles.id.

I was under the impression that has-one was meant to be used in place of has-many to deal with a one-to-one relationship.

Right, I think that's a correct impression. However, it doesn't necessarily mean we shouldn't create FKs for has-one relation just because we do not create them for has-many.

Problem is the foreign key creation on the user.

Precisely, but it is a problem for a different reason (i.e. not because it's a has-one relation). At the moment, the SQL we run to create Users table is something like that:

CREATE TABLE users (
  id BIGINT PRIMARY KEY,
  FOREIGN KEY (id) REFERENCES profiles.user_id
);

Having a PRIMARY KEY column reference another column is wrong and not intended, which is something we can rather safely assume from the User struct definition.

The solution: do not create foreign keys for Relations whose referencing ("from") column is a primary key (with one exception).


New Tag to disable FK?

We have .WithForeignKeys() to control this on the table level, perhaps it would make sense to have WithForeignKeys(fields ...string) to let users select fields for which FKs should be created. E.g.:

create.WithForeignKeys() // create FKs for all Relations
create.WithForeignKeys("Profile", "Avatar") // create FKs for Profile and Avatar relations, ignore all others

This change should also be backwards compatible. Out of this ticket's scope though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants