Testing this will be tricky and require writable access to an existing IMAP server via TEST_IMAP_WRITE_URL env. I don't know if I want to do a writable IMAP server for testing (or maybe accessible via lei somehow). 4/6 is fix for a long-standing bug carried over from Watch.pm (well, as long as -watch has had IMAP support) which was within the past year, but it's been a long year :<) Eric Wong (6): t/lei-externals: favor "-o format:$PATHNAME" over "-f" lei_to_mail: get rid of empty _post_augment_maildir tests: require Mail::IMAPClient for IMAP tests net_reader: handle single-message IMAP mailboxes net_writer: start implementing IMAP write support URIimap: overload "" to ->as_string MANIFEST | 2 ++ lib/PublicInbox/LeiToMail.pm | 14 +++++------ lib/PublicInbox/NetReader.pm | 47 +++++++++++++++++++++-------------- lib/PublicInbox/NetWriter.pm | 26 +++++++++++++++++++ lib/PublicInbox/URIimap.pm | 1 + t/lei-convert.t | 2 +- t/lei-externals.t | 8 +++--- t/lei-import-imap.t | 2 +- t/net_reader-imap.t | 2 +- t/uri_imap.t | 1 + xt/lei-auth-fail.t | 1 + xt/net_writer-imap.t | 48 ++++++++++++++++++++++++++++++++++++ 12 files changed, 121 insertions(+), 33 deletions(-) create mode 100644 lib/PublicInbox/NetWriter.pm create mode 100644 xt/net_writer-imap.t
It'll be less ambiguous for inputs with "lei convert" and "lei import" cf. https://public-inbox.org/meta/20210217044032.GA17934@dcvr/ --- lib/PublicInbox/LeiToMail.pm | 2 +- t/lei-externals.t | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm index 8a2d9471..b90756ae 100644 --- a/lib/PublicInbox/LeiToMail.pm +++ b/lib/PublicInbox/LeiToMail.pm @@ -358,7 +358,7 @@ sub new { require PublicInbox::MboxReader if $lei->{opt}->{augment}; (-d $dst || (-e _ && !-w _)) and die "$dst exists and is not a writable file\n"; - $self->can("eml2$fmt") or die "bad mbox --format=$fmt\n"; + $self->can("eml2$fmt") or die "bad mbox format: $fmt\n"; $self->{base_type} = 'mbox'; } else { die "bad mail --format=$fmt\n"; diff --git a/t/lei-externals.t b/t/lei-externals.t index f61b7e52..edfbb2bf 100644 --- a/t/lei-externals.t +++ b/t/lei-externals.t @@ -117,18 +117,18 @@ test_lei(sub { unlike($lei_out, qr!https://example\.com/ibx/!s, 'removed canonical URL'); SKIP: { - ok(!$lei->(qw(q s:prefix -o /dev/null -f maildir)), 'bad maildir'); + ok(!lei(qw(q s:prefix -o maildir:/dev/null)), 'bad maildir'); like($lei_err, qr!/dev/null exists and is not a directory!, 'error shown'); is($? >> 8, 1, 'errored out with exit 1'); - ok(!$lei->(qw(q s:prefix -f mboxcl2 -o), $home), 'bad mbox'); + ok(!lei(qw(q s:prefix -o), "mboxcl2:$home"), 'bad mbox'); like($lei_err, qr!\Q$home\E exists and is not a writable file!, 'error shown'); is($? >> 8, 1, 'errored out with exit 1'); - ok(!$lei->(qw(q s:prefix -o /dev/stdout -f Mbox2)), 'bad format'); - like($lei_err, qr/bad mbox --format=mbox2/, 'error shown'); + ok(!lei(qw(q s:prefix -o Mbox2:/dev/stdout)), 'bad format'); + like($lei_err, qr/bad mbox format: mbox2/, 'error shown'); is($? >> 8, 1, 'errored out with exit 1'); # note, on a Bourne shell users should be able to use either:
We won't have _post_augment_imap when we add IMAP support, either. _pre_augment_imap will not exist, either, since opening an IMAP(S) connection can be time consuming so we'll roll that into imap_common_init. --- lib/PublicInbox/LeiToMail.pm | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm index b90756ae..e89cca71 100644 --- a/lib/PublicInbox/LeiToMail.pm +++ b/lib/PublicInbox/LeiToMail.pm @@ -394,8 +394,6 @@ sub _do_augment_maildir { } } -sub _post_augment_maildir {} # noop - sub _pre_augment_mbox { my ($self, $lei) = @_; my $dst = $lei->{ovv}->{dst}; @@ -441,8 +439,8 @@ sub _do_augment_mbox { sub pre_augment { # fast (1 disk seek), runs in same process as post_augment my ($self, $lei) = @_; # _pre_augment_maildir, _pre_augment_mbox - my $m = "_pre_augment_$self->{base_type}"; - $self->$m($lei); + my $m = $self->can("_pre_augment_$self->{base_type}") or return; + $m->($self, $lei); } sub do_augment { # slow, runs in wq worker @@ -455,9 +453,9 @@ sub do_augment { # slow, runs in wq worker # fast (spawn compressor or mkdir), runs in same process as pre_augment sub post_augment { my ($self, $lei, @args) = @_; - # _post_augment_maildir, _post_augment_mbox - my $m = "_post_augment_$self->{base_type}"; - $self->$m($lei, @args); + # _post_augment_mbox + my $m = $self->can("_post_augment_$self->{base_type}") or return; + $m->($self, $lei, @args); } sub ipc_atfork_child {
All of our current IMAP code relies on Mail::IMAPClient at the moment, so ensure we skip those tests on systems without that module. --- t/lei-convert.t | 2 +- t/lei-import-imap.t | 2 +- t/net_reader-imap.t | 2 +- xt/lei-auth-fail.t | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/t/lei-convert.t b/t/lei-convert.t index f58a0a80..29f8ba75 100644 --- a/t/lei-convert.t +++ b/t/lei-convert.t @@ -6,7 +6,7 @@ use PublicInbox::MboxReader; use PublicInbox::MdirReader; use PublicInbox::NetReader; require_git 2.6; -require_mods(qw(DBD::SQLite Search::Xapian)); +require_mods(qw(DBD::SQLite Search::Xapian Mail::IMAPClient)); my ($tmpdir, $for_destroy) = tmpdir; my $sock = tcp_server; my $cmd = [ '-imapd', '-W0', "--stdout=$tmpdir/1", "--stderr=$tmpdir/2" ]; diff --git a/t/lei-import-imap.t b/t/lei-import-imap.t index ee308723..a6ba805f 100644 --- a/t/lei-import-imap.t +++ b/t/lei-import-imap.t @@ -3,7 +3,7 @@ # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> use strict; use v5.10.1; use PublicInbox::TestCommon; require_git 2.6; -require_mods(qw(DBD::SQLite Search::Xapian)); +require_mods(qw(DBD::SQLite Search::Xapian Mail::IMAPClient)); my ($ro_home, $cfg_path) = setup_public_inboxes; my ($tmpdir, $for_destroy) = tmpdir; my $sock = tcp_server; diff --git a/t/net_reader-imap.t b/t/net_reader-imap.t index eea8b0fd..694b5a37 100644 --- a/t/net_reader-imap.t +++ b/t/net_reader-imap.t @@ -3,7 +3,7 @@ # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> use strict; use v5.10.1; use PublicInbox::TestCommon; require_git 2.6; -require_mods(qw(DBD::SQLite Search::Xapian)); +require_mods(qw(DBD::SQLite Search::Xapian Mail::IMAPClient)); my ($tmpdir, $for_destroy) = tmpdir; my ($ro_home, $cfg_path) = setup_public_inboxes; my $cmd = [ '-imapd', '-W0', "--stdout=$tmpdir/1", "--stderr=$tmpdir/2" ]; diff --git a/xt/lei-auth-fail.t b/xt/lei-auth-fail.t index 5308d0f9..78f8466d 100644 --- a/xt/lei-auth-fail.t +++ b/xt/lei-auth-fail.t @@ -2,6 +2,7 @@ # Copyright (C) 2021 all contributors <meta@public-inbox.org> # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> use strict; use v5.10.1; use PublicInbox::TestCommon; +require_mods(qw(Mail::IMAPClient)); # TODO: mock IMAP server which fails at authentication so we don't # have to make external connections to test this:
Due to an off-by-one error, we were unable to read mailboxes with only a single message of UID:1. Without this fix, the message with UID:1 could only be read after UID:2 was created; so there's no permanent data loss as long as a new message showed up. This affects all releases of public-inbox-watch with IMAP support, though it probably went unnoticed because single message inboxes are rare. --- lib/PublicInbox/NetReader.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/NetReader.pm b/lib/PublicInbox/NetReader.pm index 61ea538b..22ba4be7 100644 --- a/lib/PublicInbox/NetReader.pm +++ b/lib/PublicInbox/NetReader.pm @@ -353,17 +353,18 @@ sub _imap_fetch_all ($$$) { PublicInbox::IMAPTracker->new($url) : 0; my ($l_uidval, $l_uid) = $itrk ? $itrk->get_last : (); $l_uidval //= $r_uidval; # first time - $l_uid //= 1; + $l_uid //= 0; if ($l_uidval != $r_uidval) { return "E: $url UIDVALIDITY mismatch\n". "E: local=$l_uidval != remote=$r_uidval"; } my $r_uid = $r_uidnext - 1; - if ($l_uid != 1 && $l_uid > $r_uid) { + if ($l_uid > $r_uid) { return "E: $url local UID exceeds remote ($l_uid > $r_uid)\n". "E: $url strangely, UIDVALIDLITY matches ($l_uidval)\n"; } return if $l_uid >= $r_uid; # nothing to do + $l_uid ||= 1; warn "# $url fetching UID $l_uid:$r_uid\n" unless $self->{quiet}; $mic->Uid(1); # the default, we hope
Requiring TEST_IMAP_WRITE_URL to be set to a writable IMAP server URL isn't ideal, but it works for now until we have time to setup a mock dovecot/cyrus/etc... instance for testing. --- MANIFEST | 2 ++ lib/PublicInbox/NetReader.pm | 42 +++++++++++++++++++------------ lib/PublicInbox/NetWriter.pm | 26 +++++++++++++++++++ xt/net_writer-imap.t | 48 ++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 lib/PublicInbox/NetWriter.pm create mode 100644 xt/net_writer-imap.t diff --git a/MANIFEST b/MANIFEST index 3d9ad616..21e37678 100644 --- a/MANIFEST +++ b/MANIFEST @@ -213,6 +213,7 @@ lib/PublicInbox/NNTP.pm lib/PublicInbox/NNTPD.pm lib/PublicInbox/NNTPdeflate.pm lib/PublicInbox/NetReader.pm +lib/PublicInbox/NetWriter.pm lib/PublicInbox/NewsWWW.pm lib/PublicInbox/OnDestroy.pm lib/PublicInbox/Over.pm @@ -471,6 +472,7 @@ xt/lei-sigpipe.t xt/mem-imapd-tls.t xt/mem-msgview.t xt/msgtime_cmp.t +xt/net_writer-imap.t xt/nntpd-validate.t xt/perf-msgview.t xt/perf-nntpd.t diff --git a/lib/PublicInbox/NetReader.pm b/lib/PublicInbox/NetReader.pm index 22ba4be7..92d004bc 100644 --- a/lib/PublicInbox/NetReader.pm +++ b/lib/PublicInbox/NetReader.pm @@ -8,6 +8,8 @@ use v5.10.1; use parent qw(Exporter PublicInbox::IPC); use PublicInbox::Eml; +our %IMAPflags2kw = map {; "\\\u$_" => $_ } qw(seen answered flagged draft); + # TODO: trim this down, this is huge our @EXPORT = qw(uri_new uri_scheme uri_section mic_for nn_new nn_for @@ -33,6 +35,7 @@ sub uri_section ($) { sub auth_anon_cb { '' }; # for Mail::IMAPClient::Authcallback +# mic_for may prompt the user and store auth info, prepares mic_get sub mic_for { # mic = Mail::IMAPClient my ($self, $url, $mic_args, $lei) = @_; require PublicInbox::URIimap; @@ -286,7 +289,12 @@ sub imap_common_init ($;$) { for my $url (@{$self->{imap_order}}) { my $uri = PublicInbox::URIimap->new($url); my $sec = uri_section($uri); - $mics->{$sec} //= mic_for($self, $url, $mic_args, $lei); + $mics->{$sec} //= mic_for($self, "$sec/", $mic_args, $lei); + next unless $self->isa('PublicInbox::NetWriter'); + my $dst = $uri->mailbox // next; + my $mic = $mics->{$sec}; + next if $mic->exists($dst); # already exists + $mic->create($dst) or die "CREATE $dst failed <$url>: $@"; } $mics; } @@ -312,13 +320,6 @@ sub errors { undef; } -my %IMAPflags2kw = ( - '\Seen' => 'seen', - '\Answered' => 'answered', - '\Flagged' => 'flagged', - '\Draft' => 'draft', -); - sub _imap_do_msg ($$$$$) { my ($self, $url, $uid, $raw, $flags) = @_; # our target audience expects LF-only, save storage @@ -418,25 +419,34 @@ sub _imap_fetch_all ($$$) { $err; } +# uses cached auth info prepared by mic_for +sub mic_get { + my ($self, $sec) = @_; + my $mic_arg = $self->{mic_arg}->{$sec} or + die "BUG: no Mail::IMAPClient->new arg for $sec"; + if (defined(my $cb_name = $mic_arg->{Authcallback})) { + if (ref($cb_name) ne 'CODE') { + $mic_arg->{Authcallback} = $self->can($cb_name); + } + } + my $mic = PublicInbox::IMAPClient->new(%$mic_arg); + $mic && $mic->IsConnected ? $mic : undef; +} + sub imap_each { my ($self, $url, $eml_cb, @args) = @_; my $uri = PublicInbox::URIimap->new($url); my $sec = uri_section($uri); - my $mic_arg = $self->{mic_arg}->{$sec} or - die "BUG: no Mail::IMAPClient->new arg for $sec"; local $0 = $uri->mailbox." $sec"; - my $cb_name = $mic_arg->{Authcallback}; - if (ref($cb_name) ne 'CODE') { - $mic_arg->{Authcallback} = $self->can($cb_name); - } - my $mic = PublicInbox::IMAPClient->new(%$mic_arg, Debug => 0); + my $mic = mic_get($self, $sec); my $err; - if ($mic && $mic->IsConnected) { + if ($mic) { local $self->{eml_each} = [ $eml_cb, @args ]; $err = _imap_fetch_all($self, $mic, $url); } else { $err = "E: not connected: $!"; } + warn $err if $err; $mic; } diff --git a/lib/PublicInbox/NetWriter.pm b/lib/PublicInbox/NetWriter.pm new file mode 100644 index 00000000..6f0a0b94 --- /dev/null +++ b/lib/PublicInbox/NetWriter.pm @@ -0,0 +1,26 @@ +# Copyright (C) 2021 all contributors <meta@public-inbox.org> +# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> + +# common writer code for IMAP (and later, JMAP) +package PublicInbox::NetWriter; +use strict; +use v5.10.1; +use parent qw(PublicInbox::NetReader); +use PublicInbox::Smsg; +use PublicInbox::MsgTime qw(msg_timestamp); + +my %IMAPkw2flags; +@IMAPkw2flags{values %PublicInbox::NetReader::IMAPflags2kw} = + keys %PublicInbox::NetReader::IMAPflags2kw; + +sub imap_append { + my ($mic, $folder, $bref, $smsg, $eml) = @_; + $bref //= \($eml->as_string); + $smsg //= bless { }, 'PublicInbox::Smsg'; + $smsg->{ts} //= msg_timestamp($eml // PublicInbox::Eml->new($$bref)); + my @f = map { $IMAPkw2flags{$_} } @{$smsg->{kw}}; + $mic->append_string($folder, $$bref, "@f", $smsg->internaldate) or + die "APPEND $folder: $@"; +} + +1; diff --git a/xt/net_writer-imap.t b/xt/net_writer-imap.t new file mode 100644 index 00000000..ea812f16 --- /dev/null +++ b/xt/net_writer-imap.t @@ -0,0 +1,48 @@ +#!perl -w +# Copyright (C) 2021 all contributors <meta@public-inbox.org> +# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> +use strict; use v5.10.1; use PublicInbox::TestCommon; +use Sys::Hostname qw(hostname); +use POSIX qw(strftime); +use PublicInbox::OnDestroy; +use PublicInbox::URIimap; +use PublicInbox::Config; +my $imap_url = $ENV{TEST_IMAP_WRITE_URL} or + plan skip_all => 'TEST_IMAP_WRITE_URL unset'; +my $uri = PublicInbox::URIimap->new($imap_url); +defined($uri->path) and + plan skip_all => "$imap_url should not be a mailbox (just host:port)"; +require_mods('Mail::IMAPClient'); +require_ok 'PublicInbox::NetWriter'; +my $host = (split(/\./, hostname))[0]; +my ($base) = ($0 =~ m!\b([^/]+)\.[^\.]+\z!); +my $folder = "INBOX.$base-$host-".strftime('%Y%m%d%H%M%S', gmtime(time)). + "-$$-".sprintf('%x', int(rand(0xffffffff))); +my $nwr = PublicInbox::NetWriter->new; +$imap_url .= '/' unless substr($imap_url, -1) eq '/'; +my $folder_uri = PublicInbox::URIimap->new("$imap_url/$folder"); +is($folder_uri->mailbox, $folder, 'folder correct') or + BAIL_OUT "BUG: bad $$uri"; +$nwr->add_url($$folder_uri); +is($nwr->errors, undef, 'no errors'); +$nwr->{pi_cfg} = bless {}, 'PublicInbox::Config'; +my $mics = $nwr->imap_common_init; +my $mic = (values %$mics)[0]; +my $cleanup = PublicInbox::OnDestroy->new(sub { + $mic->delete($folder) or fail "delete $folder <$$folder_uri>: $@"; +}); +my $imap_append = $nwr->can('imap_append'); +my $smsg = bless { kw => [ 'seen' ] }, 'PublicInbox::Smsg'; +$imap_append->($mic, $folder, undef, $smsg, eml_load('t/plack-qp.eml')); +my @res; +$nwr->{quiet} = 1; +$nwr->imap_each($$folder_uri, sub { + my ($u, $uid, $kw, $eml, $arg) = @_; + push @res, [ $kw, $eml ]; +}); +is(scalar(@res), 1, 'got appended message'); +is_deeply(\@res, [ [ [ 'seen' ], eml_load('t/plack-qp.eml') ] ], + 'uploaded message read back'); + +undef $cleanup; +done_testing;
This interpolation is used by the upstream URI package and we rely on it elsewhere for HTTP(S) URIs, so save ourselves some surprises down the line. --- lib/PublicInbox/URIimap.pm | 1 + t/uri_imap.t | 1 + xt/net_writer-imap.t | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/PublicInbox/URIimap.pm b/lib/PublicInbox/URIimap.pm index ab0908b7..db84ee5e 100644 --- a/lib/PublicInbox/URIimap.pm +++ b/lib/PublicInbox/URIimap.pm @@ -13,6 +13,7 @@ package PublicInbox::URIimap; use strict; use URI::Split qw(uri_split uri_join); # part of URI use URI::Escape qw(uri_unescape); +use overload '""' => \&as_string; my %default_ports = (imap => 143, imaps => 993); diff --git a/t/uri_imap.t b/t/uri_imap.t index 6c4207c3..f7c78665 100644 --- a/t/uri_imap.t +++ b/t/uri_imap.t @@ -19,6 +19,7 @@ is($uri->auth, undef); is($uri->user, undef); $uri = PublicInbox::URIimap->new('imaps://foo@0/'); +is("$uri", $uri->as_string, '"" overload works'); is($uri->host, '0', 'numeric host'); is($uri->user, 'foo', 'user extracted'); diff --git a/xt/net_writer-imap.t b/xt/net_writer-imap.t index ea812f16..dfd765be 100644 --- a/xt/net_writer-imap.t +++ b/xt/net_writer-imap.t @@ -29,7 +29,7 @@ $nwr->{pi_cfg} = bless {}, 'PublicInbox::Config'; my $mics = $nwr->imap_common_init; my $mic = (values %$mics)[0]; my $cleanup = PublicInbox::OnDestroy->new(sub { - $mic->delete($folder) or fail "delete $folder <$$folder_uri>: $@"; + $mic->delete($folder) or fail "delete $folder <$folder_uri>: $@"; }); my $imap_append = $nwr->can('imap_append'); my $smsg = bless { kw => [ 'seen' ] }, 'PublicInbox::Smsg';