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: pass ManyToMany onUpdate option to foreign key metadata #5714

Merged
merged 1 commit into from
May 22, 2021

Conversation

fan-tom
Copy link
Contributor

@fan-tom fan-tom commented Mar 18, 2020

Closes: #4980

I'm not sure if onDelete/onUpdate options are handled properly, it seems to me that they must be applied to foreign keys pointing to OPPOSITE entity, not to entity they are used in, but such changes break existing tests, so I considered to fix this issue not breaking them.

@fan-tom fan-tom force-pushed the fix-4980 branch 3 times, most recently from d062baf to d471187 Compare March 19, 2020 07:58
@pleerock
Copy link
Member

Based on your comments what I understood is that we need to set onUpdate and onDelete for every column in the junction table separately, am I right? If so, this temporary change doesn't make sense and we probably need to remove this option and introduce it for @JoinTable's joinColumn option?:

@ManyToMany(
        () => Book,
        book => book.authors
    )
    @JoinTable({
        name: "author_to_books",
        joinColumn: {
            name: "author_id",
            referencedColumnName: "id",
            onDelete: "CASCADE",
            onUpdate: "CASCADE"
        },
        inverseJoinColumn: {
            name: "book_id",
            referencedColumnName: "id",
            onDelete: "CASCADE",
            onUpdate: "CASCADE"
        },
    })

Also, before any change will be merged we need to be sure nothing will be broken for exist users and ORM wont generate any new queries for those who update the version. I'm a bit scary to merge such kind of things.

@fan-tom
Copy link
Contributor Author

fan-tom commented Jun 14, 2020

I just want to say that when I write

class Author {
  @ManyToMany(type => Book, {
    onUpdate: 'CASCADE',
    onDelete: 'CASCADE',
  })
  books: Book[];
}

class Book {
  @JoinTable()
  @ManyToMany(type => Author, {
    onUpdate: 'NO ACTION',
    onDelete: 'NO ACTION',
  })
  authors: Author[];
}

I expect that I can update id's of books and get fk updated accordingly, but cannot update author's id's, like when you use ManyToOne and OneToOne, you apply those decorators inside referencing table and onDelete/onUpdate options are applied to links to REFERENCED table. But in case of ManyToMany when I set onDelete: 'CASCADE' inside Author entity, I get it applied to link to CURRENT table(Author), instead of link to REFERENCED table(Book).

What about affecting current users, I just added missing property handling code for onUpdate option, that works the same way onDelete works currently, just like people requested in issue, and fixed the bug that single onDelete option was used for both sides, so I don't understand what things you are scary about.

@fan-tom
Copy link
Contributor Author

fan-tom commented Jun 30, 2020

Is it related issue? #3505

Copy link
Contributor

@imnotjames imnotjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Can you rebase and get the new test suite kicking off?

@FabianScheidt
Copy link

I am facing both of the issues that are mentioned here. Wouldn't it make sense to merge @fan-tom's suggestion now and later come up with a change that involves the @JoinTable decorator as suggested by @pleerock?

@Ami777
Copy link

Ami777 commented Apr 27, 2021

Why isn't this merged... 1 year later :|?

@jcrben
Copy link

jcrben commented May 20, 2021

@fan-tom up for rebasing for a merge as suggested by @imnotjames?

@fan-tom
Copy link
Contributor Author

fan-tom commented May 21, 2021

rebased

@AlexMesser AlexMesser merged commit 198d2c5 into typeorm:master May 22, 2021
@AlexMesser
Copy link
Collaborator

thank you for contribution!

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.

onUpdate: 'CASCADE' doesn't work on many-to-many relation
7 participants