Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(email connector): implement new email connector, Outbound SMTP, IMAP and POP3, Inbound IMAP #3132

Merged
merged 26 commits into from
Sep 23, 2024

Conversation

mathias-vandaele
Copy link
Contributor

@mathias-vandaele mathias-vandaele commented Aug 20, 2024

Description

New implementation of the email connectors.

It has not been completely polished but ready for review.

I would like to come back on the label for the ET, and add new tests

Related issues

related #3108
related #3213
related #3214
related #3215

@mathias-vandaele mathias-vandaele self-assigned this Aug 20, 2024
@mathias-vandaele mathias-vandaele linked an issue Aug 20, 2024 that may be closed by this pull request
@mathias-vandaele mathias-vandaele force-pushed the 3108-email-connector-smtp-outbound-connector branch from 2b08f94 to baa9aa6 Compare August 26, 2024 11:37
@mathias-vandaele mathias-vandaele force-pushed the 3108-email-connector-smtp-outbound-connector branch from c0f1f60 to f82fe87 Compare August 30, 2024 08:20
@mathias-vandaele mathias-vandaele changed the title 3108 email connector smtp outbound connector feat(email connector): implement new email connector, Outbound SMTP, IMAP and POP3, Inbound IMAP Aug 30, 2024
@mathias-vandaele mathias-vandaele marked this pull request as ready for review August 30, 2024 15:17
@mathias-vandaele mathias-vandaele requested a review from a team as a code owner August 30, 2024 15:17
Copy link
Collaborator

@johnBgood johnBgood left a comment

Choose a reason for hiding this comment

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

Great work, I left some minor comments

Arrays.stream(e.getMessages())
.peek(
message -> {
if (markAsRead) this.jakartaUtils.markAsSeen(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt it be marked after the correlation? If the correlation fails we have already seen the message and wont process it again.

Copy link
Contributor Author

@mathias-vandaele mathias-vandaele Sep 10, 2024

Choose a reason for hiding this comment

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

We create a new process when a new message is received (idle https://en.wikipedia.org/wiki/IMAP_IDLE ), not on every unseen email, so even if it was marked as seen and the correlation fails, It would not be processed again..

Copy link
Contributor

Choose a reason for hiding this comment

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

But dont we want to reprocess it?

Comment on lines 102 to 109
connectorContext.correlateWithResult(
new ReadEmailResponse(
email.getMessageId(),
email.getFrom(),
email.getSubject(),
email.getSize(),
email.getBody().getBodyAsPlainText(),
email.getBody().getBodyAsHtml())));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be extracted into a separate method and switched to a map(this::correlateEmail)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

}
}

private List<String> createInboxList(Object folderToListen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a proper type like List<Inbox>? or something similar without string handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is really just to format the object, we dont know if that is a String comma separated or already a List made by the Feel engine

}
this.store.close();
} catch (MessagingException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Logging this error would help to debug it.

import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;

public class EmailProperties {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmailProperties is quite generic. What about EmailInboundConnectorProperties.

import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;

public class EmailListenerData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be a record.

Copy link
Contributor

Choose a reason for hiding this comment

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

Name is quite generic. What about EmailListenerConfig

optional = true,
feel = Property.FeelMode.optional,
binding = @TemplateProperty.PropertyBinding(name = "data.folderToListen"))
private Object folderToListen;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets go with a proper type instead of Object.

Copy link
Contributor Author

@mathias-vandaele mathias-vandaele Sep 10, 2024

Choose a reason for hiding this comment

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

image

It looks like this, on other connector, It is usually managed as an Object, Do you see an other way to do ?
It is the only way I could come up with to make complex search request

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I did not know that it is that complex. Would it be possible to map this with a proper model? Like ImapSearchQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Object can really depends:

I can have

{
  "field":"FROM",
  "value":"[email protected]"
}

or the example given previously, so the root object is not always the same

@sbuettner
Copy link
Contributor

Great work @mathias-vandaele. Added some comments.

return new JakartaEmailActionExecutor(sessionFactory, objectMapper);
}

public Object execute(EmailRequest emailRequest) {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
EmailActionExecutor.execute
; it is advisable to add an Override annotation.
return new JakartaEmailListener(sessionFactory);
}

