Skip to content

Improve send message checks#410

Merged
tomholub merged 7 commits intomasterfrom
feature/issue-399-compose-message-improvements
Jul 27, 2021
Merged

Improve send message checks#410
tomholub merged 7 commits intomasterfrom
feature/issue-399-compose-message-improvements

Conversation

@Kharchevskyi
Copy link
Contributor

@Kharchevskyi Kharchevskyi commented Jul 25, 2021

This PR extracts message validation and sending logic to ComposeMessageService.
Adds improvements to compose message validation

close #399


Tests:

  • Tests added

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Kharchevskyi Kharchevskyi marked this pull request as ready for review July 27, 2021 09:09
return .failure(.validationError(.missedPublicKey))
}

switch getPubKeys(for: recipients) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's only one error scenario and one success scenario when pulling public keys, I think an if or a guard would be better suited then a switch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there some mechanism like

let recipientPublicKeys = await getPubKeys(for: recipients)

let replyToMimeMsg = input.replyToMime
   .flatMap { String(data: $0, encoding: .utf8) }

let msg = SendableMsg(
    text: text,
    to: recipients.map(\.email),
    cc: [],
    bcc: [],
    from: email,
    subject: subject,
    replyToMimeMsg: replyToMimeMsg,
    atts: atts,
    pubKeys: allRecipientPubs + [myPubKey]
)
return .success(msg)

That would re-propagate the error from getPubKeys automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPubKeys returns Result<[String], MessageValidationError>, it can be success or failure cases with some associated values in it.
I can't use guard or if because I will miss one of 2 associated values

guard case .success(let allRecipientPubs) = getPubKeys(for: recipients) else {
           return ...(here I don't have associated error if getPubKeys fails)
}

It's possible to use do catch here, but I wouldn't recommend this solution, because we need to cast to some particular error in catch

do {
            try getPubKeys(for: recipients).get()
        } catch let error {
            (here it's Error type, not MessageValidationError)
            print(error)
        }

best way is to use Result<SendableMsg, ComposeMessageError>.Publisher and then in view controller we can chain validateMessage with encryptAndSend(message: sendableMessage) functions.
Please check latest commit.

// MARK: - Encrypt and Send
func encryptAndSend(message: SendableMsg) -> AnyPublisher<Void, ComposeMessageError> {
messageGateway.sendMail(mime: encryptMessage(with: message))
.mapError { ComposeMessageError.gatewayError($0) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does .mapError with .eraseToAnyPublisher do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func sendMail(mime: Data) -> Future<Void, Error> will return just Error for us. But we are expecting to have ComposeMessageError, so mapError in case of error will transform this to ComposeMessageError.gatewayError with associated error. either from Imap or Gmail api.

eraseToAnyPublisher - exposes an instance of AnyPublisher to the downstream subscriber, rather than this publisher’s actual type.
So without this it will have a return type as Publishers.MapError<Future<Void, Error>, ComposeMessageError>.
Please don't ask why)) Swift - 🤷
Type erasure is not the best in swift nowadays.
https://developer.apple.com/documentation/combine/just/erasetoanypublisher()

Comment on lines +130 to +136
private func encryptMessage(with msg: SendableMsg) -> Data {
do {
return try core.composeEmail(msg: msg, fmt: MsgFmt.encryptInline, pubKeys: msg.pubKeys).mimeEncoded
} catch {
fatalError()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible for this to fail for various reasons. For example, try to run this with an invalid public key. It will produce an error, and this should be handled gratiously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just "replayed" previous app behaviour.
Should I fix it in scope of this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, let's make another PR then. There, we'll check exactly what happens when we give it some bad input.

Comment on lines +86 to 104
return getPubKeys(for: recipients)
.mapError { ComposeMessageError.validationError($0) }
.map { allRecipientPubs in
let replyToMimeMsg = input.replyToMime
.flatMap { String(data: $0, encoding: .utf8) }

return SendableMsg(
text: text,
to: recipients.map(\.email),
cc: [],
bcc: [],
from: email,
subject: subject,
replyToMimeMsg: replyToMimeMsg,
atts: atts,
pubKeys: allRecipientPubs + [myPubKey]
)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's similarly weird to use map if we are talking about a single Future executing one time with a single failure or success. Map implies working with array of potentially many items, which seems a mismatch for what we're doing (even if this is otherwise common in iOS development).

I'll merge this but file a separate issue.

@tomholub tomholub merged commit bc8d0ba into master Jul 27, 2021
@tomholub tomholub deleted the feature/issue-399-compose-message-improvements branch July 27, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve send message checks

2 participants