From 51b44f0f4bcfc97116d76d5a049786b245b8def6 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Sun, 1 Jul 2018 14:47:23 +1000 Subject: [PATCH] Remove ListParameter.parent property so lists can have multiple parents. The ListParameter.parent property only existed to make the deserialier's life easier, but is also why sketchy code was needed for appending search criteria from a ServerSearchEmail replay op to the actual IMAP search command sent to the server. This removes the property altogether, and replaces its only use in Deserialier with a stack, as nature intended. This means lists can be added to more than one other list, and e.g. when a search is executed, the search critera can be used for multiple requests. --- .../imap/parameter/imap-list-parameter.vala | 116 ++++-------------- .../imap/transport/imap-deserializer.vala | 57 +++++---- .../parameter/imap-list-parameter-test.vala | 30 +++++ .../transport/imap-deserializer-test.vala | 110 ++++++++++++++++- test/meson.build | 1 + test/test-engine.vala | 1 + 6 files changed, 196 insertions(+), 119 deletions(-) create mode 100644 test/engine/imap/parameter/imap-list-parameter-test.vala diff --git a/src/engine/imap/parameter/imap-list-parameter.vala b/src/engine/imap/parameter/imap-list-parameter.vala index 133d0d96..ad2ea934 100644 --- a/src/engine/imap/parameter/imap-list-parameter.vala +++ b/src/engine/imap/parameter/imap-list-parameter.vala @@ -25,76 +25,37 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { return list.size; } } - - /** - * Returns null if no parent (top-level list). - * - * In a fully-formed set of {@link Parameter}s, this means this {@link ListParameter} is - * probably a {@link RootParameters}. - */ - public weak ListParameter? parent { get; private set; default = null; } - + private Gee.List list = new Gee.ArrayList(); - + + /** - * Creates an empty ListParameter with no parent. - */ - public ListParameter() { - } - - ~ListParameter() { - // Drop back links because, although it's a weak ref, sometimes ListParameters are temporarily - // made and current Vala doesn't reset weak refs - foreach (Parameter param in list) { - ListParameter? listp = param as ListParameter; - if (listp != null) { - assert(listp.parent == this); - - listp.parent = null; - } - } - } - - /** - * Adds the {@link Parameter} to the end of the {@link ListParameter}. + * Appends a parameter to the end of this list. * - * If the Parameter is itself a ListParameter, it's {@link parent} will be set to this - * ListParameter. - * - * The same {@link Parameter} can't be added more than once to the same {@link ListParameter}. - * There are no other restrictions, however. + * The same {@link Parameter} can't be added more than once to the + * same {@link ListParameter}. There are no other restrictions, + * however. * * @return true if added. */ public bool add(Parameter param) { - // if adding a ListParameter, set its parent - ListParameter? listp = param as ListParameter; - if (listp != null) { - if (listp.parent != null) - listp.parent.list.remove(listp); - - listp.parent = this; - } - - return list.add(param); + return this.list.add(param); } - + /** - * Adds all the {@link Parameter}s to the end of the {@link ListParameter}. + * Adds all parameters in the given collection to this list. * - * If any Parameter is itself a ListParameter, it's {@link parent} will be set to this - * ListParameter. - * - * The same {@link Parameter} can't be added more than once to the same {@link ListParameter}. - * There are no other restrictions, however. + * The same {@link Parameter} can't be added more than once to the + * same {@link ListParameter}. There are no other restrictions, + * however. * * @return number of Parameters added. */ public int add_all(Gee.Collection params) { int count = 0; - foreach (Parameter param in params) + foreach (Parameter param in params) { count += add(param) ? 1 : 0; - + } return count; } @@ -118,23 +79,14 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { public int extend(ListParameter listp) { return add_all(listp.list); } - + /** * Clears the {@link ListParameter} of all its children. - * - * This also clears (sets to null) the parents of all ListParameter's children. */ public void clear() { - // sever ties to ListParameter children - foreach (Parameter param in list) { - ListParameter? listp = param as ListParameter; - if (listp != null) - listp.parent = null; - } - list.clear(); } - + // // Parameter retrieval // @@ -370,19 +322,16 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { public ListParameter? get_as_nullable_list(int index) throws ImapError { return (ListParameter?) get_as_nullable(index, typeof(ListParameter)); } - + /** - * Returns [@link ListParameter} at index, an empty list if NIL. - * - * If an empty ListParameter has to be manufactured in place of a NIL parameter, its parent - * will be null. + * Returns {@link ListParameter} at index, an empty list if NIL. */ public ListParameter get_as_empty_list(int index) throws ImapError { ListParameter? param = get_as_nullable_list(index); - + return param ?? new ListParameter(); } - + /** * Returns a {@link ListParameter} at index, null if not a list. * @@ -474,27 +423,12 @@ public class Geary.Imap.ListParameter : Geary.Imap.Parameter { public Parameter replace(int index, Parameter parameter) throws ImapError { if (list.size <= index) throw new ImapError.TYPE_ERROR("No parameter at index %d", index); - - Parameter old = list[index]; - list[index] = parameter; - - // add parent to new Parameter if a list - ListParameter? listp = parameter as ListParameter; - if (listp != null) { - if (listp.parent != null) - listp.parent.list.remove(listp); - - listp.parent = this; - } - - // clear parent of old Parameter if a list - listp = old as ListParameter; - if (listp != null) - listp.parent = null; - + + Parameter old = this.list[index]; + this.list[index] = parameter; return old; } - + /** * Moves all child parameters from the supplied list into this list, clearing this list first. * diff --git a/src/engine/imap/transport/imap-deserializer.vala b/src/engine/imap/transport/imap-deserializer.vala index f6f107de..13c7ecbe 100644 --- a/src/engine/imap/transport/imap-deserializer.vala +++ b/src/engine/imap/transport/imap-deserializer.vala @@ -74,11 +74,14 @@ public class Geary.Imap.Deserializer : BaseObject { private string identifier; private DataInputStream dins; private Geary.State.Machine fsm; + private ListParameter context; + private Gee.LinkedList context_stack = + new Gee.LinkedList(); + private Cancellable? cancellable = null; private Nonblocking.Semaphore closed_semaphore = new Nonblocking.Semaphore(); private Geary.Stream.MidstreamConverter midstream = new Geary.Stream.MidstreamConverter("Deserializer"); - private RootParameters root = new RootParameters(); private StringBuilder? current_string = null; private size_t literal_length_remaining = 0; private Geary.Memory.GrowableBuffer? block_buffer = null; @@ -144,9 +147,7 @@ public class Geary.Imap.Deserializer : BaseObject { dins = new DataInputStream(cins); dins.set_newline_type(DataStreamNewlineType.CR_LF); dins.set_close_base_stream(false); - - context = root; - + Geary.State.Mapping[] mappings = { new Geary.State.Mapping(State.TAG, Event.CHAR, on_tag_char), new Geary.State.Mapping(State.TAG, Event.EOS, on_eos), @@ -203,10 +204,12 @@ public class Geary.Imap.Deserializer : BaseObject { new Geary.State.Mapping(State.CLOSED, Event.EOS, Geary.State.nop), new Geary.State.Mapping(State.CLOSED, Event.ERROR, Geary.State.nop) }; - - fsm = new Geary.State.Machine(machine_desc, mappings, on_bad_transition); + + this.fsm = new Geary.State.Machine(machine_desc, mappings, on_bad_transition); + + reset_params(); } - + /** * Install a custom Converter into the input stream. * @@ -456,34 +459,34 @@ public class Geary.Imap.Deserializer : BaseObject { private void save_parameter(Parameter param) { context.add(param); } - + // ListParameter's parent *must* be current context private void push(ListParameter child) { - context.add(child); - - context = child; + this.context.add(child); + + this.context_stack.insert(0, child); + this.context = child; } - + private char get_current_context_terminator() { return (context is ResponseCode) ? ']' : ')'; } - + private State pop() { - ListParameter? parent = context.parent; - if (parent == null) { + State ret = State.START_PARAM; + if (this.context_stack.size > 1) { + this.context_stack.remove_at(0); + this.context = this.context_stack[0]; + } else { warning("Attempt to close unopened list/response code"); - - return State.FAILED; + ret = State.FAILED; } - - context = parent; - - return State.START_PARAM; + return ret; } private void flush_params() { bool okay = true; - if (this.context != this.root) { + if (this.context_stack.size > 1) { Logging.debug( Logging.Flag.DESERIALIZER, "[%s] Unclosed list in parameters", @@ -501,17 +504,17 @@ public class Geary.Imap.Deserializer : BaseObject { okay = false; } - if (okay && this.root != null && root.size > 0) { - parameters_ready(this.root); + if (okay && this.context.size > 0) { + parameters_ready((RootParameters) this.context); } reset_params(); } private void reset_params() { - this.context = this.root = new RootParameters(); - this.current_string = null; - this.literal_length_remaining = 0; + this.context = new RootParameters(); + this.context_stack.clear(); + this.context_stack.add(this.context); } // diff --git a/test/engine/imap/parameter/imap-list-parameter-test.vala b/test/engine/imap/parameter/imap-list-parameter-test.vala new file mode 100644 index 00000000..6f179787 --- /dev/null +++ b/test/engine/imap/parameter/imap-list-parameter-test.vala @@ -0,0 +1,30 @@ +/* + * Copyright 2018 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +class Geary.Imap.ListParameterTest : TestCase { + + + public ListParameterTest() { + base("Geary.Imap.ListParameterTest"); + add_test("add_to_multiple_parents", add_to_multiple_parents); + } + + // See GitLab Issue #26 + public void add_to_multiple_parents() throws GLib.Error { + ListParameter child = new ListParameter(); + + ListParameter parent_1 = new ListParameter(); + ListParameter parent_2 = new ListParameter(); + + parent_1.add(child); + parent_2.add(child); + + assert_int(1, parent_1.size, "Parent 1 does not contain child"); + assert_int(1, parent_2.size, "Parent 2 does not contain child"); + } + +} diff --git a/test/engine/imap/transport/imap-deserializer-test.vala b/test/engine/imap/transport/imap-deserializer-test.vala index 2ddeef24..0960331d 100644 --- a/test/engine/imap/transport/imap-deserializer-test.vala +++ b/test/engine/imap/transport/imap-deserializer-test.vala @@ -1,5 +1,5 @@ /* - * Copyright 2017 Michael Gratton + * Copyright 2017-2018 Michael Gratton * * This software is licensed under the GNU Lesser General Public License * (version 2.1 or later). See the COPYING file in this distribution. @@ -11,6 +11,7 @@ class Geary.Imap.DeserializerTest : TestCase { protected enum Expect { MESSAGE, EOS, DESER_FAIL; } private const string ID = "test"; + private const string UNTAGGED = "* "; private const string EOL = "\r\n"; private Deserializer? deser = null; @@ -19,6 +20,14 @@ class Geary.Imap.DeserializerTest : TestCase { public DeserializerTest() { base("Geary.Imap.DeserializerTest"); + add_test("parse_unquoted", parse_unquoted); + add_test("parse_quoted", parse_quoted); + add_test("parse_number", parse_number); + add_test("parse_list", parse_list); + add_test("parse_response_code", parse_response_code); + add_test("parse_bad_list", parse_bad_list); + add_test("parse_bad_code", parse_bad_response_code); + add_test("gmail_greeting", gmail_greeting); add_test("cyrus_2_4_greeting", cyrus_2_4_greeting); add_test("aliyun_greeting", aliyun_greeting); @@ -44,6 +53,105 @@ class Geary.Imap.DeserializerTest : TestCase { public override void tear_down() { this.deser.stop_async.begin((obj, ret) => { async_complete(ret); }); async_result(); + this.stream = null; + } + + public void parse_unquoted() throws Error { + string bytes = "OK"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); }); + RootParameters? message = this.process.end(async_result()); + + assert_int(2, message.size); + assert_true(message.get(1) is UnquotedStringParameter, "Not parsed as atom"); + assert_string(bytes, message.get(1).to_string()); + } + + public void parse_quoted() throws Error { + string bytes = "\"OK\""; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); }); + RootParameters? message = this.process.end(async_result()); + + assert_int(2, message.size); + assert_true(message.get(1) is QuotedStringParameter, "Not parsed as quoted"); + assert_string(bytes, message.get(1).to_string()); + } + + public void parse_number() throws Error { + string bytes = "1234"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); }); + RootParameters? message = this.process.end(async_result()); + + assert_int(2, message.size); + assert_true(message.get(1) is NumberParameter, "Not parsed as number"); + assert_string(bytes, message.get(1).to_string()); + } + + public void parse_list() throws Error { + string bytes = "(OK)"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); }); + RootParameters? message = this.process.end(async_result()); + + assert_int(2, message.size); + assert_true(message.get(1) is ListParameter, "Not parsed as list"); + assert_string(bytes, message.get(1).to_string()); + } + + public void parse_response_code() throws Error { + string bytes = "[OK]"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); }); + RootParameters? message = this.process.end(async_result()); + + assert_int(2, message.size); + assert_true(message.get(1) is ResponseCode, "Not parsed as response code"); + assert_string(bytes, message.get(1).to_string()); + } + + public void parse_bad_list() throws Error { + string bytes = "(UHH"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + // XXX We expect EOS here rather than DESER_FAIL since the + // deserializer currently silently ignores lines with + // malformed lists and continues parsing, so we get to the end + // of the stream. + this.process.begin(Expect.EOS, (obj, ret) => { async_complete(ret); }); + this.process.end(async_result()); + } + + public void parse_bad_response_code() throws Error { + string bytes = "[UHH"; + this.stream.add_data(UNTAGGED.data); + this.stream.add_data(bytes.data); + this.stream.add_data(EOL.data); + + // XXX We expect EOS here rather than DESER_FAIL since the + // deserializer currently silently ignores lines with + // malformed lists and continues parsing, so we get to the end + // of the stream. + this.process.begin(Expect.EOS, (obj, ret) => { async_complete(ret); }); + this.process.end(async_result()); } public void gmail_greeting() throws Error { diff --git a/test/meson.build b/test/meson.build index 0dd628d7..80928653 100644 --- a/test/meson.build +++ b/test/meson.build @@ -32,6 +32,7 @@ geary_test_engine_sources = [ 'engine/imap/command/imap-create-command-test.vala', 'engine/imap/message/imap-data-format-test.vala', 'engine/imap/message/imap-mailbox-specifier-test.vala', + 'engine/imap/parameter/imap-list-parameter-test.vala', 'engine/imap/response/imap-namespace-response-test.vala', 'engine/imap/transport/imap-deserializer-test.vala', 'engine/imap-db/imap-db-attachment-test.vala', diff --git a/test/test-engine.vala b/test/test-engine.vala index c6cfc3ca..e0bfec84 100644 --- a/test/test-engine.vala +++ b/test/test-engine.vala @@ -42,6 +42,7 @@ int main(string[] args) { engine.add_suite(new Geary.Imap.DeserializerTest().get_suite()); engine.add_suite(new Geary.Imap.MailboxSpecifierTest().get_suite()); engine.add_suite(new Geary.Imap.NamespaceResponseTest().get_suite()); + engine.add_suite(new Geary.Imap.ListParameterTest().get_suite()); engine.add_suite(new Geary.ImapDB.AttachmentTest().get_suite()); engine.add_suite(new Geary.ImapDB.AttachmentIoTest().get_suite()); engine.add_suite(new Geary.ImapDB.DatabaseTest().get_suite());