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());