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

Unmentioned downside for object deletion #61

Closed
JanStevens opened this issue Feb 8, 2018 · 15 comments
Closed

Unmentioned downside for object deletion #61

JanStevens opened this issue Feb 8, 2018 · 15 comments

Comments

@JanStevens
Copy link

This library looks very interesting and promising but one downside I see by storing the snapshots in the same level as the record is that you are totally unaware of any delete operations.

Example I want to keep an exact log of a join association between two records. Using logidze I'm unable to see any historical changes between the two models as there is no way to see any removed associations.

Or I'm missing something?

@amalagaura
Copy link

amalagaura commented Feb 9, 2018

I am also struggling with this and thinking of the best way to model it. Logidze seems to handle the case of an associated record added later. Thus when you ask for an initial version an associated record which was added later will be removed from the association.

To remove a has_many I was thinking of adding a "deleted" column and then removing those from the association after logidze has done it's processing. I am not sure it can fit within logidze. The "deleted" column will also be tracked so the value after logidze processing will be ok to filter on.

@palkan
Copy link
Owner

palkan commented Feb 9, 2018

@JanStevens

Or I'm missing something?

You're right. Logidze doesn't take into account deleted records (and we should add to the limitations list in our wiki – /cc @charlie-wasp).

Actually, versioned associations feature is just a syntactic sugar for applying at to associated records.

Since we generate logs DB-side, we know nothing about associations.

There could be a workaround for deleted records when using paranoia; but not sure it's possible to deal with detached records (i.e. when we change the parent record).

@amalagaura

I was thinking of adding a "deleted" column

What you're talking about sound pretty similar to paranoia) As I've already mentioned above, we can use it somehow.

@palkan palkan added the awaiting response Awaiting reporter's response label Feb 9, 2018
@amalagaura
Copy link

amalagaura commented Feb 9, 2018

@palkan Sounds like you could possibly have a delete_tracking_column (default to :deleted_at) option and Logidze could check the value for that column and if it is not nil it could filter the association where you check associated records? Is that all it needs to integrate? I am not sure of how the delete tracking of paranio will integrate with the rest of the Logidze features.

@JanStevens
Copy link
Author

I think https://github.com/jhawthorn/discard is a better solution or act as archival. Both it all comes down to the same, you hide deleted records for the UI but when showing history you can include them.

The only problem is then that you lose database constraints for associations (or you have to bring the deleted object back to the living). Sounds a bit messy and possibly dangerous IMO.

@jasonjho
Copy link

Is associations versioning still considered experimental and if so, what are the implications to using this feature in production?

@palkan
Copy link
Owner

palkan commented May 14, 2018

@jasonjho "Experimental" in this case means that there could be some yet-unknown limitations (i.e. not mentioned on the list. So, it depends on your use case; we're trying to keep track (and fix if possible) all known issues, but we do not guarantee that the list is final.

@patleb
Copy link

patleb commented May 17, 2018

As a suggestion, what about a separate table logidze_trashes with this kind of structure:

create_table :logidze_trashes do |t|
  t.bigint    :record_id,    null: false
  t.string    :record_type,  null: false
  t.jsonb     :log_data,     null: false

  t.timestamp :created_at,   null: false
end

add_index :logidze_trashes, :log_data, using: :gin
add_index :logidze_trashes, [:record_type, :created_at, :record_id], unique: true, order: { created_at: :desc }
add_index :logidze_trashes, [:record_type, :record_id, :created_at], order: { created_at: :desc }

and an associated model Logidze::Trash which could possibly cover the application logic?

The idea would be that a trigger could be added alongside the one for versionning, but for the deletion case and pushes the serialized record into the logidze_trashes table.

@palkan
Copy link
Owner

palkan commented May 18, 2018

what about a separate table logidze_trashes

I don't think that soft-delete feature is something that should be a part of Logidze. I'd better consider adding integrations with the existing solutions for this, like paranoia and discard gems. If need to update our internal APIs for that, let's do that.

@patleb
Copy link

patleb commented May 18, 2018

Soft-deleting is one use-case for that kind of solution, but I was trying to propose something that could make Logidze similar to PaperTrail in term of functionalities. Right now, create and update are covered, but delete isn't and it's not about soft-delete, but the whole point of versionning/auditing (and possibly help simplifying association cases by normalization). I understand though that it might not be your goal to replace other versionning/auditing gem, since they cover a lot. But, if by any chance you need some help to make it happen, I would be willing to help with that.

@palkan
Copy link
Owner

palkan commented May 18, 2018

but delete isn't and it's not about soft-delete, but the whole point of versionning/auditing

Frankly speaking, Logidze is not about auditing but about versioning; and the difference here is that in versioning case you're interested for the history of the specified record; in auditing case you're interested in the actions have been made, usually project-wide and not bound to a single object.

Keeping trashes won't help us to solve the initial problem with associations, btw.

Could you, please, provide an example of a problem that can be solved using the approach you proposed and cannot be solved using soft-delete approach? Maybe, I just don't get it

@patleb
Copy link

patleb commented May 18, 2018

with_responsible is the audit part of Logidze (and possibly with_meta). Maybe trashes isn't a good name for the problem at hand, but I was refering to this as to how paper_trail solves a part of the problem with associations (it might not be the best way though... I haven't put too much toughts into it).

I'm not saying soft-delete doesn't solve the problem, but suggesting that Logidze could be better at it by keeping track of what was deleted by using triggers and a separate table which is the equivalent of what is done with the soft-delete approach at the application level with a scope. What I think could be better is not having to resort to scopes in order to simulate a delete and keeping the performance of a clean table even though a big part could be deleted.

@seanlinsley
Copy link

I'm coming to Logidze after having used Paper Trail for many years on a large project. Logidze is appealing b/c of the significantly smaller disk usage (Paper Trail records all attrs, even if they didn't change), but we need to track deletions for auditing purposes.

With as much code as we have it's probably infeasible to roll out gems like paranoia and discard. I'd also be worried that query performance with those gems would suffer b/c of the index on the boolean field indicating whether a record has been deleted.

Since it doesn't sound like y'all think this belongs in the Logidze gem, I'd be happy to fork this project & re-release under a different name.

Re: finding deleted records on an association, one way we could implement that without requiring a sequence scan on the table would be to store all foreign keys in a separate, indexed JSONB field.

@palkan palkan added discussion and removed awaiting response Awaiting reporter's response labels May 28, 2018
@palkan
Copy link
Owner

palkan commented May 28, 2018

Since it doesn't sound like y'all think this belongs in the Logidze gem, I'd be happy to fork this project & re-release under a different name.

I'd like to suggest building a plugin instead of a separate project if you want to re-use the existing functionality. From our side we can refactor our internals to make Logidze easily to extend.

@seanlinsley
Copy link

It seems like most of the code changes would be in the SQL functions. Is that really something that belongs as a plugin? It's not clear what the API boundary would be between the two gems.

Ideally Logidze itself would have the ability to store data in a separate table (and thus track deletions), but forking seems easier than building this functionality as a plugin.

@oleg-kiviljov
Copy link
Contributor

I have setup paper_trail only to track 'delete' events and for everything else I use logidze. I personally think this is the best solution right now.

@palkan palkan closed this as completed in 2d2500c Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants