Implement middleware concept and their tests

[wneessen]

Thank you very much for the PR! Much appreciated!

I feel though, that the Client might be the wrong part for this extension. The Client only handles connections to a mail server, so the Middleware would only work when you actually use Send() to delivery the mail via a mail server. We have a lot of WriteTo*() methods on the Msg itself, which i. e. write the Msg into a file or use a local sendmail installation to deliver the mail. In these cases the Middleware would not be applied.

Do you think it would make sense to rather have the Middleware integrated in the Msg or even the Msgwriter instead to cover all current (and possibly future) cases? Also please fix the GolangCI Lint finding.

Looking forward to your comments!

[dhia-gharsallaoui]

Sounds cool, how would we add global middleware? Do you have any clue?

[wneessen]

Since all msg.WriteTo* methods basically rely on https://github.com/wneessen/go-mail/blob/main/msg.go#L661 and correspodingly on https://github.com/wneessen/go-mail/blob/main/msgwriter.go#L61, I suggest to have the Middleware type implemented on either of those two types and then have the middleware applied in one of the two methods: msg.WriteTo() or msgwriter.writeMsg(). I think that's a central place that would allow the middleware to be applied for either the msg.WriteTo() methods as well as any client.Send() method.

[dhia-gharsallaoui]

It's sound cool to rattach the middleware to the msgWriter. In this case I think we need to have the possibility to inject the msgWriter in the client like WithMsgWriter. In this case I suggest the following:

• Add msgWriter to the Client struct
• Add the msgWriter argument in msg.WriteTo().
• In msg.WriteTo() check if there is msgWriter if exist use it otherwise create a new one.
• Add middlewares to the msgWriter struct.
• Create the methods msgWriter.AddMiddleware and msgWriter.applyMiddlewares attached to msgWriter.
• Use msgWriter.applyMiddlewares in the msgwriter.writeMsg()
• Maybe we need also a msgWriter factory

Looking forward to your comments!

[wneessen]

I fear that route is will not really work, since i. e. in step 3 we have no reference to the Client, so we would need to make the message aware of the Client which is impractical. Giving it a further though, I believe the msgWriter route is too complex and not worth it. It's probably best to use the Msg route instead.

• Implement thte Middleware on Msg and add it to the Msg struct
• Since msgWriter is basically only used in msg.WriteTo and all other methods use WriteTo to access the msgWriter, that would the be the central point to apply the middleware just before the msgWriter is executed.

This way we keep the complexity simple and can handle all msg.WriteTo methods as well as the Client.Send methods.

What do you think?

[dhia-gharsallaoui]

Hello! Adding the middlewares to the Msg struct will force to attach the middlewares to each Msg, which means we can't use global middlewares. We believe it's much better to have global middlewares (you usually want to treat all your messages the same way, and if not, being global you can still filter the messages within the middlewares)

in step 3 we have no reference to the Client, so we would need to make the message aware of the Client

Actually, in step 2 we inject the msgWriter to the Msg, passing it thru Msg.WriteTo so the message is still unaware of the Client, it is (still) aware of the msgWriter but instead of instanciating it by itself it gets injected (and may fallback to instanciate it by itself if none are passed so it won't break the BC in case MsgWriteTo is called directly, not thru Client.Send).

Doing so we make the Client aware of the msgWriter since then we have to inject it to the client using WithMsgWriter.

[wneessen]

You have a good point and I agree with you. I'd say, let stop discussing and see what can be built from this brainstorming! :) Let me know if I can be of any assistance.

[dhia-gharsallaoui]

@wneessen feel free to review and give your remarks.

[codecov-commenter]

Codecov Report

Merging #50 (a4733f0) into main (7b03047) will decrease coverage by 9.51%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   83.80%   74.28%   -9.52%     
==========================================
  Files          11       11              
  Lines        1247     1256       +9     
==========================================
- Hits         1045      933     -112     
- Misses        139      289     +150     
+ Partials       63       34      -29     

Impacted Files	Coverage Δ
msg.go	86.66% <100.00%> (+0.22%)	:arrow_up:
client.go	40.65% <0.00%> (-32.80%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[wneessen]

Looks awesome! Thanks again for this cool feature!

[dhia-gharsallaoui]

@wneessen can you share a new release containing this feature please?

[wneessen]

Yes. the release is planed for this morning. I am just waiting for PR #54 to complete.