Make sure queries by name to Folder/Label/Category account for truncation

nylas / sync-engine

:incoming_envelope: IMAP/SMTP sync system with modern APIs

https://nylas.com/docs/platform

GNU Affero General Public License v3.0

3.5k stars 354 forks source link

Make sure queries by name to Folder/Label/Category account for truncation #358

Closed jstejada closed 8 years ago

jstejada commented 8 years ago

Cleans up Folder/Label/Category name sanitization code which was mostly duplicated
Added a custom string type for Category names to make sure we always store properly sanitized strings, and extends __eq__ comparison to compare against the truncated string, which is what we actually store in the database, even if the user queries using a longer (original) name. Going forward, any queries by name will now compare against the correct string.
This only affects querying-- we still use the full folder name to be able to select the folder correctly in IMAP inside the sync engine
Improves logging -- don't interpolate strings in the logs!
This addresses most occurrences of https://sentry.nylas.com/sentry/sync-prod/group/8243/events/

spang commented 8 years ago

This diff LGTM for the most part—thanks for cleaning things up! I left a few minor comments inline, and I would also love to see a unit test for the custom comparator type.

jstejada commented 8 years ago

@spang, @bengotow, addressed your comments and made the custom string type more generic as per discussion with ben -- it might be worth reusing for stuff like filename truncation. Also added some unit tests. Could you take a look at this latest diff? It seems that the bot didn't catch spang's lgtm.

bengotow commented 8 years ago

This is really nice. Thanks for cleaning up the duplicate logic! I think in the future we should definitely consider using StringWithTransform for filenames and other things. Doing a quick grep for [: it looks like there are several other places where we're guarding against string length that might be better placed into ORM-level transforms 👍

Ship it! LGTM

spang commented 8 years ago

I like it! looking forward to more changes that make the codebase easier to understand and work with and it harder for us to introduce bugs :) :+1: