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

log SQL for association query #686

Merged
merged 5 commits into from Sep 17, 2022
Merged

log SQL for association query #686

merged 5 commits into from Sep 17, 2022

Conversation

briskt
Copy link
Contributor

@briskt briskt commented Jan 28, 2022

As I described in Slack, the automatic CREATE for many-to-many relationship join table doesn't get logged. It looks like this may log more than this one query, but I'm not familiar enough with this code to know what other situations are addressed.

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.

Hi @schparky,
Thank you for your contribution.

I took a look at this PR along with the surrounding code and context, and I think replacing the low-level exec methods (c.TX.Exec() and c.Store.Exec()) with a logging-enabled higher-level exec method could be better than adding log() in more places.

Could you please update the PR with the patch below? Just let me know if you don't have enough time. I will proceed with it as a part of the ongoing clean-up tasks.

diff --git a/executors.go b/executors.go
index 37b03b3..a290543 100644
--- a/executors.go
+++ b/executors.go
@@ -286,7 +286,7 @@ func (c *Connection) Create(model interface{}, excludeColumns ...string) error {
                                        }
                                        stm := after[index].AfterProcess()
                                        if c.TX != nil && !stm.Empty() {
-                                               _, err := c.TX.Exec(c.Dialect.TranslateSQL(stm.Statement), stm.Args...)
+                                               err := c.RawQuery(c.Dialect.TranslateSQL(stm.Statement), stm.Args...).Exec()
                                                if err != nil {
                                                        return err
                                                }
@@ -297,14 +297,7 @@ func (c *Connection) Create(model interface{}, excludeColumns ...string) error {
                                for index := range stms {
                                        statements := stms[index].Statements()
                                        for _, stm := range statements {
-                                               if c.TX != nil {
-                                                       _, err := c.TX.Exec(c.Dialect.TranslateSQL(stm.Statement), stm.Args...)
-                                                       if err != nil {
-                                                               return err
-                                                       }
-                                                       continue
-                                               }
-                                               _, err = c.Store.Exec(c.Dialect.TranslateSQL(stm.Statement), stm.Args...)
+                                               err := c.RawQuery(c.Dialect.TranslateSQL(stm.Statement), stm.Args...).Exec()
                                                if err != nil {
                                                        return err
                                                }

@sio4 sio4 self-assigned this Sep 17, 2022
@sio4 sio4 added bug Something isn't working s: accepted This proposal was accepted. Someone can start working on it. labels Sep 17, 2022
@briskt
Copy link
Contributor Author

briskt commented Sep 17, 2022

The patch didn't work, possibly because I hadn't yet merged in the latest commits. I did it manually, so please check my work.

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! Thanks!

@sio4 sio4 merged commit f192ebb into gobuffalo:main Sep 17, 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. s: fixed was fixed or solution offered labels Sep 17, 2022
@sio4 sio4 mentioned this pull request Sep 20, 2022
30 tasks
@sio4 sio4 added the s: fixed was fixed or solution offered label Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working s: fixed was fixed or solution offered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants