Optimize and reduce potential errors #3120

unquietwiki · 2022-12-19T06:48:02Z

Changes proposed in this pull request:

Safe optimizations via P42
More use of option-chaining (assisted by P42)
Error-checking via SonarLint

Checklist:

[n/a] docs updated
tests updated
Changes updated

plugins/helo.checks.js

connection.js

unquietwiki · 2022-12-19T08:10:01Z

@msimerson Well, I cleaned up the eslint errors that I missed on the first go; sorry about that. It's all come down to the two regex errors you'd have to look at and the self-signed test email certs I tried to update, but I guess the originals are still running in the testing environment. Hopefully, this was a worthwhile endeavor.

unquietwiki · 2022-12-19T09:56:07Z

@msimerson sorry for the extra testing emails. I was basically zerging through the testing errors. Think I fixed the self-signed cert error using XCA.

unquietwiki · 2022-12-19T10:09:11Z

Alright, calling it a night. I think I've done all I can with this; the remaining error seems to be git-based.

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +55927ac6944ce009372bac8a17e859caaae43fd1:refs/remotes/pull/3120/merge
Error: fatal: unable to access 'https://github.com/haraka/Haraka/': The requested URL returned error: 429

msimerson · 2022-12-20T09:03:12Z

I made it re-run the tests. The git checkout errors are now gone but there's some errors surfacing now in the test suite.

unquietwiki · 2022-12-21T00:50:45Z

@msimerson Thanks for this. I reverted most of the changes to clamd & helo.checks; only saw 1 assert failure, and that's apparently the ec.pem check; you may need to add the new certs on your end since I updated the ones I could to 3-year expirations. Otherwise, regarding clamd, it may not even be running in the test environment: I was getting connection failures to port 3310 in the logs.

msimerson · 2022-12-21T01:13:46Z

tests/plugins/spf.js

-const constants    = require('haraka-constants');
+const { Address }   = require('address-rfc2821');
+const fixtures      = require('haraka-test-fixtures');
+const constants     = require('haraka-constants');


There's a lot of lines in this PR that have nothing changed except the addition or removal of a space character. That's churn and it's hard to argue it serves any useful purpose. It also makes reviewing the actual changes substantially more work. I'd love to see the superfluous whitespace changes reverted.

msimerson · 2022-12-21T01:22:56Z

tests/plugins/dns_list_base.js

