Closed Bug 1160399 Opened 7 years ago Closed 7 years ago

Reduce table abstraction to consolidate data access

Categories

(Firefox for iOS :: Data Storage, defect)

All
iOS 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file)

47 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review
Wes and I have been talking about this a lot; for the sake of the historical record, below is a long email I wrote.

This bug tracks the initial work for hacking on this, and it'll incorporate Bug 1128621. Follow-up work might switch out the DB layer, change its signatures (particularly getting rid of the inout NSError approach and switching to Deferred), and reconcile the various uses we have for Site/Visit/etc.

---

* I want the storage representation and the API to be decoupled, so that we don't need to, say, pass a Site to foo.insert just because foo.query ends up giving us Sites. This is particularly relevant in joins… but almost everything we do with history is joined, and the same is true for anything that uses a favicon.

This coupling (again, particularly in joins) leads to some crazy stuff. For example:

https://github.com/mozilla/firefox-ios/blob/master/Storage/SQL/VisitsTable.swift#L47

VisitsTable needs to fill in all the details of the site, even though it doesn't know them! And we thus can't make a Site's URL an NSURL, because "" isn't a valid URL. But a Site can't not have a URL, so we can't make it optional; nope, this is simply a modeling error that we've lived with until now. The visits table just doesn't know about site URLs!

But changing this is hairy. Do we switch this table to be just Visit (a simple timestamp/type pair)? Do we need a "SiteIDVisit" that has the siteID but not the site URL and title (because only get those from the join)? I started digging into this earlier today, and ran into snags in every direction.

This is the pain point of an ORM: you get an explosion of classes, or an explosion of optional fields, or you have to switch to untyped Dictionaries, whenever you introduce a new join or a new projection. And we've got a few of these, and will get more real soon…


* I want to fix Cursor, so you'll have profile.browser.sitesVisitedSince:(Timestamp) -> Cursor<Site>, not profile.history.query(Options) -> Cursor. This'll make calling and DB code much more pleasant. This is part and parcel of decoupling APIs and storage: the 'shape' of the cursor you get back is a property of the query!


* "Table" starts to break down when the API ("history") isn't a close correspondence with storage. For example, as I'm introducing syncability I need to add the concept of a local overlay on a remote mirror. Most of the rest of the codebase doesn't need to know about this, but as a result callers can't just insert into or query a plain ol' table; the query is really a UNION ALL over a subquery, or a LEFT OUTER JOIN, or something else. As Nick mentioned, this also gets painful in the CP world on Android, but at least there we're given the opportunity to write the SQL.


* I want strongly defined APIs: so not foo.update(Site) but foo.visit(URL, type). At its logical extreme we get stuff like foo.getMostRecentVisitTime(URL) -> Deferred<Result<Timestamp?>>, which is totally clear and immediately consumable — no cursor nonsense for callers to deal with, and much better perf envelope, because the implementation gets to run the best query for the job. Moreover, this gives me a fighting chance of tracking sync metadata for changes — reading list taught us that trying to work backwards from general change to specific intent is hard to do well.


* I think we'll get much more understandable code if you don't need to hop between History, SQLiteHistory, HistoryTable, Site, Visit, VisitTable, etc. to understand how a history item is added. This is particularly important when you consider how Sync is going to work: it needs to get a bunch of bookkeeping just right for every write (and even some reads!), and that's much harder when the definition of a write is spread across multiple source files.
See Also: → 1160400
Attached file Pull req.
Tacking on test re-enablements on the end of the PR as I work on those.
Attachment #8602776 - Flags: review?(nalexander)
Comment on attachment 8602776 [details] [review]
Pull req.

Comments (!) on GH.
Attachment #8602776 - Flags: review?(nalexander) → review+
Blocks: 1163210
https://github.com/mozilla/firefox-ios/commit/6db9f8b2a2b9f5cd7b74a941f836c14318d378f6

Oof.

 52 files changed, 1383 insertions(+), 1798 deletions(-)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.