You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "jeantil (via GitHub)" <gi...@apache.org> on 2025/04/01 14:16:26 UTC
[PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
jeantil opened a new pull request, #2696:
URL: https://github.com/apache/james-project/pull/2696
PR stacked on https://github.com/apache/james-project/pull/2695 please only review commits for JAMES-4128
This introduces a breaking change in Mailet api and probably warrants detailed discussion, the motivation is detailed in the issue and the commit message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil merged PR #2696:
URL: https://github.com/apache/james-project/pull/2696
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2769699264
> As such I'd expect this to by design remain the case.
Agreed which is why I didn't add the obvious case as a default behaviour on the interface.
> Thinking more about it those properties do not concern the mailets itself but the way the mailet container should run it. As such it do not seem legitimate to even have the mailet aware about this.
:100:
I'm trying your suggestion to see if I can make it work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2769575785
```
The last 2 methods have an obvious implementation of using Option.empty but we would not use a default implementation on the interface. The GenericMailet would provide an implementation matching the current behaviour that extracts the value from the config.
```
As somebody extensively doing mailet configuration on a day to day basis I expect to be able to say :"manage errors from this mailet that way" with conf.
Being able to do so arbitrarily with code had several time been invaluable.
As such I'd expect this to *by design* remain the case.
Thinking more about it those properties do not concern the mailets itself but the way the mailet container should run it. As such it do not seem legitimate to even have the mailet aware about this.
What I think we are after is to add this `MailProcessingErrorHandlingConfiguration` config be part of the `MatcherMailetPair` and thus the mailet be transparent to it. AbstractStateMailetProcessor could initialise the `MailProcessingErrorHandlingConfiguration` at the same time than the mailet and the matcher. Bonus: we would not polute the mailet API with this, not change anything to the existing behaviour, and still achieve perfect encapsulation of the mailet config.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2769981633
See the fixups for a version that exposes neither the config nor the `onMatchException` and `onMailetException`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2771535236
Needs a rebase !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil commented on code in PR #2696:
URL: https://github.com/apache/james-project/pull/2696#discussion_r2023550113
##########
server/mailet/mailetcontainer-impl/src/main/java/org/apache/james/mailetcontainer/lib/AbstractStateCompositeProcessor.java:
##########
@@ -151,13 +150,17 @@ private void checkProcessors() throws ConfigurationException {
}
}
- private Stream<ProcessingState> requiredProcessorStates(Mailet mailet) {
- return Stream.concat(mailet.requiredProcessingState().stream(),
- Stream.of(
- mailet.onMailetException(),
- mailet.onMatchException())
- .flatMap(Optional::stream)
- .map(ProcessingState::new));
+ private Stream<ProcessingState> requiredProcessorStates(MatcherMailetPair matcherMailetPair) {
+ var mailet = matcherMailetPair.getMailet();
+
+ return Stream.concat(
+ mailet.requiredProcessingState().stream(),
+ Stream.of(
+ matcherMailetPair.onMailetException(),
+ matcherMailetPair.onMatchException()
+ ).flatMap(Optional::stream)
+ .map(ProcessingState::new)
+ );
Review Comment:
don't be sorry, byt I'm not sure I understand your comment/suggestion.
I find that clearly separating scopes in stream pipelines is much easier to read. I put the closing parens on line 160 and 163 on the next line on purpose to clearly align the scope nmarkers
Your propose change is much more compact but the visual scoping is lost.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2770470120
> Thanks for putting this together.
my pleasure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2696:
URL: https://github.com/apache/james-project/pull/2696#discussion_r2023252193
##########
server/mailet/mailetcontainer-impl/src/main/java/org/apache/james/mailetcontainer/lib/AbstractStateCompositeProcessor.java:
##########
@@ -151,13 +150,17 @@ private void checkProcessors() throws ConfigurationException {
}
}
- private Stream<ProcessingState> requiredProcessorStates(Mailet mailet) {
- return Stream.concat(mailet.requiredProcessingState().stream(),
- Stream.of(
- mailet.onMailetException(),
- mailet.onMatchException())
- .flatMap(Optional::stream)
- .map(ProcessingState::new));
+ private Stream<ProcessingState> requiredProcessorStates(MatcherMailetPair matcherMailetPair) {
+ var mailet = matcherMailetPair.getMailet();
+
+ return Stream.concat(
+ mailet.requiredProcessingState().stream(),
+ Stream.of(
+ matcherMailetPair.onMailetException(),
+ matcherMailetPair.onMatchException()
+ ).flatMap(Optional::stream)
+ .map(ProcessingState::new)
+ );
Review Comment:
```
Stream.of(
matcherMailetPair.onMailetException(),
matcherMailetPair.onMatchException())
.flatMap(Optional::stream)
.map(ProcessingState::new));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "jeantil (via GitHub)" <gi...@apache.org>.
jeantil commented on PR #2696:
URL: https://github.com/apache/james-project/pull/2696#issuecomment-2771641649
rebased and fixups squashed. I'm waiting for jenkins build to complete since this has not had a green build yet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2696:
URL: https://github.com/apache/james-project/pull/2696#discussion_r2023555561
##########
server/mailet/mailetcontainer-impl/src/main/java/org/apache/james/mailetcontainer/lib/AbstractStateCompositeProcessor.java:
##########
@@ -151,13 +150,17 @@ private void checkProcessors() throws ConfigurationException {
}
}
- private Stream<ProcessingState> requiredProcessorStates(Mailet mailet) {
- return Stream.concat(mailet.requiredProcessingState().stream(),
- Stream.of(
- mailet.onMailetException(),
- mailet.onMatchException())
- .flatMap(Optional::stream)
- .map(ProcessingState::new));
+ private Stream<ProcessingState> requiredProcessorStates(MatcherMailetPair matcherMailetPair) {
+ var mailet = matcherMailetPair.getMailet();
+
+ return Stream.concat(
+ mailet.requiredProcessingState().stream(),
+ Stream.of(
+ matcherMailetPair.onMailetException(),
+ matcherMailetPair.onMatchException()
+ ).flatMap(Optional::stream)
+ .map(ProcessingState::new)
+ );
Review Comment:
Yes mostly personal taste.
Please feel free to move on with this if prefered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [JAMES-4128] improve encapsulation of config in Mailet [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2696:
URL: https://github.com/apache/james-project/pull/2696#discussion_r2023252193
##########
server/mailet/mailetcontainer-impl/src/main/java/org/apache/james/mailetcontainer/lib/AbstractStateCompositeProcessor.java:
##########
@@ -151,13 +150,17 @@ private void checkProcessors() throws ConfigurationException {
}
}
- private Stream<ProcessingState> requiredProcessorStates(Mailet mailet) {
- return Stream.concat(mailet.requiredProcessingState().stream(),
- Stream.of(
- mailet.onMailetException(),
- mailet.onMatchException())
- .flatMap(Optional::stream)
- .map(ProcessingState::new));
+ private Stream<ProcessingState> requiredProcessorStates(MatcherMailetPair matcherMailetPair) {
+ var mailet = matcherMailetPair.getMailet();
+
+ return Stream.concat(
+ mailet.requiredProcessingState().stream(),
+ Stream.of(
+ matcherMailetPair.onMailetException(),
+ matcherMailetPair.onMatchException()
+ ).flatMap(Optional::stream)
+ .map(ProcessingState::new)
+ );
Review Comment:
```
Stream.of(matcherMailetPair.onMailetException(),
matcherMailetPair.onMatchException())
.flatMap(Optional::stream)
.map(ProcessingState::new));
```
(Sorry)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org