feat: per account imap and smtp debugging #10301

SebastianKrupinski · 2024-10-28T23:27:00Z

At the moment there is no ability to debug a single mail account.

This adds the ability to enable debugging from the command line using a occ command on a single account and protocol. This also creates a separate file for each account that is enable instead of logging all accounts to the same log file. ( {data directory}/mail-{user id}-{account id}-imap.log )

// command arguments and options
mail:account:debug [--imap] [--imap-full] [--smtp]

// enable debugging for imap
mail:account:debug 14 --imap

// enable debugging for imap with message contents
mail:account:debug 14 --imap-full

// enable debugging for smtp
mail:account:debug 14 --smtp

// enable debugging for imap and smtp
mail:account:debug 14 --imap --smtp

// disable debugging
mail:account:debug 14

// enable for single run with environmental variable
NC_MAIL_DEBUG=imap php occ mail:account:sync 14

SebastianKrupinski · 2024-10-28T23:59:21Z

TODO: Fix integration test

SebastianKrupinski · 2024-10-29T00:36:15Z

TODO: Fix integration test

Done

Signed-off-by: SebastianKrupinski <[email protected]>

st3iny · 2024-11-05T15:52:10Z

lib/IMAP/IMAPClientFactory.php

+	private int $debugDefault = 1 << 0;   // 1 (0000 0001)
+	private int $debugFull = 1 << 1;     // 2 (0000 0010)


Can we please not bit-shift integers here. This is very hard to understand and this ain't C or a high frequency trading application.

I think it is fine to use an associative array here and serialize it to JSON.

Sure. I've removed the bit shifting, we can still just use a regular integer and do a bit comparison.

I meant both. I don't like having a compressed int as a field. That makes it hard to understand when encountered outside of the code, e.g. in a database dump.

As this is not performance critical code I would prefer another way, e.g. a serialized JSON string or concatenated string ("imap|smtp") or similar.

Done, I've changed it to a concatenated string, although I think int would have cleaner.

st3iny

See the comment above.

Signed-off-by: SebastianKrupinski <[email protected]>

st3iny

I posted a reply in the thread above.

I meant both. I don't like having a compressed int as a field. That makes it hard to understand when encountered outside of the code, e.g. in a database dump.

As this is not performance critical code I would prefer another way, e.g. a serialized JSON string or concatenated string ("imap|smtp") or similar.

kesselb · 2024-11-05T18:57:44Z

Please change "4.1.0-alpha.2" to "4.1.0-alpha.3" in info.xml to trigger the migration.

kesselb · 2024-11-05T19:32:01Z

Please change "4.1.0-alpha.2" to "4.1.0-alpha.3" in info.xml to trigger the migration.

You may want to wait until #10326 is merged and bump after a rebase.

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski · 2024-11-05T21:33:45Z

Please change "4.1.0-alpha.2" to "4.1.0-alpha.3" in info.xml to trigger the migration.

You may want to wait until #10326 is merged and bump after a rebase.

Done. I've changed the version to '4.2.0-alpha.1' that way it will migrate properly after the '4.2.0-alpha.0' release.

Signed-off-by: SebastianKrupinski <[email protected]>

ChristophWurst

What I'm missing now is a way to enable Mail debug mode for one occ run.

Before I could do php NC_debug=1 occ mail:account:sync .... This allowed running one sync in debug mode and leave the rest (of production) untouched. Now I have to globally set the account for debug and it will influence production, e.g. through background jobs or when the user has the app open in the browser.

Any idea how we can bring the old trick back?

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski · 2024-11-07T00:47:25Z

What I'm missing now is a way to enable Mail debug mode for one occ run.

Before I could do php NC_debug=1 occ mail:account:sync .... This allowed running one sync in debug mode and leave the rest (of production) untouched. Now I have to globally set the account for debug and it will influence production, e.g. through background jobs or when the user has the app open in the browser.

Any idea how we can bring the old trick back?

So you want to use a single use environmental variable to enable debugging, no problem.

NC_MAIL_DEBUG=imap php occ mail:account:sync 14

Signed-off-by: Sebastian Krupinski <[email protected]>

Signed-off-by: SebastianKrupinski <[email protected]>

kesselb · 2024-11-07T19:19:54Z

I’d like to suggest removing the option to enable debugging separately for each protocol (like IMAP or SMTP). This would make the setup simpler, with just one debug option instead. I think enabling debugging for individual protocols adds extra complexity without much benefit.

kesselb · 2024-11-07T19:21:14Z

.github/workflows/test.yml

@@ -184,7 +184,7 @@ jobs:
            run: echo "SELECT * FROM mysql.slow_log WHERE sql_text LIKE '%oc_mail%' AND sql_text NOT LIKE '%information_schema%'" | mysql -h 127.0.0.1 -u root -pmy-secret-pw
          - name: Print debug logs
            if: ${{ always() }}
-            run: cat nextcloud/data/horde_*.log
+            run: cat nextcloud/data/mail-*-*-imap.log


Suggested change

run: cat nextcloud/data/mail-*-*-imap.log

run: cat nextcloud/data/mail*.log

To also print smtp and sieve like before?

This would kind of defeat the purpose of per account logging, as all account smtp transmissions would end up in the same file?

NVM. I miss understood this, sure I can change this.

kesselb · 2024-11-07T19:23:47Z

lib/Command/DebugAccount.php

+	protected const ARGUMENT_ACCOUNT_ID = 'account-id';
+	protected const OPTION_IMAP_DEFAULT = 'imap';
+	protected const OPTION_IMAP_FULL = 'imap-full';
+	protected const OPTION_SMTP_DEFAULT = 'smtp';


What about Sieve? ;)

It's using the SieveClientFactory

I looked at the SieveClientFactory, it will need to be refactored first, so I thought it would be best to add sieve in a separate PR.

kesselb · 2024-11-07T19:27:20Z

lib/IMAP/IMAPClientFactory.php

@@ -117,8 +117,16 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C
 				'backend' => $this->hordeCacheFactory->newCache($account),
 			];
 		}
-		if ($this->config->getSystemValue('debug', false)) {
-			$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
+		$debug = array_merge(


If we decide to keep the protocol option, then this code should go into a small service or into the account object.

classDiagram class DebugHelper DebugHelper : +debugImap(account) bool DebugHelper : +debugSmtp(account) bool DebugHelper : +debugSieve(account) bool DebugHelper : +fileNameImap(account) string DebugHelper : +fileNameSmtp(account) string DebugHelper : +fileNameSieve(account) string

Loading

Sure, that can be done. Let see what the rest think.

kesselb · 2024-11-07T19:51:17Z

lib/IMAP/IMAPClientFactory.php

-		if ($this->config->getSystemValue('debug', false)) {
-			$params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log';
+		$debug = array_merge(
+			explode('|', $this->config->getSystemValueString('MAIL_DEBUG')),


I'm aware that getSystemValueString is used here as a workaround for reading environment variables, but I have some concerns:

Although we don't have strict conventions for naming configuration variables, all variables in config.sample.php use lowercase. I’d prefer to keep this consistent.

For configuration options related to SMTP in config.sample.php, we currently use the mail_ prefix. While smtp_ might be more accurate, it’s not feasible to change this now 😞

Our standard format is usually app.mail.xyz, but this could be problematic when working with NC_ environment variables due to naming restrictions in Linux (see allowed characters in Linux environment variable names).

1 sure I can change it to lower case

2 yes I could make it support mail_ and smtp_ or anything else and this would enable global debugging, but the idea behind this PR, is to NOT touch the config file at all for mail communication issues, also to NOT enable debugging for all user(s) mail account(s) at the same time and to separate the transmission logs so that its easier and faster to find issues. Just imagine if you have a system with 100 mail accounts and only 2 accounts have an intermittent problem a few times a day.
yes I know that by adding the 'getSystemValueString' you can now set the debugging in the config.php, but the purpose was for one time environmental variables.

3 yes that is why it has to stay with an under sore, the only other option is one word

For configuration options related to SMTP in config.sample.php, we currently use the mail_ prefix. While smtp_ might be more accurate, it’s not feasible to change this now 😞

I was referring to the symfony mailer configuration in Nextcloud using mail_ as prefix and thus consider mail_debug not an ideal choice.

yes I know that by adding the 'getSystemValueString' you can now set the debugging in the config.php, but the purpose was for one time environmental variables.

In a previous version, you were using $_ENV / getenv, why did you withdraw this approach? I think that would be cleaner than hijacking SystemConfig 🏴‍☠️

I was referring to the symfony mailer configuration in Nextcloud using mail_ as prefix and thus consider mail_debug not an ideal choice.

Ah, Okay, yeah we can still change it to mail_app_debug or anything else.

In a previous version, you were using $_ENV / getenv, why did you withdraw this approach? I think that would be cleaner than hijacking SystemConfig 🏴‍☠️

I agree it was cleaner but abandoned the approach, ironically because I thought it would get flagged during the review process, for one of two reasons,

A. Because I was overriding account data in the MailAccount class with information from a different source. Didn't think this was best practice.

B. Because I was using raw system values

SebastianKrupinski · 2024-11-07T20:23:35Z

I’d like to suggest removing the option to enable debugging separately for each protocol (like IMAP or SMTP). This would make the setup simpler, with just one debug option instead. I think enabling debugging for individual protocols adds extra complexity without much benefit.

Well the logic is that you only diagnose issues in one protocol at a time anyway, so there was no point in logging protocols you are not interested in. Lets see what @ChristophWurst and @st3iny think.

SebastianKrupinski requested review from ChristophWurst, kesselb and GretaD as code owners October 28, 2024 23:27

SebastianKrupinski self-assigned this Oct 28, 2024

SebastianKrupinski added the 3. to review label Oct 28, 2024

SebastianKrupinski force-pushed the enh/add-occ-debug-command branch 2 times, most recently from 0e8d1a5 to 22ba6c1 Compare October 28, 2024 23:49

SebastianKrupinski marked this pull request as draft October 28, 2024 23:58

SebastianKrupinski marked this pull request as ready for review October 29, 2024 00:32

feat: per account imap and smtp debugging

421a8dc

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski force-pushed the enh/add-occ-debug-command branch from 77998a6 to 421a8dc Compare October 29, 2024 14:40

st3iny reviewed Nov 5, 2024

View reviewed changes

fixup! feat: per account imap and smtp debugging

f832baa

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski requested a review from st3iny November 5, 2024 16:25

st3iny reviewed Nov 5, 2024

View reviewed changes

SebastianKrupinski added 2 commits November 5, 2024 16:27

fixup! feat: per account imap and smtp debugging

3f64efd

Signed-off-by: SebastianKrupinski <[email protected]>

fixup! feat: per account imap and smtp debugging

29e8e0b

Signed-off-by: SebastianKrupinski <[email protected]>

fixup! feat: per account imap and smtp debugging

97a09c0

Signed-off-by: SebastianKrupinski <[email protected]>

ChristophWurst reviewed Nov 6, 2024

View reviewed changes

fixup! feat: per account imap and smtp debugging

7aa605d

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski and others added 3 commits November 6, 2024 19:49

Merge branch 'main' into enh/add-occ-debug-command

34aa301

Signed-off-by: Sebastian Krupinski <[email protected]>

fixup! fixup! feat: per account imap and smtp debugging

7cc73f6

Signed-off-by: SebastianKrupinski <[email protected]>

fixup! fixup! feat: per account imap and smtp debugging

21c5495

Signed-off-by: SebastianKrupinski <[email protected]>

fixup! fixup! feat: per account imap and smtp debugging

378fa95

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski requested review from ChristophWurst and st3iny November 7, 2024 18:22

SebastianKrupinski and others added 2 commits November 7, 2024 13:23

Merge branch 'main' into enh/add-occ-debug-command

6f37da9

fixup! fixup! feat: per account imap and smtp debugging

2c0f6fe

Signed-off-by: SebastianKrupinski <[email protected]>

kesselb reviewed Nov 7, 2024

View reviewed changes

SebastianKrupinski added the enhancement label Nov 8, 2024

SebastianKrupinski added 2 commits November 11, 2024 18:42

Merge branch 'main' into enh/add-occ-debug-command

131fcea

Merge branch 'main' into enh/add-occ-debug-command

565737f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: per account imap and smtp debugging #10301

feat: per account imap and smtp debugging #10301

SebastianKrupinski commented Oct 28, 2024 •

edited

Loading

SebastianKrupinski commented Oct 28, 2024

SebastianKrupinski commented Oct 29, 2024

st3iny Nov 5, 2024

SebastianKrupinski Nov 5, 2024

st3iny Nov 5, 2024 •

edited

Loading

SebastianKrupinski Nov 5, 2024

st3iny left a comment •

edited

Loading

st3iny left a comment •

edited

Loading

kesselb commented Nov 5, 2024

kesselb commented Nov 5, 2024

SebastianKrupinski commented Nov 5, 2024

ChristophWurst left a comment

SebastianKrupinski commented Nov 7, 2024

kesselb commented Nov 7, 2024

kesselb Nov 7, 2024

SebastianKrupinski Nov 7, 2024

SebastianKrupinski Nov 7, 2024

kesselb Nov 7, 2024

SebastianKrupinski Nov 7, 2024

kesselb Nov 7, 2024

SebastianKrupinski Nov 7, 2024

kesselb Nov 7, 2024

SebastianKrupinski Nov 7, 2024

kesselb Nov 7, 2024

kesselb Nov 7, 2024

SebastianKrupinski Nov 7, 2024

SebastianKrupinski commented Nov 7, 2024

		private int $debugDefault = 1 << 0; // 1 (0000 0001)
		private int $debugFull = 1 << 1; // 2 (0000 0010)

	run: cat nextcloud/data/mail---imap.log
	run: cat nextcloud/data/mail*.log

feat: per account imap and smtp debugging #10301

Are you sure you want to change the base?

feat: per account imap and smtp debugging #10301

Conversation

SebastianKrupinski commented Oct 28, 2024 • edited Loading

SebastianKrupinski commented Oct 28, 2024

SebastianKrupinski commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st3iny Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st3iny left a comment • edited Loading

Choose a reason for hiding this comment

st3iny left a comment • edited Loading

Choose a reason for hiding this comment

kesselb commented Nov 5, 2024

kesselb commented Nov 5, 2024

SebastianKrupinski commented Nov 5, 2024

ChristophWurst left a comment

Choose a reason for hiding this comment

SebastianKrupinski commented Nov 7, 2024

kesselb commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski commented Nov 7, 2024

SebastianKrupinski commented Oct 28, 2024 •

edited

Loading

st3iny Nov 5, 2024 •

edited

Loading

st3iny left a comment •

edited

Loading

st3iny left a comment •

edited

Loading