duesee / imap-codec

Rock-solid and complete codec for IMAP
Apache License 2.0
35 stars 13 forks source link

fix: Rework `DecodeError`. #333

Closed duesee closed 11 months ago

duesee commented 11 months ago

I would argue the tag is required in LiteralFound, so doing this breaking change is fine.

The implementation is... peculiar. But we are limited by the current parsing architecture. The only way we can "signal" that a literal was found is through IMAPParseError { kind: IMAPErrorKind::Literal { .. } }. This structure can be returned in any of the deeply-nested fn literal parsers that don't have access to anything before, i.e., the tag.

Edit: I have a better idea: The fn literal parser now trusts that the upper fn command parser will fill in the missing tag. This way, we don't need to parse the tag twice.

What do you think, @jakoschiko?

coveralls commented 11 months ago

Pull Request Test Coverage Report for Build 5798042220


Changes Missing Coverage Covered Lines Changed/Added Lines %
imap-codec/src/codec/decode.rs 143 149 95.97%
imap-types/src/response.rs 1 16 6.25%
<!-- Total: 222 243 91.36% -->
Files with Coverage Reduction New Missed Lines %
imap-codec/src/codec/encode.rs 6 91.02%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 5797532236: -0.2%
Covered Lines: 8767
Relevant Lines: 9389

💛 - Coveralls
jakoschiko commented 11 months ago

I don't have any opinion about the implementation, but a Tag inside of LiteralFound is fine for me. Currently it's a Option<Tag>. Does this reflect the IMAP standard? Is it possible to construct a Command with literals but without Tag? If yes, can the server reject this Command?

duesee commented 11 months ago

Currently it's a Option<Tag>. Does this reflect the IMAP standard? Is it possible to construct a Command with literals but without Tag?

No, it doesn't reflect the IMAP standard :-/ Every command in IMAP4rev1 must have a tag. ABNF:

command = tag SP (command-any / command-auth / command-nonauth / command-select) CRLF

If yes, can the server reject this Command?

Putting it more strongly: LiteralFound { tag: None } during command parsing should be unreachable!.

Option<T> is there for ~my own sanity~ technical reason. If we would like to make this stronger, we would need to split DecodeError into CommandDecodeError and ResponseDecodeError. Let me try something out. Actually, this would resolve some other unreachable! cases.

jakoschiko commented 11 months ago

Can't you simply preinitialize the Tag with a const default value?

duesee commented 11 months ago

I think the better solution would be to split the DecodeError. The tag is not the only thing that doesn't make sense during response decoding. There is also #332 and currently DecodeError could also tell that we received a non-sync literal in a response which is not allowed.

duesee commented 11 months ago

I think this is what we want:

pub enum GreetingDecodeError<'a> {
    Incomplete,
    Failed,
}

pub enum CommandDecodeError<'a> {
    Incomplete,
    LiteralFound {
        tag: Tag<'a>,
        length: u32,
        mode: LiteralMode,
    },
    Failed,
}

pub enum ResponseDecodeError {
    Incomplete,
    LiteralFound {
        length: u32,
    },
    Failed,
}