Page MenuHomePhabricator
Differential D14561

[native][web] Add migrations unshimming delete messages
ClosedPublic
Actions

Authored by tomek on Wed, Apr 9, 7:57 AM.
Tags
None
Referenced Files
F6082985: D14561.diff
Sun, Apr 20, 10:44 PM
F6080100: D14561.id.diff
Sun, Apr 20, 7:00 PM
F6064244: D14561.id47803.diff
Sun, Apr 20, 11:12 AM
Unknown Object (File)
Sun, Apr 20, 1:56 AM
Unknown Object (File)
Fri, Apr 18, 9:48 PM
Unknown Object (File)
Fri, Apr 18, 10:45 AM
Unknown Object (File)
Fri, Apr 18, 4:06 AM
Unknown Object (File)
Fri, Apr 18, 4:06 AM
View All 44 Files
Subscribers
bartek

Details

Reviewers
kamil
ashoat
bartek
Commits
rCOMMc4ddbd5076cc: [native][web] Add migrations unshimming delete messages
Summary

This migration handles received messages that were unsupported.

https://linear.app/comm/issue/ENG-10465/unshim-delete-message

Depends on D14560

Test Plan

Run the app in an older version, receive a delete message, run an updated app - check if the unsupported messages disappear and deleted messages are replaced.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek created this revision.Wed, Apr 9, 7:57 AM
Harbormaster completed remote builds in B33931: Diff 47713.Wed, Apr 9, 8:15 AM
tomek requested review of this revision.Wed, Apr 9, 8:15 AM
tomek edited the summary of this revision. (Show Details)Wed, Apr 9, 8:26 AM
tomek edited the test plan for this revision. (Show Details)
tomek updated this revision to Diff 47718.Wed, Apr 9, 8:29 AM

Rebase

Harbormaster completed remote builds in B33936: Diff 47718.Wed, Apr 9, 8:46 AM
ashoat accepted this revision.Wed, Apr 9, 7:56 PM
ashoat added inline comments.
web/redux/persist.js
822–826 ↗(On Diff #47718)

Why don't we need handleReduxMigrationFailure here?

This revision is now accepted and ready to land.Wed, Apr 9, 7:56 PM
tomek updated this revision to Diff 47738.Thu, Apr 10, 5:50 AM

Handle migration failure

tomek added a subscriber: bartek.Thu, Apr 10, 5:50 AM
tomek added inline comments.
web/redux/persist.js
822–826 ↗(On Diff #47718)

It's an interesting question. This util was initially introduced in https://phab.comm.dev/D11885 where handleReduxMigrationFailure wasn't present as a parameter, but it was used internally when handling exceptions. Then in https://phab.comm.dev/D13889 handleReduxMigrationFailure was replaced by rethrowing an exception. I don't understand why we removed this call, but I believe it should be there. @bartek do you have some context?

tomek added a child revision: D14564: [lib] Handle deleted messages when generating notifs.Thu, Apr 10, 6:04 AM
Harbormaster completed remote builds in B33954: Diff 47738.Thu, Apr 10, 6:07 AM
ashoat added a reviewer: bartek.Thu, Apr 10, 6:41 AM
This revision now requires review to proceed.Thu, Apr 10, 6:41 AM
bartek accepted this revision.Fri, Apr 11, 12:20 AM
bartek added inline comments.
web/redux/persist.js
822–826 ↗(On Diff #47718)
  • In D13889, original unshimClientDB was renamed to legacyUnshimClientDB, and new unshimClientDB was introduced. The former was used for legacy migrations.
  • There is some context in D11928 as to why handleMigrationFailure was introduced as a parameter to the unshim func. But I guess it's only for native, and web has never had this param.

Then in https://phab.comm.dev/D13889 handleReduxMigrationFailure was replaced by rethrowing an exception.

In https://phab.comm.dev/D13889?id=45703 @ashoat did these changes to the unshim func after I failed to do the migration (see the diff history). I didn't have much knowledge about unshimming and migrations and I didn't argue with these changes. Unfortunately, I have no more context beyond that.
Just note that migration I introduced there was native-only and it was no-op for web, so the unshim func wasn't really tested.

Anyway, since the unshim func on web doesn't take the handleMigrationFailure param, this diff is okay and any fixes/adjustment to the unshimming should probably be done separately.

This revision is now accepted and ready to land.Fri, Apr 11, 12:20 AM
ashoat requested changes to this revision.EditedFri, Apr 11, 8:06 AM

A cursory glance shows that createAsyncMigrate handles calling handleReduxMigrationFailure when an exception is thrown. I wish both @tomek and @bartek had looked at the code a little closer here.

This revision now requires changes to proceed.Fri, Apr 11, 8:06 AM
tomek updated this revision to Diff 47770.Mon, Apr 14, 5:03 AM

Revert error handling change

Harbormaster completed remote builds in B33979: Diff 47770.Mon, Apr 14, 5:20 AM
ashoat accepted this revision.Mon, Apr 14, 6:41 AM
ashoat added inline comments.
native/redux/persist.js
1590 ↗(On Diff #47770)

It looks like native's unshimClientDB is different since it doesn't throw an exception. It would probably be safe to change it to match web given that both use createAsyncMigrate, but I suppose that's outside the scope of this diff, and probably not the best thing to prioritize right now

This revision is now accepted and ready to land.Mon, Apr 14, 6:41 AM
tomek added inline comments.Mon, Apr 14, 6:47 AM
native/redux/persist.js
1590 ↗(On Diff #47770)

Created a general task https://linear.app/comm/issue/ENG-10552/review-client-migrations-error-handling to track reviewing our error handling

Closed by commit rCOMMc4ddbd5076cc: [native][web] Add migrations unshimming delete messages (authored by tomek). · Explain WhyTue, Apr 15, 2:11 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMc4ddbd5076cc: [native][web] Add migrations unshimming delete messages.

Revision Contents
Changeset List

PathSize
keyserver/
src/
fetchers/
6 lines
lib/
shared/
messages/
6 lines
native/
redux/
2 lines
6 lines
web/
redux/
2 lines
7 lines

Diff 47802

View Options

keyserver/src/fetchers/thread-fetchers.js

Show First 20 Lines • Show All 2,327 Lines • ▼ Show 20 Lines OlmPersistSession persistSession = {
it->first, it->first,
std::string(it->second.buffer.begin(), it->second.buffer.end()), std::string(it->second.buffer.begin(), it->second.buffer.end()),
it->second.version}; it->second.version};
this->storeOlmPersistSession(persistSession); this->storeOlmPersistSession(persistSession);
} }
} }
void SQLiteQueryExecutor::setNotifyToken(std::string token) const {
this->setMetadata("notify_token", token);
}
void SQLiteQueryExecutor::clearNotifyToken() const {
this->clearMetadata("notify_token");
}
void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const { void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const {
this->setMetadata("current_user_id", userID); this->setMetadata("current_user_id", userID);
} }
std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const { std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const {
return this->getMetadata("current_user_id"); return this->getMetadata("current_user_id");
} }
▲ Show 20 Lines • Show All 919 Lines • Show Last 20 Lines
View Options

lib/shared/messages/delete-message-spec.js

Show First 20 Lines • Show All 2,327 Lines • ▼ Show 20 Lines OlmPersistSession persistSession = {
it->first, it->first,
std::string(it->second.buffer.begin(), it->second.buffer.end()), std::string(it->second.buffer.begin(), it->second.buffer.end()),
it->second.version}; it->second.version};
this->storeOlmPersistSession(persistSession); this->storeOlmPersistSession(persistSession);
} }
} }
void SQLiteQueryExecutor::setNotifyToken(std::string token) const {
this->setMetadata("notify_token", token);
}
void SQLiteQueryExecutor::clearNotifyToken() const {
this->clearMetadata("notify_token");
}
void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const { void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const {
this->setMetadata("current_user_id", userID); this->setMetadata("current_user_id", userID);
} }
std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const { std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const {
return this->getMetadata("current_user_id"); return this->getMetadata("current_user_id");
} }
▲ Show 20 Lines • Show All 919 Lines • Show Last 20 Lines
View Options

native/redux/persist-constants.js

Show First 20 Lines • Show All 2,327 Lines • ▼ Show 20 Lines OlmPersistSession persistSession = {
it->first, it->first,
std::string(it->second.buffer.begin(), it->second.buffer.end()), std::string(it->second.buffer.begin(), it->second.buffer.end()),
it->second.version}; it->second.version};
this->storeOlmPersistSession(persistSession); this->storeOlmPersistSession(persistSession);
} }
} }
void SQLiteQueryExecutor::setNotifyToken(std::string token) const {
this->setMetadata("notify_token", token);
}
void SQLiteQueryExecutor::clearNotifyToken() const {
this->clearMetadata("notify_token");
}
void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const { void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const {
this->setMetadata("current_user_id", userID); this->setMetadata("current_user_id", userID);
} }
std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const { std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const {
return this->getMetadata("current_user_id"); return this->getMetadata("current_user_id");
} }
▲ Show 20 Lines • Show All 919 Lines • Show Last 20 Lines
View Options

native/redux/persist.js

Show First 20 Lines • Show All 2,327 Lines • ▼ Show 20 Lines OlmPersistSession persistSession = {
it->first, it->first,
std::string(it->second.buffer.begin(), it->second.buffer.end()), std::string(it->second.buffer.begin(), it->second.buffer.end()),
it->second.version}; it->second.version};
this->storeOlmPersistSession(persistSession); this->storeOlmPersistSession(persistSession);
} }
} }
void SQLiteQueryExecutor::setNotifyToken(std::string token) const {
this->setMetadata("notify_token", token);
}
void SQLiteQueryExecutor::clearNotifyToken() const {
this->clearMetadata("notify_token");
}
void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const { void SQLiteQueryExecutor::stampSQLiteDBUserID(std::string userID) const {
this->setMetadata("current_user_id", userID); this->setMetadata("current_user_id", userID);
} }
std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const { std::string SQLiteQueryExecutor::getSQLiteStampedUserID() const {
return this->getMetadata("current_user_id"); return this->getMetadata("current_user_id");
} }
▲ Show 20 Lines • Show All 919 Lines • Show Last 20 Lines
View Options

web/redux/persist-constants.js

Loading...
View Options

web/redux/persist.js

Loading...