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

Pgxmock batch example in Go #212

Closed
wants to merge 22 commits into from

Conversation

giuliocn
Copy link

@giuliocn giuliocn commented Jul 19, 2024

This PR would like to improve my work on #201

@giuliocn giuliocn changed the title Pr batch example Pgxmock batch example Jul 19, 2024
@giuliocn giuliocn changed the title Pgxmock batch example Pgxmock batch example in Go Jul 19, 2024
@giuliocn giuliocn marked this pull request as draft July 19, 2024 14:49
@pashagolub
Copy link
Owner

Why didn't you just reopen the previous pull request?

examples/batch/batch.go Outdated Show resolved Hide resolved
examples/batch/batch_test.go Outdated Show resolved Hide resolved
@giuliocn
Copy link
Author

giuliocn commented Jul 19, 2024

Why didn't you just reopen the previous pull request?

@pashagolub I have no right to re-open closed PRs in this repository. I cannot see the button near "Comment".

@pashagolub pashagolub self-assigned this Jul 22, 2024
@pashagolub pashagolub added the documentation Improvements or additions to documentation label Jul 22, 2024
@giuliocn giuliocn requested a review from pashagolub July 26, 2024 14:26
@giuliocn
Copy link
Author

giuliocn commented Aug 1, 2024

The example could be completed by serving all the results within an HTML page. What Do you think?

@pashagolub
Copy link
Owner

Sorry for delay with answer.

I want examples to be as real as possible showing the real life use cases. That why I don't buy this one:

	example.batch.Queue("SELECT * FROM ledger")
	example.batch.Queue("SELECT * FROM ledger WHERE amount = 1")

I will never in my life execute such a batch. Why? The first result set has it all.

@pashagolub
Copy link
Owner

Tests failed and coverage is below threshold.

=== RUN   TestExpectBatch
--- PASS: TestExpectBatch (0.00s)
=== RUN   TestExpectBegin
    .\pgxmock\examples\batch\batch_test.go:50: there were unfulfilled expectations: there is a remaining expectation which was not matched: ExpectedBegin => expecting call to Begin() or to BeginTx()
--- FAIL: TestExpectBegin (0.00s)
FAIL
coverage: 6.7% of statements
FAIL	github.com/pashagolub/pgxmock/v4/examples/batch	0.029s

}
sql := `INSERT INTO metadata (title, authors, subject, description)
VALUES
(` + strings.Join(fields[:4], ", ") + ");"
Copy link
Owner

Choose a reason for hiding this comment

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

We don't do parameters concatenation, this is the possible place for SQL injection. We should use parameterized queries.

defer example.Close()

// Add an example to database table
example.insertRow(
Copy link
Owner

Choose a reason for hiding this comment

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

Why we insert here and in databaseSetup()?


}

func (ex *ExampleBatch) requestBatch() (err error) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any need in this function. Let's keep it simple

output strings.Builder
}

func (ex *ExampleBatch) insertRow(fields ...string) (err error) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any value in insertRow function. And it's not covered by tests. It's better to remove it and keep things simple

Comment on lines 16 to 17
Begin(context.Context) (pgx.Tx, error)
Exec(context.Context, string, ...interface{}) (pgconn.CommandTag, error)
Copy link
Owner

Choose a reason for hiding this comment

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

Nor Begin neither Exec used anywhere in code. Although this is not an error, we want to show our user that interfaces must be minimal

@giuliocn
Copy link
Author

giuliocn commented Sep 5, 2024

@pashagolub Can you explain in what order a single row is retrieved with BatchResults? Can I consume a Rows object independently for each query?

// For example,  
// Could I consume only the first row of each query? 

Thanks for your comments. 💡

@pashagolub
Copy link
Owner

Yes, this is the idea behind the batch. You send all queries at once and then receive all results at once. And you are free to work with every result set as you wish.

If the example below the magic of sending and execution happened during SendBatch(). And then you just work with results you got back firing results.Exec(). This is not you really execute something, it's just to tell pgx you treat all statements as Exec(). But, of course, you can change it to Query() or even QueryRow(), see here.

func (pg *postgres) BulkInsertUsers(ctx context.Context, users []model.User) error {
  query := `INSERT INTO users (name, email) VALUES (@userName, @userEmail)`

  batch := &pgx.Batch{}
  for _, user := range users {
    args := pgx.NamedArgs{
      "userName": user.Name,
      "userEmail": user.Email,
    }
    batch.Queue(query, args)
  }
  
  results := pg.db.SendBatch(ctx, batch)
  defer results.Close()

  for _, user := range users {
    _, err := results.Exec()
    if err != nil {
      var pgErr *pgconn.PgError
      if errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation {
          log.Printf("user %s already exists", user.Name)
          continue
      }

      return fmt.Errorf("unable to insert row: %w", err)
    }
  }

  return results.Close()
}

Comment on lines +140 to +144
example.SendCustomBatch([]string{
"SELECT title FROM metadata",
"SELECT authors FROM metadata",
"SELECT subject, description FROM metadata",
})
Copy link
Owner

Choose a reason for hiding this comment

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

I don't buy it. It should be one query

SELECT title, authors, subject, description FROM metadata

This is bad example.

Comment on lines +147 to +155
for _, query := range example.batch.QueuedQueries {
fmt.Println(query.SQL)
rows, _ := example.results.Query()
for rows.Next() {
values, _ := rows.Values()
fmt.Println(values)
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Useless piece of code. No purpose. No meaning.

Comment on lines +19 to +23
type exampleBatch struct {
batch *pgx.Batch // batch pointer
results pgx.BatchResults // query results
db PgxPoolInterface // database connection
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the meaning of this.

@pashagolub
Copy link
Owner

Unfortunately, I can't accept this work. This is not a proper example of using pgx.Batch.
Thanks for your time.

@pashagolub pashagolub closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants