Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

IMAP Speed Improvements for 1.9.3 branch #193

Closed
wants to merge 4 commits into from

4 participants

@tonyarkles

My initial problem was that I couldn't specify "1:*" for the range to pull all of the messages from the selected mailbox. After I could retrieve them all, I encountered a few bugs parsing the emails in my inbox.

I originally patched the 1.9.3 branch, and have made a separate patch for the trunk branch. This is the 1.9.3 branch, I will make a separate pull request for the trunk.

@tenderlove tenderlove commented on the diff
test/net/imap/test_imap_messageset.rb
((7 lines not shown))
+ @strings = []
+ end
+ def put_string(s)
+ @strings << s
+ end
+ attr_accessor :strings
+end
+
+class IMAPMessageSetTest < Test::Unit::TestCase
+
+ ### Validation Tests
+
+ def assert_messageset_ok_with(set)
+ ms = Net::IMAP::MessageSet.new(set)
+ assert_nothing_raised do
+ ms.validate
@tenderlove Collaborator

Is validate supposed to return something? If so, could we just test that it returns the appropriate thing (rather than testing that it doesn't raise an exception).

I don't think so. It throws an exception if things don't validate, but I think that's about it. There's a bunch of #validate methods on other objects that do nothing at all.

@shugo Collaborator
shugo added a note

You're right. validate is supposed to return nothing and to raise an exception if the receiver is not valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove
Collaborator

This issue should be for @shugo, but I can't seem to assign it to him.

@shugo
Collaborator

Could you file a ticket for trunk at bugs.ruby-lang.org?

@tonyarkles

I can definitely file a ticket for trunk. This is kind of addressing a few issues at once; should I just make a single ticket for all of them, or should I make a separate ticket for each?

@shugo
Collaborator

I prefer a separate ticket for each feature.

@zzak
Collaborator

I'm closing this please refer to the following tickets in redmine: #7145, #7146 and #7147

@zzak zzak closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
35 lib/net/imap.rb
@@ -1519,7 +1519,7 @@ def initialize(data)
def format_internal(data)
case data
- when "*"
+ when String
return data
when Integer
if data == -1
@@ -1541,6 +1541,7 @@ def format_internal(data)
def validate_internal(data)
case data
when "*"
+ when String
when Integer
ensure_nz_number(data)
when Range
@@ -1998,6 +1999,14 @@ def media_subtype
end
end
+ class BodyTypeExtension < Struct.new(:media_type, :subtype,
+ :params, :content_id,
+ :description, :encoding, :size)
+ def multipart?
+ return false
+ end
+ end
+
class ResponseParser # :nodoc:
def initialize
@str = nil
@@ -2360,6 +2369,30 @@ def body_type_msg
mtype, msubtype = media_type
match(T_SPACE)
param, content_id, desc, enc, size = body_fields
+
+ # If this is not message/rfc822, we shouldn't apply the RFC822 spec
+ # to it.
+ # We should handle anything other than message/rfc822 using
+ # multipart extension data [rfc3501] (i.e. the data itself won't be
+ # returned, we would have to retrieve it with BODYSTRUCTURE instead
+ # of with BODY
+ if "#{mtype}/#{msubtype}" != 'MESSAGE/RFC822' then
+ return BodyTypeExtension.new(mtype, msubtype,
+ param, content_id,
+ desc, enc, size)
+ end
+
+ # Also, sometimes a message/rfc822 is included as a large
+ # attachment instead of having all of the other details
+ # (e.g. attaching a .eml file to an email)
+
+ token = lookahead
+ if token.symbol == T_RPAR then
+ return BodyTypeMessage.new(mtype, msubtype, param, content_id,
+ desc, enc, size, nil, nil, nil, nil,
+ nil, nil, nil)
+ end
+
match(T_SPACE)
env = envelope
match(T_SPACE)
View
66 test/net/imap/test_imap_messageset.rb
@@ -0,0 +1,66 @@
+require 'net/imap'
+require 'test/unit'
+
+
+class IMAPTestStub
+ def initialize
+ @strings = []
+ end
+ def put_string(s)
+ @strings << s
+ end
+ attr_accessor :strings
+end
+
+class IMAPMessageSetTest < Test::Unit::TestCase
+
+ ### Validation Tests
+
+ def assert_messageset_ok_with(set)
+ ms = Net::IMAP::MessageSet.new(set)
+ assert_nothing_raised do
+ ms.validate
@tenderlove Collaborator

Is validate supposed to return something? If so, could we just test that it returns the appropriate thing (rather than testing that it doesn't raise an exception).

I don't think so. It throws an exception if things don't validate, but I think that's about it. There's a bunch of #validate methods on other objects that do nothing at all.

@shugo Collaborator
shugo added a note

You're right. validate is supposed to return nothing and to raise an exception if the receiver is not valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ end
+
+ def test_allows_integer
+ assert_messageset_ok_with 1
+ end
+ def test_allows_range
+ assert_messageset_ok_with 1..5
+ end
+ def test_allows_array
+ assert_messageset_ok_with [1,2,3,8]
+ end
+ def test_allows_string_range
+ assert_messageset_ok_with "1:*"
+ end
+
+
+ ### Formatting Tests
+
+ def assert_formats_as(expected, from)
+ ms = Net::IMAP::MessageSet.new(from)
+ fake_imap = IMAPTestStub.new
+ ms.send_data(fake_imap)
+ assert_equal(expected, fake_imap.strings[0])
+ end
+
+ def test_formats_integer
+ assert_formats_as "1", 1
+ end
+ def test_formats_negative_one_as_star
+ assert_formats_as "*", -1
+ end
+ def test_formats_range
+ assert_formats_as '1:5', 1..5
+ end
+
+ def test_formats_array
+ assert_formats_as '1,2,5', [1,2,5]
+ end
+
+ def test_formats_string_range
+ assert_formats_as '1:*', '1:*'
+ end
+end
View
19 test/net/imap/test_imap_response_parser.rb
@@ -116,4 +116,23 @@ def test_msg_att_extra_space
* 1 FETCH (UID 92285 )
EOF
end
+
+ def assert_parseable(s)
+ parser = Net::IMAP::ResponseParser.new
+ parser.parse(s.gsub(/\n/, "\r\n").taint)
+ end
+
+ def test_msg_delivery_status
+ # This was part of a larger response that caused crashes, but this was the
+ # minimal test case to demonstrate it
+ assert_parseable <<EOF
+* 4902 FETCH (BODY (("MESSAGE" "DELIVERY-STATUS" NIL NIL NIL "7BIT" 324) "REPORT"))
+EOF
+ end
+
+ def test_msg_with_message_rfc822_attachment
+ assert_parseable <<EOF
+* 5441 FETCH (BODY ((("TEXT" "PLAIN" ("CHARSET" "iso-8859-1") NIL NIL "QUOTED-PRINTABLE" 69 1)("TEXT" "HTML" ("CHARSET" "iso-8859-1") NIL NIL "QUOTED-PRINTABLE" 455 12) "ALTERNATIVE")("MESSAGE" "RFC822" ("NAME" "ATT00026.eml") NIL NIL "7BIT" 4079755) "MIXED"))
+EOF
+ end
end
Something went wrong with that request. Please try again.