Bug 8060 - [review] Fix meta handling for metas without dependencies
Summary: [review] Fix meta handling for metas without dependencies
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P2 blocker
Target Milestone: 4.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on: 7735
Blocks:
  Show dependency tree
 
Reported: 2022-10-11 10:21 UTC by Henrik Krohns
Modified: 2022-10-11 17:41 UTC (History)
3 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Fix metas without deps patch None Henrik Krohns [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Henrik Krohns 2022-10-11 10:21:12 UTC 
As seen on list, metas without dependencies are not always run correctly. Depending on rule running order due to perl hashes being random order, SA4TA3 will not evaluate if both subrules did not run before it.

meta __SA4TA3_1  6
meta __SA4TA3_2  2
meta  SA4TA3    (__SA4TA3_1 > 2) && (__SA4TA3_2 > 1)

This was because metas without dependencies were not recorder in $pms->{meta_check_ready}, thus do_meta_tests did not run them at all, and finish_meta_tests is not able to handle this scenario.

Fixed by saving metas without dependencies to $conf->{meta_nodeps} and initializing $pms->{meta_check_ready} from it.
Comment 1 Henrik Krohns 2022-10-11 10:22:20 UTC 
Created attachment 5840 [details]
Fix metas without deps

Fix with minor cleanups and improved test that catches this. Vote to commit.
Comment 2 Henrik Krohns 2022-10-11 10:30:36 UTC 
The patch might affect the common "meta DISABLED_RULE 0" usage, which previously I think was considered an "unrun" rule, now it is considered run rule that did not hit.

Which now differs from other common way of "score DISABLED_RULE 0", which still makes rule "unrun".

I think both should have identical results, but should be allow metas depending on them to fully evaluate or not?
Comment 3 Henrik Krohns 2022-10-11 10:47:50 UTC 
On the other hand, maybe this is an advantage, so user can choose which way to disable a rule?

# Disable rule, consider rule unrun, don't let any meta depending on it be run
score DISABLED_RULE 0

# Disable rule, consider result not hitting, let any meta depending on it be run
meta DISABLED_RULE 0 --
Comment 4 Henrik Krohns 2022-10-11 10:51:49 UTC 
More I think about it, more it seems to make sense.

meta FOO 0
meta FOO 1

Those should not differ in the way they effect any metas. It's just a difference of rule hitting or not.

So +1 for my original patch.
Comment 5 Bill Cole 2022-10-11 12:45:41 UTC 
+1 to commit
Comment 6 Kevin A. McGrail 2022-10-11 16:46:37 UTC 
+1 from me as well
Comment 7 Henrik Krohns 2022-10-11 17:41:20 UTC 
Committed with few extra additions to basic_meta2.t tests:

Sending        trunk/lib/Mail/SpamAssassin/Conf/Parser.pm
Sending        trunk/lib/Mail/SpamAssassin/Conf.pm
Sending        trunk/lib/Mail/SpamAssassin/Plugin/Check.pm
Sending        trunk/t/basic_meta2.t
Transmitting file data ....done
Committing transaction...
Committed revision 1904529.