public void startListener(InboundConnectorContext context) {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
EmailListener.startListener
; it is advisable to add an Override annotation.
switch (correlationResult) {
case CorrelationResult.Failure failure -> {
switch (failure.handlingStrategy()) {
case CorrelationFailureHandlingStrategy.ForwardErrorToUpstream __ -> {}

Check notice

Code scanning / CodeQL

Unread local variable Note

Variable 'ForwardErrorToUpstream __' is never read.
case CorrelationResult.Failure failure -> {
switch (failure.handlingStrategy()) {
case CorrelationFailureHandlingStrategy.ForwardErrorToUpstream __ -> {}
case CorrelationFailureHandlingStrategy.Ignore __ ->

Check notice

Code scanning / CodeQL

Unread local variable Note

Variable 'Ignore __' is never read.
executePostProcess(message, emailListenerConfig);
}
}
case CorrelationResult.Success __ -> executePostProcess(message, emailListenerConfig);

Check notice

Code scanning / CodeQL

Unread local variable Note

Variable 'Success __' is never read.
// Given
SmtpConfig smtpConfig = new SmtpConfig("smtp.example.com", 587, CryptographicProtocol.TLS);
Smtp smtp = new Smtp(null, smtpConfig);
Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
// Given
SmtpConfig smtpConfig = new SmtpConfig("smtp.example.com", 25, CryptographicProtocol.NONE);
Smtp smtp = new Smtp(null, smtpConfig);
Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
void testCreatePropertiesWithPop3AndNoSecurity() {
// Given
Pop3Config pop3Config = new Pop3Config("pop3.example.com", 110, CryptographicProtocol.NONE);
Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
// Given
Pop3Config pop3Config = new Pop3Config("pop3.example.com", 995, CryptographicProtocol.TLS);

Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
// Given
ImapConfig imapConfig = new ImapConfig("imap.example.com", 143, CryptographicProtocol.NONE);

Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
// Given
ImapConfig imapConfig = new ImapConfig("imap.tls-example.com", 993, CryptographicProtocol.TLS);
// When
Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
ImapConfig imapConfig = new ImapConfig("imap.ssl-example.com", 993, CryptographicProtocol.SSL);

// When
Authentication auth = mock(SimpleAuthentication.class);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'Authentication auth' is never read.
@mathias-vandaele
Copy link
Contributor Author

@johnBgood Would you be ok for final approvement ?

Copy link
Collaborator

@johnBgood johnBgood left a comment

Choose a reason for hiding this comment

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

Nothing blocking, just a few comments 👍

name = "type",
defaultValue = "simple",
description = "Specify the Email authentication strategy.")
public sealed interface Authentication permits SimpleAuthentication {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this interface if there's only a SimpleAuth ? It can be good for the future if we plan to support other types, so I'm just flagging it in case

import io.camunda.connector.api.inbound.InboundConnectorContext;

public interface EmailListener {
void startListener(InboundConnectorContext context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe start/stop would be sufficient

Action action = protocol.getProtocolAction();
Session session = jakartaUtils.createSession(protocol.getConfiguration());
return switch (action) {
case SmtpSendEmail smtpSendEmail -> smtpSendEmail(smtpSendEmail, authentication, session);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd split the methods called here into 3 different classes (imap, smtp, pop3) to avoid adding cognitive load to this class.
This would also enable testability for each of these methods.

import jakarta.mail.search.*;
import java.util.*;

public class JakartaEmailActionExecutor implements EmailActionExecutor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class is very important, it should have a Logger and proper logging in place, so that we can debug it some day

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class JakartaUtils {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This Utils class is huge, not sure if we can/want to refactor it a bit but I feel it touches upon too many things

@mathias-vandaele mathias-vandaele added this to the 8.6.0 milestone Sep 23, 2024
@mathias-vandaele mathias-vandaele added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit 5ab8ecb Sep 23, 2024
10 of 11 checks passed
@mathias-vandaele mathias-vandaele deleted the 3108-email-connector-smtp-outbound-connector branch September 23, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:qa Task containing all details related to QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[email connector] SMTP outbound connector
3 participants