-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] Added a mechanism to track when the public UserProfile data was last updated #62
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
base: dev
Are you sure you want to change the base?
[WIP] Added a mechanism to track when the public UserProfile data was last updated #62
Conversation
include/session/config/base.hpp
Outdated
/// - `source` -- The config data that the diffs conflicted with. | ||
/// | ||
/// Outputs: None | ||
virtual void resolve_conflicts(dict& data, oxenc::bt_dict& diff, const dict& source) {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual void resolve_conflicts(dict& data, oxenc::bt_dict& diff, const dict& source) {}; | |
virtual void resolve_conflicts(dict& data, oxenc::bt_dict& diff, const dict& source) {} |
I certainly understand the need for this change, but I don't think we can (maintainably) do it like this, because diff resolution where it's being applied here (i.e. as an alternative to apply_diff) would have to deal with all of the complexities that I'm also worried about the interplay between upgraded and non-upgraded versions applying this to an existing field: if app A has new conflict resolution, and app B has old conflict resolution, then as soon as a conflict occurs they will both attempt to push a merged (resolved) update, but those updates will not be the same, and so then they will try to push another merge, and so on. |
An alternative we might consider is to allow different tie-breaking mechanisms at the config type level: i.e. the class would declare that in the case of conflicting scalar values you can: "pseudo-random", "prefer greater", "prefer smaller". (But that does nothing to resolve the new-old version conflicting updates, which doesn't have an immediately obvious solution) |
Ah I forgot about the backwards compatibility for deterministic merging 😞
Mm yea I think something like this could work well for a lot of cases (eg. the
It's not ideal (and potentially a nightmare if we end up with multiple versions and want to maximise compatibility...) but I was wondering whether there would be a way to add some form of "merge logic version" into the data where the new conflict resolution code is only used when both "merge logic version" values are higher than the version that the custom logic was added - earlier (or missing) values would just fallback to the existing "pseudo-random" logic That'd allow us to resolve edge-cases like this in future versions without breaking backwards compatibility (it just means users running different versions would be subject to merge conflict edge-cases) Note: Using "merge logic version" here instead of "version" as I don't know whether we'd want to have a separate versioning mechanism between the merging and other libsession/config logic 🤷 |
• Reverted the previous changes • Added "t" to indicate the timestamp the profile pic was last updated via a user-driven change • Added new "P", "Q" and "T" keys to UserProfile to represent re-uploaded profile pic data • Added logic to auto-update the "t" and "T" timestamps when content is changed
This PR adds
profile_updated
to theUserProfile
config, as well as aset_reupload_profile_pic
function to allow us to distinguish between user-driven profile updates and automatic uploads done by the clients.The
UserProfile
will return theprofile_pic
for whichever was last updated, and automatically update the appropriate timestamps when the user has changed the data to prevent the edge-case where client-side caches can incorrectly replace "new" profile data with "old" data.Old Content:
~~Currently when there are conflicts between two configs the code will sort the configs by hash and resolve the conflicts in a deterministic way, unfortunately this is not always the behaviour that we want.
Currently the
ConvoInfoVolatile
config can have conflicts with it's thelast_read
value and the conflict resolution deterministically picks one of the two values, relying on the client to "fix" any mistakes it may have made (in this case the clients update the config post-merge to use the largestlast_read
value).As part of #54 we added a
profile_updated
timestamp tocontact_info
andgroups::member
in order to help resolve a situation where client-side caches could incorrectly replace "new" profile data with "old" profile data due to a lack of versioning mechanism being attached to profile data sent along with messages. We need to add an equivalentpublic_profile_updated
value to theUserProfile
config in order to manage the same situation across multiple devices but this can run into a similar issue as thelast_read
value onConvoInfoVolatile
(additionally, since the timestamp is basically a separate versioning mechanism for a number ofUserProfile
fields, having thepublic_profile_updated
automatically merge could result in a mismatch between thepublic_profile_updated
and the associated profile values)This PR adds the
public_profile_updated
to theUserProfile
config, and adds aresolve_conflicts
which can be overwritten by individual config types to do any custom handling in the case of a merge conflict.~~