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

Rspamd not found in transaction.results since 1.3.0 #34

Closed
DoobleD opened this issue Feb 27, 2023 · 2 comments · Fixed by #35
Closed

Rspamd not found in transaction.results since 1.3.0 #34

DoobleD opened this issue Feb 27, 2023 · 2 comments · Fixed by #35

Comments

@DoobleD
Copy link

DoobleD commented Feb 27, 2023

system info

Haraka | Haraka.js — Version: 3.0.1
 --- | :--- 
Node | v14.21.3
OS | Linux <redacted> 5.15.0-60-generic #66-Ubuntu SMP Fri Jan 20 14:29:49 UTC 2023 x86_64 GNU/Linux
openssl | OpenSSL 1.1.1n  15 Mar 2022

Expected behavior

connection.transaction.results.get('rspamd') should be defined.

Observed behavior

connection.transaction.results.get('rspamd') is not defined.

Steps to reproduce

This happens for me since 1.3.0. The rspamd transaction result isn't defined, for instance in the watch plugin and in our own plugins.

DEBUG] [-] [watch]  skip="rspamd not found"

Reverting to 1.2.0 resolves the issue. I tried tracking down the exact issue and suprisngly it's because of the change from a forEach to a for of in get_clean:

37d9067#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L383-R396

Specifically, it seems that the if (data[b].length) inside the loop returns more often since the change. It's unclear to me why this change affects anything, as far as I know it should be identical.

@nishils
Copy link
Contributor

nishils commented Mar 2, 2023

This is a bug that I also ran into. The root cause here is pretty subtle.

A return in a forEach loop goes to the next iteration. A return in a for loop exits out of the function entirely. This is a major bug introduced in 1.3.0 and should be patched. I'll see if I can get a PR up soon to help with this.

fyi @msimerson - the code is a bit unfamiliar to me but I can try putting a PR up but I do think releasing a patch version to fix this could be appropriate.

The smallest possible change to maintain the proposed es6 compatibility I think would be to change any return statements inside any loops that changes from forEach to for to be continue statements.

@nishils nishils mentioned this issue Mar 2, 2023
1 task
@msimerson
Copy link
Member

Thanks @nishils, that was exactly correct. I missed updating those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants