gnodet-bot commented on code in PR #26763:
URL: https://github.com/apache/camel/pull/26763#discussion_r4080419283
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailBinding.java:
##########
@@ -363,14 +373,28 @@ public void extractAttachmentsFromMail(Message message,
Map<String, Attachment>
protected void extractAttachmentsFromMultipart(Multipart mp, Map<String,
Attachment> map)
throws MessagingException, IOException {
+ extractAttachmentsFromMultipart(mp, map, 0);
+ }
+
+ private void extractAttachmentsFromMultipart(Multipart mp, Map<String,
Attachment> map, int depth)
+ throws MessagingException, IOException {
+
+ if (depth > maxMultipartDepth) {
Review Comment:
**Depth semantics are off by one vs. the Javadoc.** With
`maxMultipartDepth=20` (the default), the guard fires at `depth > 20`, meaning
levels 0–20 are all processed — that's 21 levels, not 20. A user setting
`maxMultipartDepth=3` gets 4 levels traversed. The implementation is consistent
and the test confirms it, but the description (`"maximum nesting depth"`)
implies the bound is inclusive-exclusive rather than inclusive-inclusive.
Either use `depth >= maxMultipartDepth` (so the bound is the hard limit on
recursion count), or update the Javadoc to clarify that the option is the last
depth that is *not* skipped:
```suggestion
if (depth >= maxMultipartDepth) {
```
…and adjust the constant accordingly (`MAIL_DEFAULT_MAX_MULTIPART_DEPTH =
21` if you want 20 levels of nesting, or leave at 20 and document that 20 means
"up to 20 levels processed"). Either way, make the semantics explicit.
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailEndpoint.java:
##########
@@ -199,6 +199,9 @@ public MailBinding getBinding() {
headerFilterStrategy, contentTypeResolver, decode,
mapMailMessage, failDuplicate,
generateMissingAttachmentNames,
handleDuplicateAttachmentNames);
+ if (getConfiguration() != null) {
+
binding.setMaxMultipartDepth(getConfiguration().getMaxMultipartDepth());
+ }
Review Comment:
**Inconsistent null-guard style.** Every other config value in this method
uses an inline ternary: `getConfiguration() != null ?
getConfiguration().isDecodeFilename() : false`. This block uses a separate `if`
statement instead. Functionally identical since `MailBinding` already defaults
to `MAIL_DEFAULT_MAX_MULTIPART_DEPTH`, but the inconsistency will trip up
future readers.
```suggestion
binding.setMaxMultipartDepth(
getConfiguration() != null ?
getConfiguration().getMaxMultipartDepth() :
MailConstants.MAIL_DEFAULT_MAX_MULTIPART_DEPTH);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
