[SQLite] add ThreadStore Emscripten bindings and converters
ClosedPublic
Actions

Authored by kamil on Nov 22 2023, 6:51 AM.

Details

Reviewers

tomek
michal

Commits

rCOMM039bb3f714fa: [SQLite] add ThreadStore Emscripten bindings and converters

Summary

Unfortunately this code needs to be written because od Emscripten limitations.

Good news is that only two stores needs it, Threads (this one, which is the biggest and most complicated) and Messages which are simple. This is a lot of code, but it's really easy, just converts null -> NullableString on JS and std::unique_ptr -> NullableString in C++.

Depends on D9951, D9902

Test Plan

Tests

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 22 2023, 6:51 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptNov 22 2023, 6:51 AM

kamil added inline comments.Nov 22 2023, 7:00 AM

web/database/types/entities.js
65 ↗	(On Diff #33487)	it's defined as nullable but it's required in C++ src. (have default value which means will always be defined anyway
76 ↗	(On Diff #33487)	avatar is defined `avatar?: ?string`, but this field is always passed to DB src - so we can assume that it's more like `avatar: ?string` what is more, at DB level we can't distinguish those two
93 ↗	(On Diff #33487)	the difference is caused by the fact that `sourceMessageID` is defined like `sourceMessageID?: string`, not `sourceMessageID: ?string`

Harbormaster failed remote builds in B24331: Diff 33487!Nov 22 2023, 7:19 AM

kamil published this revision for review.Nov 22 2023, 7:24 AM

kamil added a child revision: D9954: [web] processing ThreadStore ops on worker.Nov 22 2023, 8:20 AM

Why not just use NullableString for native too? What's the benefit of this "forking" between web/native?

tomek added inline comments.Nov 23 2023, 2:49 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
88–94 ↗	(On Diff #33487)	How about inverting the condition?
native/cpp/CommonCpp/DatabaseManagers/entities/Thread.h
47 ↗	(On Diff #33487)	It might be more convenient to e.g. make this a constructor, or a class method that returns an instance of `WebThread`.

In D9952#290478, @ashoat wrote:

Why not just use NullableString for native too? What's the benefit of this "forking" between web/native?

There is no benefit, it was forced by technical limitations.

Why not NullableString on native:

Our schema forces pointers, when you query data and there is no data in the column it returns null.
Entities are used to deduce sqlite_orm schema, so when the column is nullable we have to use smart pointers.
Any changes there are complicated, and what is more will require migrating existing tables on native.

Why not smart_ptr on web:

Described here and here.

This is some kind of workaround to allow me to proceed with this work, but when follow-up ENG-5719 is resolved we can remove the "forked" code added here.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
88–94 ↗	(On Diff #33487)	makes sense
native/cpp/CommonCpp/DatabaseManagers/entities/Thread.h
47 ↗	(On Diff #33487)	class method that returns an instance of `WebThread`. that makes sense

Thanks for explaining. I guess there's nothing better we can do...

I've always been a bit skeptical of sqlite-orm. I think if we didn't use that we could probably replace Thread with WebThread.

In D9952#290605, @kamil wrote:

This is some kind of workaround to allow me to proceed with this work, but when follow-up ENG-5719 is resolved we can remove the "forked" code added here.

Do you foresee that task getting resolved anytime soon? It seems like you're kind of stuck there... is there a possibility that there is no alternative here?

tomek accepted this revision.Nov 24 2023, 1:10 AM

This revision is now accepted and ready to land.Nov 24 2023, 1:10 AM

address review

In D9952#290639, @ashoat wrote:

Thanks for explaining. I guess there's nothing better we can do...

I've always been a bit skeptical of sqlite-orm. I think if we didn't use that we could probably replace Thread with WebThread.

Yes, sqlite_orm forced us to add this diff (and one more for Message entity) but on the other hand there is some amount of code that we don't have to write because sqlite_orm handles this for us, so overallI think we saved some time, but we need to "fork" this code a bit for Message and Thread.

In D9952#290605, @kamil wrote:

This is some kind of workaround to allow me to proceed with this work, but when follow-up ENG-5719 is resolved we can remove the "forked" code added here.

Do you foresee that task getting resolved anytime soon? It seems like you're kind of stuck there... is there a possibility that there is no alternative here?

I want to, at some point, I think I was close but it's really time-consuming to dive into all those C++ template specializations. When I will have any spare cycles first I'll look on ENG-4679 and then get back to ENG-5719.
And yes, unfortunately, there is some risk there is no alternative here, but I believe it can be done.

Harbormaster completed remote builds in B24499: Diff 33704.Nov 27 2023, 5:45 AM

Closed by commit rCOMM039bb3f714fa: [SQLite] add ThreadStore Emscripten bindings and converters (authored by kamil). · Explain WhyNov 27 2023, 6:47 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM039bb3f714fa: [SQLite] add ThreadStore Emscripten bindings and converters.