rel icon indicating copy to clipboard operation
rel copied to clipboard

[Bug] Change `assoc many` slice and `Update`/`Insert` with `Changeset`

Open qRoC opened this issue 3 years ago • 8 comments

Append

Example

type Main struct {
    ID        int `db:",primary"`
    Childs []*Child `ref:"id" fk:"main_id" autosave:"true" autoload:"true"`
}

type Child struct {
    ID        int `db:",primary"`
    MainID int
    Name string
}

main := new(Main)
repo.Find(ctx, main, rel.Eq("id", id))
changeset := rel.NewChangeset(main)
main.Childs = append(main.Childs, &Child{...})
repo.Update(ctx, main, changeset)

Actual result

panic("rel: invalid mutator")

Change

Example

main := new(Main)
repo.Find(ctx, main, rel.Eq("id", id))
changeset := rel.NewChangeset(main)
main.Childs[0].Name = "foo"
main.Childs[1].Name = "bar"
repo.Update(ctx, main, changeset)

Actual result

generate invalid queries

DELETE FROM "child" WHERE "child"."main_id"=$1;
INSERT INTO "child" ("name","main_id") VALUES ($1,$2),($3,$4) RETURNING "id"

qRoC avatar Aug 23 '22 07:08 qRoC

Full example:

type Parent struct {
	ID     int      `db:",primary"`
	Childs []*Child `ref:"id" fk:"parent_id" autosave:"true" autoload:"true"`
}

type Child struct {
	ID       int `db:",primary"`
	ParentID int
	Name     string
}

func(repo rel.Repository) {
    ctx := context.Background()

    repo.Exec(ctx, "CREATE TABLE IF NOT EXISTS parents (id int PRIMARY KEY)")
    repo.Exec(ctx, "CREATE TABLE IF NOT EXISTS children (id int PRIMARY KEY, parent_id int NOT NULL, name text, FOREIGN KEY (parent_id) REFERENCES parents (id))")

    p := new(Parent)
    if err := repo.Find(ctx, p, rel.Eq("id", 1)); err != nil {
        p.ID = 1
        p.Childs = []*Child{
            {ID: 1, ParentID: 1, Name: "First"},
            {ID: 2, ParentID: 1, Name: "Second"},
        }
        repo.Insert(ctx, p)
    }
    changeset := rel.NewChangeset(p)

    // panic: rel: invalid mutator
    if false {
        p.Childs = append(p.Childs, &Child{ID: 3, ParentID: 1, Name: "Third"})
        repo.Update(ctx, p, changeset)
    }

    // panic: rel: invalid mutator
    if false {
        p.Childs = []*Child{p.Childs[1]}
        repo.Update(ctx, p, changeset)
    }

    // DELETE FROM "children" WHERE "children"."parent_id"=$1;
    // INSERT INTO "children" ("name","parent_id") VALUES ($1,$2),($3,$4) RETURNING "id";
    if false {
        p.Childs[0].Name = "Foo"
        p.Childs[1].Name = "Bar"
        repo.Update(ctx, p, changeset)
    }
}

qRoC avatar Aug 23 '22 08:08 qRoC

@qRoC created a related fix here https://github.com/go-rel/rel/pull/311, please test it if you can

but, since your example is not using primary key generated in database, it's tricky to make repo.Update call successful REL can't determine whether to update or insert child 3 correctly, because the logic is based on whether ID is empty or not

Fs02 avatar Aug 26 '22 14:08 Fs02

Append - no panic, but generate queries:

DELETE FROM "children" WHERE "children"."parent_id"=$1;
INSERT INTO "children" ("parent_id","id","name") VALUES ($1,DEFAULT,DEFAULT),($2,DEFAULT,DEFAULT),($3,$4,$5) RETURNING "id"

Update - no panic, but generate queries:

DELETE FROM "children" WHERE "children"."parent_id"=$1;
INSERT INTO "children" ("name","parent_id") VALUES ($1,$2),($3,$4) RETURNING "id"

REL can't determine whether to update or insert child 3 correctly, because the logic is based on whether ID is empty or not

Main problem - rel generates absolutely wrong queries:

  1. You can't use DELETE+INSERT because it breaks deep assocs with cascading delete.
  2. Insert queries is not valid.

qRoC avatar Aug 29 '22 09:08 qRoC

I will do a PR with fix today

qRoC avatar Aug 29 '22 10:08 qRoC

You can't use DELETE+INSERT because it breaks deep assocs with cascading delete.

Nested association of has many is not supported at the moment because of complexity in the implementation both case generate DELETE+INSERT query because REL can't determine the operation correctly, so it replace the association which is default behavior when using struct based insert/update

Fs02 avatar Aug 29 '22 14:08 Fs02

because REL can't determine the operation correctly

Can you show an example? I see only one case - ID is not serial + no changeset.

qRoC avatar Aug 29 '22 14:08 qRoC

This fix my issue: https://github.com/qRoC/rel/commit/45adbeb9589f0e28bb89b4b74e7c21ec64f521ca

I don't create a PR because it changes the default behavior.

qRoC avatar Aug 29 '22 15:08 qRoC

Can you show an example? I see only one case - ID is not serial + no changeset

yes, if not using changeset, the only way to tell a record is new or not is just by checking the primary field

Fs02 avatar Aug 30 '22 12:08 Fs02