bun icon indicating copy to clipboard operation
bun copied to clipboard

(Accidental?) Change in `ForceDelete()` behavior vs. go-pg/pg

Open maximerety opened this issue 1 year ago • 0 comments

Given a Model using the soft deletion feature:

type Model struct {
	// [...]
	DeletedAt *time.Time `bun:",soft_delete"` // previously `pg:",soft_delete"`
}

Previously in go-pg/pg

The query db.Model((*Model)(nil)).ForceDelete() generated DELETE FROM model WHERE deleted_at IS NOT NULL, i.e. performed the hard deletion of all soft deleted rows.

This is because the deletedFlag was forcefully set and taken into account when generating the WHERE clause (isSoftDelete returns true, appendSoftDelete is called, and the clause is added).

Now with bun

The query db.NewDelete().Model((*Model)(nil)).ForceDelete().Exec(ctx) generates DELETE FROM model, i.e. performs the hard deletion of all rows, whether soft deleted or not.

Indeed:

  1. In (q *DeleteQuery) AppendQuery, the deletedFlag is still forcefully applied to the query (query_delete.go#L161), but
  2. The deletedFlag is then ignored by (q *whereBaseQuery) appendWhere (query_base.go#L778) because the new implementation of (q *DeleteQuery) isSoftDelete() returns false (query_delete.go#L205 due to forceDeleteFlag being set).

I wonder if that change was intentional or accidental, as forcing a flag to be set that is not then used seems to be a mistake.

It is possible to restore previous behavior by manually adding a Where("deleted_at IS NOT NULL") clause to the query, but using WhereDeleted() explicitly is not an option since the deletedFlag would still be ignored.

IMHO, keeping the new behavior but letting WhereDeleted() work in conjunction with ForceDelete() would be the nicest option here:

db.NewDelete().Model((*Model)(nil)).ForceDelete().Exec(ctx)
// Generates: DELETE FROM model

db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Generates: DELETE FROM model
// I would like instead: DELETE FROM model WHERE deleted_at IS NOT NULL

What do you think?

maximerety avatar Sep 15 '22 10:09 maximerety