From 29ae18b3f04a406f21500deb524d972d25bd3910 Mon Sep 17 00:00:00 2001 From: Jim Nelson Date: Wed, 25 Sep 2013 18:04:58 -0700 Subject: [PATCH] In Non-English locales, Geary displays wrong month: Closes #7354 Stricter parsing and application of INTERNALDATE. --- sql/CMakeLists.txt | 1 + sql/version-015.sql | 6 ++ src/CMakeLists.txt | 1 + src/engine/api/geary-logging.vala | 2 +- src/engine/db/db-connection.vala | 4 +- src/engine/db/db-result.vala | 43 +++++++- src/engine/imap-db/imap-db-account.vala | 8 +- src/engine/imap-db/imap-db-database.vala | 67 ++++++++++++- src/engine/imap-db/imap-db-folder.vala | 2 +- src/engine/imap-db/imap-db-message-row.vala | 2 +- .../imap/message/imap-internal-date.vala | 98 ++++++++++++++++--- .../response/imap-fetch-data-decoder.vala | 2 +- src/engine/util/util-time.vala | 30 ++++++ 13 files changed, 238 insertions(+), 28 deletions(-) create mode 100644 sql/version-015.sql create mode 100644 src/engine/util/util-time.vala diff --git a/sql/CMakeLists.txt b/sql/CMakeLists.txt index d33cff87..6f207fec 100644 --- a/sql/CMakeLists.txt +++ b/sql/CMakeLists.txt @@ -14,3 +14,4 @@ install(FILES version-011.sql DESTINATION ${SQL_DEST}) install(FILES version-012.sql DESTINATION ${SQL_DEST}) install(FILES version-013.sql DESTINATION ${SQL_DEST}) install(FILES version-014.sql DESTINATION ${SQL_DEST}) +install(FILES version-015.sql DESTINATION ${SQL_DEST}) diff --git a/sql/version-015.sql b/sql/version-015.sql new file mode 100644 index 00000000..39ef9d78 --- /dev/null +++ b/sql/version-015.sql @@ -0,0 +1,6 @@ +-- +-- Dummy database upgrade to fix the INTERNALDATE of messages that were accidentally stored in +-- localized format. See src/engine/imap-db/imap-db-database.vala in post_upgrade() for the code +-- that runs the upgrade, and http://redmine.yorba.org/issues/7354 for more information. +-- + diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 0f49a3e1..ef7a1612 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -285,6 +285,7 @@ engine/util/util-single-item.vala engine/util/util-stream.vala engine/util/util-string.vala engine/util/util-synchronization.vala +engine/util/util-time.vala engine/util/util-trillian.vala ) diff --git a/src/engine/api/geary-logging.vala b/src/engine/api/geary-logging.vala index c75ccfd9..48e5d761 100644 --- a/src/engine/api/geary-logging.vala +++ b/src/engine/api/geary-logging.vala @@ -144,7 +144,7 @@ private void on_log(string prefix, LogLevelFlags log_levels, string message) { if (stream == null) return; - Time tm = Time.local(time_t()); + GLib.Time tm = GLib.Time.local(time_t()); stream.printf("%s %02d:%02d:%02d %lf %s\n", prefix, tm.hour, tm.minute, tm.second, entry_timer.elapsed(), message); diff --git a/src/engine/db/db-connection.vala b/src/engine/db/db-connection.vala index aac0df98..5a9db9c7 100644 --- a/src/engine/db/db-connection.vala +++ b/src/engine/db/db-connection.vala @@ -170,7 +170,7 @@ public class Geary.Db.Connection : Geary.Db.Context { * in SQLite, however, includes 1 and 0, so an integer may be mistaken as a boolean. */ public bool get_pragma_bool(string name) throws Error { - string response = query("PRAGMA %s".printf(name)).string_at(0); + string response = query("PRAGMA %s".printf(name)).nonnull_string_at(0); switch (response.down()) { case "1": case "yes": @@ -221,7 +221,7 @@ public class Geary.Db.Connection : Geary.Db.Context { * Returns the result of a PRAGMA as a string. See [[http://www.sqlite.org/pragma.html]] */ public string get_pragma_string(string name) throws Error { - return query("PRAGMA %s".printf(name)).string_at(0); + return query("PRAGMA %s".printf(name)).nonnull_string_at(0); } /** diff --git a/src/engine/db/db-result.vala b/src/engine/db/db-result.vala index 81aa9d01..476cca30 100644 --- a/src/engine/db/db-result.vala +++ b/src/engine/db/db-result.vala @@ -126,16 +126,33 @@ public class Geary.Db.Result : Geary.Db.Context { /** * column is zero-based. + * + * Returns a null string if the element is NULL. + * + * @see nonnull_string_at */ - public unowned string string_at(int column) throws DatabaseError { + public unowned string? string_at(int column) throws DatabaseError { verify_at(column); - unowned string s = statement.stmt.column_text(column); - log("string_at(%d) -> %s", column, s); + unowned string? s = statement.stmt.column_text(column); + log("string_at(%d) -> %s", column, (s != null) ? s : "(null)"); return s; } + /** + * column is zero-based. + * + * Returns an empty string if the element is NULL. + * + * @see string_at + */ + public unowned string nonnull_string_at(int column) throws DatabaseError { + unowned string? s = string_at(column); + + return (s != null) ? s : ""; + } + /** * column is zero-based. */ @@ -143,7 +160,7 @@ public class Geary.Db.Result : Geary.Db.Context { // Memory.StringBuffer is not entirely suited for this, as it can result in extra copies // internally ... GrowableBuffer is better for large blocks Memory.GrowableBuffer buffer = new Memory.GrowableBuffer(); - buffer.append(string_at(column).data); + buffer.append(nonnull_string_at(column).data); return buffer; } @@ -231,11 +248,27 @@ public class Geary.Db.Result : Geary.Db.Context { /** * name is the name of the column in the result set. See Statement.get_column_index() for name * matching rules. + * + * Returns a null string if the element is NULL. + * + * @see nonnull_string_for */ - public unowned string string_for(string name) throws DatabaseError { + public unowned string? string_for(string name) throws DatabaseError { return string_at(convert_for(name)); } + /** + * name is the name of the column in the result set. See Statement.get_column_index() for name + * matching rules. + * + * Returns an empty string if the element is NULL. + * + * @see string_for + */ + public unowned string nonnull_string_for(string name) throws DatabaseError { + return nonnull_string_at(convert_for(name)); + } + /** * name is the name of the column in the result set. See Statement.get_column_index() for name * matching rules. diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 3ae67ca1..1cbbd9e9 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -331,7 +331,7 @@ private class Geary.ImapDB.Account : BaseObject { Db.Result result = statement.exec(cancellable); while (!result.finished) { try { - Contact contact = new Contact(result.string_at(0), result.string_at(1), + Contact contact = new Contact(result.nonnull_string_at(0), result.string_at(1), result.int_at(2), result.string_at(3), ContactFlags.deserialize(result.string_at(4))); contacts.add(contact); } catch (Geary.DatabaseError err) { @@ -823,7 +823,7 @@ private class Geary.ImapDB.Account : BaseObject { Db.Result result = stmt.exec(cancellable); while (!result.finished) { // Build a list of search offsets. - string[] offset_array = result.string_at(0).split(" "); + string[] offset_array = result.nonnull_string_at(0).split(" "); Gee.ArrayList all_offsets = new Gee.ArrayList(); int j = 0; while (true) { @@ -837,7 +837,7 @@ private class Geary.ImapDB.Account : BaseObject { // Iterate over the offset list, scrape strings from the database, and push // the results into our return set. foreach(SearchOffset offset in all_offsets) { - string text = result.string_at(offset.column + 1); + string text = result.nonnull_string_at(offset.column + 1); search_matches.add(text[offset.byte_offset : offset.byte_offset + offset.size].down()); } @@ -1222,7 +1222,7 @@ private class Geary.ImapDB.Account : BaseObject { return null; int64 parent_id = result.int64_at(0); - string name = result.string_at(1); + string name = result.nonnull_string_at(1); // Here too, one level of loop detection is better than nothing. if (folder_id == parent_id) { diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index bac99642..10ed3106 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -87,6 +87,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { case 14: post_upgrade_expand_page_size(); break; + + case 15: + post_upgrade_fix_localized_internaldates(); + break; } } @@ -114,7 +118,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { Db.Result select = query("SELECT id, name FROM FolderTable"); while (!select.finished) { int64 id = select.int64_at(0); - string encoded_name = select.string_at(1); + string encoded_name = select.nonnull_string_at(1); try { string canonical_name = Geary.ImapUtf7.imap_utf7_to_utf8(encoded_name); @@ -207,7 +211,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { try { time_t as_time_t = (internaldate != null ? - new Geary.Imap.InternalDate(internaldate).as_time_t : -1); + Geary.Imap.InternalDate.decode(internaldate).as_time_t : -1); Db.Statement update = cx.prepare( "UPDATE MessageTable SET internaldate_time_t=? WHERE id=?"); @@ -301,6 +305,65 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { } } + // Version 15 + private void post_upgrade_fix_localized_internaldates() { + try { + exec_transaction(Db.TransactionType.RW, (cx) => { + Db.Statement stmt = cx.prepare(""" + SELECT id, internaldate, fields + FROM MessageTable + """); + + Gee.HashMap invalid_ids = new Gee.HashMap< + int64?, Geary.Email.Field>(); + + Db.Result results = stmt.exec(); + while (!results.finished) { + string? internaldate = results.string_at(1); + + try { + if (!String.is_empty(internaldate)) + Imap.InternalDate.decode(internaldate); + } catch (Error err) { + int64 invalid_id = results.rowid_at(0); + + debug("Invalid INTERNALDATE \"%s\" found at row %s in %s: %s", + internaldate != null ? internaldate : "(null)", + invalid_id.to_string(), db_file.get_path(), err.message); + invalid_ids.set(invalid_id, (Geary.Email.Field) results.int_at(2)); + } + + results.next(); + } + + // used prepared statement for iterating over list + stmt = cx.prepare(""" + UPDATE MessageTable + SET fields=?, internaldate=?, internaldate_time_t=?, rfc822_size=? + WHERE id=? + """); + stmt.bind_null(1); + stmt.bind_null(2); + stmt.bind_null(3); + + foreach (int64 invalid_id in invalid_ids.keys) { + stmt.bind_int(0, invalid_ids.get(invalid_id).clear(Geary.Email.Field.PROPERTIES)); + stmt.bind_rowid(4, invalid_id); + + stmt.exec(); + + // reuse statment, overwrite invalid_id, fields only + stmt.reset(Db.ResetScope.SAVE_BINDINGS); + } + + return Db.TransactionOutcome.COMMIT; + }); + } catch (Error err) { + debug("Error fixing INTERNALDATES during upgrade to schema 15 for %s: %s", + db_file.get_path(), err.message); + } + } + private void on_prepare_database_connection(Db.Connection cx) throws Error { cx.set_busy_timeout_msec(Db.Connection.RECOMMENDED_BUSY_TIMEOUT_MSEC); cx.set_foreign_keys(true); diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 72d89cf4..caece3a1 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -1854,7 +1854,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { Gee.List list = new Gee.ArrayList(); do { list.add(new ImapDB.Attachment(cx.database.db_file.get_parent(), results.string_at(1), - results.string_at(2), results.int64_at(3), message_id, results.rowid_at(0), + results.nonnull_string_at(2), results.int64_at(3), message_id, results.rowid_at(0), Geary.Attachment.Disposition.from_int(results.int_at(4)))); } while (results.next(cancellable)); diff --git a/src/engine/imap-db/imap-db-message-row.vala b/src/engine/imap-db/imap-db-message-row.vala index 0721cd4a..cb32cb6d 100644 --- a/src/engine/imap-db/imap-db-message-row.vala +++ b/src/engine/imap-db/imap-db-message-row.vala @@ -154,7 +154,7 @@ private class Geary.ImapDB.MessageRow { Imap.InternalDate? constructed = null; try { - constructed = new Imap.InternalDate(internaldate); + constructed = Imap.InternalDate.decode(internaldate); } catch (Error err) { debug("Unable to construct internaldate object from \"%s\": %s", internaldate, err.message); diff --git a/src/engine/imap/message/imap-internal-date.vala b/src/engine/imap/message/imap-internal-date.vala index 8caa1c82..3a1011f6 100644 --- a/src/engine/imap/message/imap-internal-date.vala +++ b/src/engine/imap/message/imap-internal-date.vala @@ -7,26 +7,89 @@ /** * A representations of IMAP's INTERNALDATE field. * + * INTERNALDATE's format is + * + * dd-Mon-yyyy hh:mm:ss +hhmm + * + * Note that Mon is the standard ''English'' three-letter abbreviation. + * * See [[http://tools.ietf.org/html/rfc3501#section-2.3.3]] */ public class Geary.Imap.InternalDate : Geary.MessageData.AbstractMessageData, Geary.Imap.MessageData, Gee.Hashable, Gee.Comparable { + // see get_en_us_mon() for explanation + private const string[] EN_US_MON = { + "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" + }; + + private const string[] EN_US_MON_DOWN = { + "jan", "feb", "mar", "apr", "may", "jun", "jul", "aug", "sep", "oct", "nov", "dec" + }; + public DateTime value { get; private set; } public time_t as_time_t { get; private set; } + public string? original { get; private set; default = null; } - public InternalDate(string internaldate) throws ImapError { - as_time_t = GMime.utils_header_decode_date(internaldate, null); - if (as_time_t == 0) { - throw new ImapError.PARSE_ERROR("Unable to parse \"%s\": not INTERNALDATE format", - internaldate); - } - - value = new DateTime.from_unix_local(as_time_t); + private InternalDate(string original, DateTime datetime) { + this.original = original; + value = datetime; + as_time_t = Time.datetime_to_time_t(datetime); } public InternalDate.from_date_time(DateTime datetime) throws ImapError { value = datetime; + as_time_t = Time.datetime_to_time_t(datetime); + } + + public static InternalDate decode(string internaldate) throws ImapError { + if (String.is_empty(internaldate)) + throw new ImapError.PARSE_ERROR("Invalid INTERNALDATE: empty string"); + + if (internaldate.length > 64) + throw new ImapError.PARSE_ERROR("Invalid INTERNALDATE: too long (%d)", internaldate.length); + + // Alas, GMime.utils_header_decode_date() is too forgiving for our needs, so do it manually + int day, year, hour, min, sec; + char mon[4]; + char tz[6]; + int count = internaldate.scanf("%d-%3s-%d %d:%d:%d %5s", out day, mon, out year, out hour, + out min, out sec, tz); + if (count != 7) + throw new ImapError.PARSE_ERROR("Invalid INTERNALDATE \"%s\": too few fields (%d)", internaldate, count); + + // check numerical ranges; this does not verify this is an actual date, DateTime will do + // that (and round upward, which has to be accepted) + if (!Numeric.int_in_range_inclusive(day, 1, 31) + || !Numeric.int_in_range_inclusive(hour, 0, 23) + || !Numeric.int_in_range_inclusive(min, 0, 59) + || !Numeric.int_in_range_inclusive(sec, 0, 59) + || year < 1970) { + throw new ImapError.PARSE_ERROR("Invalid INTERNALDATE \"%s\": bad numerical range", internaldate); + } + + // check month (this catches localization problems) + int month = -1; + string mon_down = ((string) mon).down(); + for (int ctr = 0; ctr < EN_US_MON_DOWN.length; ctr++) { + if (mon_down == EN_US_MON_DOWN[ctr]) { + month = ctr; + + break; + } + } + + if (month < 0) + throw new ImapError.PARSE_ERROR("Invalid INTERNALDATE \"%s\": bad month", internaldate); + + // TODO: verify timezone + + // assemble into DateTime, which validates the time as well (this is why we want to keep + // original around, for other reasons) ... month is 1-based in DateTime + DateTime datetime = new DateTime(new TimeZone((string) tz), year, month + 1, day, hour, min, + sec); + + return new InternalDate(internaldate, datetime); } /** @@ -51,19 +114,32 @@ public class Geary.Imap.InternalDate : Geary.MessageData.AbstractMessageData, Ge * @see serialize_for_search */ public string serialize() { - return value.format("%d-%b-%Y %H:%M:%S %z"); + return original ?? value.format("%d-%%s-%Y %H:%M:%S %z").printf(get_en_us_mon()); } /** - * Returns the {@link InternalDate}'s string representation for a SEARCH function. + * Returns the {@link InternalDate}'s string representation for a SEARCH command. * * SEARCH does not respect time or timezone, so drop when sending it. See * [[http://tools.ietf.org/html/rfc3501#section-6.4.4]] * * @see serialize + * @see SearchCommand */ public string serialize_for_search() { - return value.format("%d-%b-%Y"); + return value.format("%d-%%s-%Y").printf(get_en_us_mon()); + } + + /** + * Because IMAP's INTERNALDATE strings are ''never'' localized (as best as I can gather), so + * need to use en_US appreviated month names, as that's the only value in INTERNALDATE that is + * in a language and not a numeric value. + */ + private string get_en_us_mon() { + // month is 1-based inside of DateTime + int mon = (value.get_month() - 1).clamp(0, EN_US_MON.length - 1); + + return EN_US_MON[mon]; } public uint hash() { diff --git a/src/engine/imap/response/imap-fetch-data-decoder.vala b/src/engine/imap/response/imap-fetch-data-decoder.vala index 8d9a359b..5dc1a6f6 100644 --- a/src/engine/imap/response/imap-fetch-data-decoder.vala +++ b/src/engine/imap/response/imap-fetch-data-decoder.vala @@ -109,7 +109,7 @@ public class Geary.Imap.InternalDateDecoder : Geary.Imap.FetchDataDecoder { } protected override MessageData decode_string(StringParameter stringp) throws ImapError { - return new InternalDate(stringp.value); + return InternalDate.decode(stringp.value); } } diff --git a/src/engine/util/util-time.vala b/src/engine/util/util-time.vala new file mode 100644 index 00000000..baca261a --- /dev/null +++ b/src/engine/util/util-time.vala @@ -0,0 +1,30 @@ +/* Copyright 2013 Yorba Foundation + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +namespace Geary.Time { + +/** + * Converts a DateTime object into the nearest approximation of time_t. + * + * Since DateTime can store down to the microsecond and dates before UNIX epoch, there's some + * truncating going on here. + */ +public time_t datetime_to_time_t(DateTime datetime) { + GLib.Time tm = GLib.Time(); + tm.second = datetime.get_second(); + tm.minute = datetime.get_minute(); + tm.hour = datetime.get_hour(); + tm.day = datetime.get_day_of_month(); + // month is 1-based in DateTime + tm.month = Numeric.int_floor(datetime.get_month() - 1, 0); + // Time's year is number of years after 1900 + tm.year = Numeric.int_floor(datetime.get_year() - 1900, 1900); + tm.isdst = datetime.is_daylight_savings() ? 1 : 0; + + return tm.mktime(); +} + +}