Ignore old federated post edits (ref #4529)

[Nutomic]

No description provided.

[Nutomic]

Unfortunately this is rather verbose, no way to avoid that unless we add a trait (which would be even more verbose and wouldnt be used anywhere else).

[dullbananas]

The timestamp should be checked by calling filter after set in each diesel upsert. Checking it separately in Object::verify is slower and doesn't work if multiple updates for the same object are processed at the same time.

If you don't want Object::from_json to return Err when the update is skipped, then it needs to ignore the diesel::result::Error::NotFound returned by the upsert.

[phiresky]

This looks generally fine to me, but I agree with dullbanas that filter the verify might not be the right place to do it?

To clarify on their second point, if you do it here you have a race condition where if two updates are processed simultaneously then the "verify" of the second can be called after the db update of the first, which then causes the older update to override the newer one.

The only guaranteed way to do this correctly is if the filtering happens as a WHERE in the update query.

[Nutomic]

Good suggestion, Ive changed it like that. Also added separate insert_apub() methods so that creates via api dont need to do upsert or timestamp check.

[phiresky]

not sure but I think this will result in an error if the value is not updated? as in, the query will return zero rows which will cause get_result to return a row not found error unless you add .optional()

[dullbananas]

Correct

[Nutomic]

That should be fine, if we are receiving an old comment while already storing a newer one, it throws an error and gets rejected. We dont need to do anything in that case.

[dullbananas]

Assuming @Nutomic is correct about returning an error when skipping update