@@ -211,8 +208,7 @@ function zone_disable_test_func (zones, test, cb) {
    const again = () => {
        i++;
        setTimeout(() => {
-            if (this.plugin.zones.length === zones.length) return fin_check();
-            if (i > 4) return fin_check();
+            if (this.plugin.zones.length === zones.length || i > 4) return fin_check();


I'm not a fan of replacing two very simple statements with one more complicated one.

msimerson · 2022-12-21T01:24:37Z

tests/plugins/dkim_signer.js

 }

 const props = { selector: 'selector', domain: 'haraka.top', private_key: privateKey };

+const email = 'Ignored: header\r\n\r\nHi.\r\n\r\nWe lost the game. Are you hungry yet?\r\n\r\nJoe.\r\n';
+
+const ignoredHeader = 'Ignored: header\r\n\r\n';


Why extract these from within their test cases? This certainly doesn't improve legibility.

msimerson · 2022-12-21T01:26:07Z

tests/outbound/index.js

@@ -78,15 +78,20 @@ exports.outbound = {
    }
 }

+const repeats = 1000;


How does hoisting this out of the test case improve anything?

msimerson · 2022-12-21T01:28:32Z

tests/host_pool.js


        };

-        const retry_secs = 0.001; // 1ms


Hoisting this variable is an improvement why?

msimerson · 2022-12-21T01:32:41Z

tests/fixtures/util_hmailitem.js

-            callback(null, hmail);
+        for (const element of hmails) {
+            element.hostlist = [ delivery_domain ];
+            callback(null, element);


why rename the iterable to element instead of using hmail, as was already defined? More changed lines of code for what benefit?

msimerson · 2022-12-21T01:35:33Z

tests/connection.js

@@ -285,6 +285,7 @@ exports.relaying = {
    'sets and gets' (test) {
        test.expect(3);
        test.equal(this.connection.relaying, false);
+        // FIXME: should this be assigned vs compared?


Doesn't the next test answer the question? Try changing it and see what happens to the test. Poke at it until your satisfied and then remove these comments.

msimerson · 2022-12-21T01:37:39Z

spf.js

@@ -195,6 +185,7 @@ class SPF {
            let match;
            for (let txt_rr of txt_rrs) {
                // Node 0.11.x compatibility
+                // FIXME: remove when 0.11.x is no longer supported
                if (Array.isArray(txt_rr)) txt_rr = txt_rr.join('');


0.11 is long since no longer supported.

msimerson · 2022-12-21T01:40:58Z

smtp_client.js

-            const next = this.next;
-            delete this.next;
-            next(retval, msg);
+        if (!this.next) {


this is a good place for a one liner.

msimerson · 2022-12-21T01:46:34Z

server.js

@@ -514,7 +515,8 @@ Server.setup_http_listeners = () => {

 Server.init_master_respond = (retval, msg) => {
    if (!(retval === constants.ok || retval === constants.cont)) {
-        Server.logerror(`init_master returned error${((msg) ? `: ${msg}` : '')}`);
+        const errormsg = msg ? `: ${msg}` : ''; // should this indicate a reason if unknown?


Ask questions like this in comments, or issues, but not in the code.

msimerson · 2022-12-21T01:47:30Z

server.js

@@ -586,7 +588,8 @@ Server.init_child_respond = (retval, msg) => {
    }

    const pid = process.env.CLUSTER_MASTER_PID;
-    Server.logerror(`init_child returned error ${((msg) ? `: ${msg}` : '')}`);
+    const errormsg = msg ? `: ${msg}` : ''; // should this indicate a reason if unknown?
+    Server.logerror(`init_child returned error ${errormsg}`);


how does this change improve the code?

msimerson · 2022-12-21T01:50:21Z

rfc1869.js

-    else {
-        line = line.replace(strict ? /to:/i : /to:\s*/i, '');
-    }
+    line = (String(line)).replace(/\s*$/, '');


Why change this syntax for returning a new string?

msimerson · 2022-12-21T01:51:24Z

rfc1869.js

-        line = line.replace(strict ? /to:/i : /to:\s*/i, '');
-    }
+    line = (String(line)).replace(/\s*$/, '');
+    line = type === 'mail' ? line.replace(strict ? /from:/i : /from:\s*/i, '') : line.replace(strict ? /to:/i : /to:\s*/i, '');


this is dramatically less easier for a human to read and follow the flow of logic. In what way is it better?

msimerson · 2022-12-21T01:53:04Z

plugins/xclient.js

        if (match) {
            connection.logdebug(this, `found key=${match[1]} value=${match[2]}`);
            switch (match[1]) {
                case 'destaddr':
                case 'addr': {
                    // IPv6 is prefixed in the XCLIENT protocol
                    let ipv6;
+                    // FIXME: should this be assigned vs compared?


useless FIXME comments. Read them. Understand the code, and then get rid of them.

msimerson · 2022-12-21T02:40:19Z

I know this was a fair bit of work, but I think a fair bit more is in order before this thing can be landed. Suggestion: abandon this PR. Use your P42 & SonarLint tools anew. Except this time, make just one type of change. Create a PR for that. Get that PR landed. Then make another change. PR that, get it landed. Repeat. That'll also make it much easier to see which of the changes are causing test failures.

unquietwiki · 2022-12-21T03:32:51Z

Yeah, that works for me. I'll go over what you came up with and revisit. You may also want to have a look at the tooling yourself, too, to see what it's suggesting to me. This is JS: I've been using it for years and still get it wrong quite a bit since it's definitely trickier than other languages I work with; due apologies on that front.

unquietwiki · 2022-12-21T03:35:49Z

@msimerson Hey, can you make a determination on the test cert situation? I was trying to use XCA to fix those; the EC one's already expired, and the other two expire within the next 1-3 months. Whenever I do self-signed certs in my environments, I go for 1095 days (3y). Thx.

msimerson · 2022-12-21T03:46:31Z

You may also want to have a look at the tooling yourself, too, to see what it's suggesting to me.

I've seen what's its suggesting, and it's mostly right. I've been making a lot of those same changes, just here and there in the code as I'm visiting for other reasons. Most of the code is the way it is because it was written many years ago, for a different version of JS.

Don't worry about the test certs, they produce warnings but not test failures.

unquietwiki · 2022-12-21T05:09:44Z

@msimerson Thanks for clarifying on that, and being cool about this overall. I saw I was messing with this stuff 2-3 years ago, and wanted to try to get back into the swing of things; got some projects I'm fleshing out. I'll be more careful!

msimerson · 2022-12-21T05:19:10Z

Appreciate it. For fun, I took your suggestion and just kicked the tires on P42 with PR #3123. I think doing "one type of change" per PR is a workable flow. Makes reviewing a PR, even one that touches dozens of files, not horrible to review.

unquietwiki · 2022-12-21T05:24:20Z

@msimerson Rad. I found these tools trying to work on other open-source stuff and for a job I had for a year doing backend engineering for a datacenter setup. Beating your head on years-old code, mistakes and successes even out... 🤣

github-advanced-security bot found potential problems Dec 19, 2022

View reviewed changes

plugins/helo.checks.js Show resolved Hide resolved

msimerson reviewed Dec 19, 2022

View reviewed changes

connection.js Show resolved Hide resolved

P42 + SonarLint assisted cleanup

27a55c4

Try to fix helo.checks + clamd tests & update another cert

0cda6e9

msimerson reviewed Dec 21, 2022

View reviewed changes

tests/host_pool.js

};

const retry_secs = 0.001; // 1ms

Copy link

Member

msimerson Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoisting this variable is an improvement why?

msimerson reviewed Dec 21, 2022

View reviewed changes

unquietwiki closed this Dec 21, 2022

This was referenced Dec 21, 2022

P42: inline variables #3126

Closed

P42: inline variables into their references #3129

Merged

P42: if-else conditional streamlining #3130

Closed

P42 if-else refactorings #3133

Closed

SonarLint pass against detected bugs and code smell issues #3135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize and reduce potential errors #3120

Optimize and reduce potential errors #3120

unquietwiki commented Dec 19, 2022

unquietwiki commented Dec 19, 2022

unquietwiki commented Dec 19, 2022 •

edited

unquietwiki commented Dec 19, 2022

msimerson commented Dec 20, 2022

unquietwiki commented Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022 •

edited

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022 •

edited

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson Dec 21, 2022

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

Optimize and reduce potential errors #3120

Optimize and reduce potential errors #3120

Conversation

unquietwiki commented Dec 19, 2022

unquietwiki commented Dec 19, 2022

unquietwiki commented Dec 19, 2022 • edited

unquietwiki commented Dec 19, 2022

msimerson commented Dec 20, 2022

unquietwiki commented Dec 21, 2022

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

msimerson Dec 21, 2022 • edited

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

msimerson Dec 21, 2022 • edited

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

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

msimerson commented Dec 21, 2022

unquietwiki commented Dec 21, 2022

unquietwiki commented Dec 19, 2022 •

edited

msimerson Dec 21, 2022 •

edited

msimerson Dec 21, 2022 •

edited