Soft Delete has no way to opt out

nii236 commented 3 years ago

This is related to the feature added in #716

What version of SQLBoiler are you using (`sqlboiler --version`)?

v4.2.0

What is your database and version (eg. Postgresql 10)

Postgres 11

If this happened at generation time what was the full SQLBoiler command you used to generate your models? (if not applicable leave blank)

If this happened at runtime what code produced the issue? (if not applicable leave blank)

What is the output of the command above with the `-d` flag added to it? (Provided you are comfortable sharing this, it contains a blueprint of your schema)

Please provide a relevant database schema so we can replicate your issue (Provided you are comfortable sharing this)

CREATE TABLE pilots (
  id integer NOT NULL,
  name text NOT NULL,

  deleted_at TIMESTAMP
);

Further information. What did you do, what did you expect?

What I did:

Generate SQLBoiler
Soft deleted a record
Attempt to list soft deleted records so the admin UI can undelete them
Attempt to list soft-deleted and non-soft-deleted records so the admin UI can filter between the two

What happened:

deleted_at is not null is appended to all retrieval funcs, preventing me from using the generated code to retrieve soft deleted records.

What I expected:

The ability to retrieve all records
The ability to retrieve all records that are soft deleted
The ability to retrieve all records that are not soft deleted

aarondl commented 3 years ago

I'm okay with this functionality getting in. Need some proposals for what it would look like. Sneak it in using context until v5? New overloaded methods?

namco1992 commented 3 years ago

@nii236 @aarondl First of all, soft delete to me is still a deletion. We shouldn't consider retrieving the deleted records in the normal workflows just because we keep the records for backtracking. For your software, a deletion is a deletion.

The use case in this issue seems to be a flag that allows the admin to toggle and should be handled by updating a flag column instead of soft delete. It doesn't look like a deletion for me coz you want to list the deleted records and un-delete some of them.

However, it's just my personal view and I'm happy to discuss this with you further. I just feel that we are not treating a deletion as a deletion which could complicate the situation, that's all.

aarondl commented 3 years ago

@namco1992 what is the purpose of a soft delete if you can never retrieve the soft-deleted records though? Why not just actually delete the data? :) I agree with you in so far as by default - without going out of your way - sqlboiler should respect soft deleted records everywhere by omitting them. But I'm not opposed to being able to use sqlboiler to retrieve the soft deleted records for admin interfaces or other use cases (trash bin for users etc).

nii236 commented 3 years ago

Hey @namco1992, @aarondl, thanks for the discussion here.

Having the default behaviour filtering out the deleting records is totally fine. But if i were to explicitly pass in deleted_at IS NOT NULL, I should be able to retrieve the deleted records.

In its current form, even with this passed in, the query will force deleted_at IS NULL into the query, making it impossible for me to retrieve deleted records.

namco1992 commented 3 years ago

@aarondl hmm then I think my use cases might be too narrowed 😂 I only look into the soft deleted records manually for auditing purposes. As a general ORM-ish tool it's reasonable to support these use cases. Now I agree with you and @nii236 that we should consider having a way to retrieve the soft deleted records when needed.

aarondl commented 3 years ago

Okay. So we agree on "what". Let's start on "how". Any proposals for an API?

nii236 commented 3 years ago

These two lines require a way to conditionally bypass the deleted_at IS NOT NULL clause.

Is this possible with either querymods qm.WithDeleted() or with the generated where helpers boil.UserWhere.WithDeleted?

hikyaru-suzuki commented 3 years ago

I have same problem now. I will use this solution until that problem solved. By the way, my usecase is using boil.AfterDeleteHook for users table. I used Reload method in the hook to notice deleted user to another system, But query result is sql: no rows in result set because that is is null search.

id := 1
u := &models.User{}
err := models.NewQuery(
   qm.Select("*"),
   qm.From(models.TableNames.Users),
   models.UserWhere.ID.EQ(id),
   models.UserWhere.DeletedAt.IsNotNull(),
).Bind(ctx, exec, u)

aarondl commented 3 years ago

The reason this is stagnating is there's just no good solution to this.

Overloads

models.UsersWithDeleted(...qms).All(...)
models.FindDeletedUser(...)

Query mod

The query mod could only work if we changed the way Find builds queries. Right now it only accepts arguments.

Breaking change to accept a bool

models.Users(withDeleted, ...qms).All()
models.FindUser(..., withDeleted)

hikyaru-suzuki commented 3 years ago

Why this query is overloads? Would you please tell me FindDeletedUser arguments? if that has primary key, generated query should fast.

models.FindDeletedUser(...)

nii236 commented 3 years ago

It should also be noted that if global generation (add-global-variants) and soft deletion is enabled (add-soft-deletes) then SQLBoiler will fail the generation step.

$ ./bin/sqlboiler ./bin/sqlboiler-psql --tag db --output ./server/db --config server/sqlboiler.toml
Error: unable to generate output: failed to format template

 2120   }
 2121
 2122   return rowsAff, nil
 2123 }
 2124
>>>> func (q areaQuery) DeleteAllG(, hardDelete bool) (int64, error){
 2126   return q.DeleteAll(boil.GetDB(), hardDelete)
 2127 }
 2128
 2129 // DeleteAll deletes all matching rows.
 2130 func (q areaQuery) DeleteAll(exec boil.Executor, hardDelete bool) (int64, error){

: 2125:31: expected type, found ','

aarondl commented 3 years ago

Ah. The global variants thing isn't as well tested generally. Fixed that bug on dev.

nii236 commented 3 years ago

Overloads

Query mod

Breaking change to accept a bool

I vote for using overloads as a good approach:

They will only be available if add-soft-deletes is enabled
It will prevent generated code bloat for the users who do not want to opt into soft deletion
It also avoids breaking the existing API, only extending it

models.UsersWithDeleted(...qms).All(...)
models.FindDeletedUser(...)

aarondl commented 3 years ago

Running into this issue again so I think I'm going to make a quick fix to do something horrible ^_^

I'm going to add a query mod. I'm in a bit of a rush so I'm not going to fix the following two spots by adding the overloads but it could easily be done by anyone at any later date:

https://github.com/volatiletech/sqlboiler/blob/master/templates/14_find.go.tpl#L50 https://github.com/volatiletech/sqlboiler/blob/master/templates/20_exists.go.tpl#L47

aarondl commented 3 years ago

Somewhat hilariously my linked commit omits the definition of the exported query mod itself but it now exists as of a later commit on dev branch.

volatiletech / sqlboiler