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

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

Closed
maximerety opened this issue Sep 15, 2022 · 0 comments · Fixed by #679
Closed

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

maximerety opened this issue Sep 15, 2022 · 0 comments · Fixed by #679

Comments

@maximerety
Copy link
Contributor

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 added a commit to maximerety/bun that referenced this issue Sep 22, 2022
```go
db.NewDelete().Model((*Model)(nil)).Where("1 = 1").WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: DELETE FROM model WHERE 1 = 1
// After this commit:  DELETE FROM model WHERE 1 = 1 AND deleted_at IS NOT NULL
```

Additionally, the `Where("1 = 1")` trick is no longer required as `WhereDeleted()`
counts as a WHERE clause in mustAppendWhere.

```go
db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: Errors with "bun: Update and Delete queries require at least one Where"
// After this commit:  DELETE FROM model WHERE deleted_at IS NOT NULL
```

This gives us an easy and natural way to hard delete all soft deleted records.

Fixes issue uptrace#673.
maximerety added a commit to maximerety/bun that referenced this issue Sep 22, 2022
```go
db.NewDelete().Model((*Model)(nil)).Where("1 = 1").WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: DELETE FROM model WHERE 1 = 1
// After this commit:  DELETE FROM model WHERE 1 = 1 AND deleted_at IS NOT NULL
```

Additionally, the `Where("1 = 1")` trick is no longer required as `WhereDeleted()`
counts as a WHERE clause in mustAppendWhere.

```go
db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: Errors with "bun: Update and Delete queries require at least one Where"
// After this commit:  DELETE FROM model WHERE deleted_at IS NOT NULL
```

This gives us an easy and natural way to hard delete all soft deleted records.

Fixes issue uptrace#673.
maximerety added a commit to maximerety/bun that referenced this issue Sep 22, 2022
As of Bun 1.1.8, `WhereDeleted()` is ignored when used with `ForceDelete()`.

This commit fixes that problem:

```go
db.NewDelete().Model((*Model)(nil)).Where("1 = 1").WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: DELETE FROM model WHERE 1 = 1
// After this commit:  DELETE FROM model WHERE 1 = 1 AND deleted_at IS NOT NULL
```

Additionally, the `Where("1 = 1")` trick is no longer required as `WhereDeleted()` now counts as a user-defined `WHERE` clause in `mustAppendWhere`:

```go
db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: Errors with "bun: Update and Delete queries require at least one Where"
// After this commit:  DELETE FROM model WHERE deleted_at IS NOT NULL
```

This gives us an easy and natural way to hard delete all soft deleted records.

Fixes uptrace#673.
maximerety added a commit to maximerety/bun that referenced this issue Sep 22, 2022
As of Bun 1.1.8, `WhereDeleted()` is ignored when used with `ForceDelete()`.

This commit fixes that problem:

```go
db.NewDelete().Model((*Model)(nil)).Where("1 = 1").WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: DELETE FROM model WHERE 1 = 1
// After this commit:  DELETE FROM model WHERE 1 = 1 AND deleted_at IS NOT NULL
```

Additionally, the `Where("1 = 1")` trick is no longer required as `WhereDeleted()` now counts
as a user-defined `WHERE` clause in `mustAppendWhere`:

```go
db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: Errors with "bun: Update and Delete queries require at least one Where"
// After this commit:  DELETE FROM model WHERE deleted_at IS NOT NULL
```

This gives us an easy and natural way to hard delete all soft deleted records.

Fixes uptrace#673.
maximerety added a commit to maximerety/bun-docs that referenced this issue Sep 27, 2022
maximerety added a commit to maximerety/bun-docs that referenced this issue Sep 27, 2022
maximerety added a commit to maximerety/bun that referenced this issue Nov 22, 2022
As of Bun 1.1.8, `WhereDeleted()` is ignored when used with `ForceDelete()`.

This commit fixes that problem:

```go
db.NewDelete().Model((*Model)(nil)).Where("1 = 1").WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: DELETE FROM model WHERE 1 = 1
// After this commit:  DELETE FROM model WHERE 1 = 1 AND deleted_at IS NOT NULL
```

Additionally, the `Where("1 = 1")` trick is no longer required as `WhereDeleted()` now counts
as a user-defined `WHERE` clause in `mustAppendWhere`:

```go
db.NewDelete().Model((*Model)(nil)).WhereDeleted().ForceDelete().Exec(ctx)
// Before this commit: Errors with "bun: Update and Delete queries require at least one Where"
// After this commit:  DELETE FROM model WHERE deleted_at IS NOT NULL
```

This gives us an easy and natural way to hard delete all soft deleted records.

Fixes uptrace#673.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant