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 eager creation for associations by adding where clause #607

Merged
merged 2 commits into from
Sep 24, 2022

Conversation

xixinjie
Copy link
Contributor

@xixinjie xixinjie commented Dec 4, 2020

Problem

When try to use db.Eager().ValidateAndCreate(user) to create a user and underlying has_many books, it succeeded for the first time when books table is empty, but failed to save the books in following creations.

How to fix

Add where clause when check if the books exist as shown in the PR.

exists, errE := Q(c).Where("id = ?", id).Exists(i)

Samples

type User struct {
	ID string `json:"id" db:"id"`
	Name string `json:"name" db:"name"`
	Books []Book `json:"books" has_many:"books" fk_id:"user_id"`
	CreatedAt time.Time `json:"created_at" db:"created_at"`
	UpdatedAt time.Time `json:"updated_at" db:"updated_at"`
}

type Book struct {
	ID string `json:"id" db:"id"`
	Name string `json:"name" db:"name"`

	UserID string `json:"user_id" db:"user_id"`
	User *User `json:"user" belongs_to:"users"`

	CreatedAt time.Time `json:"created_at" db:"created_at"`
	UpdatedAt time.Time `json:"updated_at" db:"updated_at"`
}
	u := &User{
		ID:        "1",
		Name:      "Foo",
		Books:     []Book{{
			ID:        "1",
			Name:      "Hello World!",
		}},
	}

	vErrs, err := db.Eager().ValidateAndCreate(u)
	if err != nil {
		panic(fmt.Sprintf("failed to create user: %s", err))
	}

	if vErrs.HasAny() {
		panic(fmt.Sprintf("failed to match validation: %s", vErrs))
	}

	log.Printf("created a new user: %s", u.ID)

SQL logs:

sql - INSERT INTO `users` (`created_at`, `id`, `name`, `updated_at`) VALUES (:created_at, :id, :name, :updated_at)
sql - SELECT EXISTS (SELECT books.created_at, books.id, books.name, books.user_id, books.updated_at FROM books AS books WHERE id = ?) | ["1"]
INSERT INTO `books` (`created_at`, `id`, `name`, `user_id`, `updated_at`) VALUES (:created_at, :id, :name, :user_id, :updated_at)

@xixinjie xixinjie requested a review from a team as a code owner December 4, 2020 20:41
@xixinjie
Copy link
Contributor Author

xixinjie commented Jan 4, 2021

@stanislas-m @markbates any chance to review or any suggestion?

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you, could you please add a failing test case first which shows the behaviour you describe - first to verify your findings and second to verify your fix!

@@ -265,7 +265,8 @@ func (c *Connection) Create(model interface{}, excludeColumns ...string) error {
if IsZeroOfUnderlyingType(id) {
return c.Create(m.Value)
}
exists, errE := Q(c).Exists(i)

exists, errE := Q(c).Where("id = ?", id).Exists(i)
Copy link
Member

@aeneasr aeneasr Jan 4, 2021

Choose a reason for hiding this comment

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

The ID column is variable depending on the struct and should therefore not be hardcoded. m has a function whereID which should be used instead.

@sio4
Copy link
Member

sio4 commented Sep 18, 2022

Hello @xixinjie,

Thank you for reporting this issue and contribution. I took a look at the issue and definitely the original usage was problematic. Please resolve the review comment, then I will take care of the next steps along with test cases.

I am not sure if you still have an interest in this issue. If you do not respond, I will take over this issue.

@sio4 sio4 self-assigned this Sep 18, 2022
@sio4 sio4 added bug Something isn't working s: accepted This proposal was accepted. Someone can start working on it. f: associations the associations feature in pop labels Sep 18, 2022
@sio4 sio4 added this to the v6.0.7 milestone Sep 18, 2022
@sio4 sio4 mentioned this pull request Sep 20, 2022
30 tasks
@sio4 sio4 changed the base branch from main to fix-missing-where September 24, 2022 05:02
@sio4 sio4 merged commit 36e6fe0 into gobuffalo:fix-missing-where Sep 24, 2022
@sio4 sio4 added s: fixed was fixed or solution offered and removed s: accepted This proposal was accepted. Someone can start working on it. labels Sep 24, 2022
Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

LGTM

minor fix will be on another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working f: associations the associations feature in pop s: fixed was fixed or solution offered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants