1
\$\begingroup\$

This is a simple implementation of a rules engine. I am really hating the very long MATCH/CASE function. I am wondering what kind of Design Patterns could be used there to make this code more maintainable, reusable, and extensible.

Efficiency and Performance:

How can the performance be optimized when evaluating multiple rules and conditions, especially when dealing with a large number of emails?

Error Handling:

Are there sufficient error handling mechanisms in place, particularly for database operations and regular expression matching? What happens if an invalid regular expression is provided?

Extensibility:

How can this code be refactored to make it easier to add new condition types or rules in the future without modifying the existing codebase extensively?

Database Management:

What are the implications of opening a new SQLite connection for each instance of EmailFilterEngine? Should there be a connection pool or a more efficient way to manage database connections?

Code Readability and Maintenance:

The evaluate_scored_email method is quite lengthy and contains repetitive code blocks. How can this method be refactored to improve readability and reduce redundancy?

Testing and Validation:

How can unit tests be designed to cover various scenarios, including edge cases for each condition type? Is the current design testable enough, or are there any dependencies that should be mocked?

Security:

Are there any security concerns with the way SQL queries are handled, especially with string formatting for SQL statements? How can SQL injection attacks be prevented?

Configuration and Deployment:

Should the rules be loaded from an external configuration file or database rather than hard-coded in the code? What are the best practices for managing and updating rules in a production environment?

from dataclasses import dataclass
from enum import Enum, auto
from typing import List, Optional
import re
import sqlite3

class ConditionKind(Enum):
    SUBJECT_EQUALS = auto()
    SUBJECT_NOT_EQUALS = auto()
    SUBJECT_PATTERN = auto()
    SUBJECT_NOT_PATTERN = auto()
    BODY_EQUALS = auto()
    BODY_NOT_EQUALS = auto()
    BODY_PATTERN = auto()
    BODY_NOT_PATTERN = auto()
    REPLY_TO_EQUALS = auto()
    REPLY_TO_NOT_EQUALS = auto()
    REPLY_TO_PATTERN = auto()
    REPLY_TO_NOT_PATTERN = auto()
    DOMAIN_EQUALS = auto()
    DOMAIN_NOT_EQUALS = auto()
    DOMAIN_PATTERN = auto()
    DOMAIN_NOT_PATTERN = auto()
    SQL_QUERY = auto()

@dataclass
class EmailMessage:
    subject: str
    body: str
    sender_email: str
    reply_to: str
    domain: str
    company_domain: str

@dataclass(kw_only=True)
class ScoredEmailMessage(EmailMessage):
    score: float

@dataclass
class Condition:
    condition_kind: ConditionKind
    value: str

@dataclass
class FilteringRule:
    """
    conditions must be evaluated in conjunction.
    """
    name: str
    min_score: float
    conditions: List[Condition]

class EmailFilterEngine:
    def __init__(self, rules: List[FilteringRule], db_location: Optional[str] = None):
        self.rules = rules
        self.con = None
        if db_location:
            self.con = sqlite3.connect(db_location)
    
    def evaluate_scored_email(self, email: ScoredEmailMessage) -> List[str]:
        matched_rules = []
        for rule in self.rules:
            if email.score > rule.min_score:
                for condition in rule.conditions:
                    match [condition.condition_kind, condition.value]:
                        case [ConditionKind.SUBJECT_EQUALS, value]:
                            if email.subject == value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.SUBJECT_PATTERN, value]:
                            if re.match(value, email.subject):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.SUBJECT_NOT_EQUALS, value]:
                            if email.subject != value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.SUBJECT_NOT_PATTERN, value]:
                            if not re.match(value, email.subject):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.BODY_EQUALS, value]:
                            if email.body == value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.BODY_PATTERN, value]:
                            if re.match(value, email.body):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.BODY_NOT_EQUALS, value]:
                            if email.body != value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.BODY_NOT_PATTERN, value]:
                            if not re.match(value, email.body):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.REPLY_TO_EQUALS, value]:
                            if email.reply_to == value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.REPLY_TO_PATTERN, value]:
                            if re.match(value, email.reply_to):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.REPLY_TO_NOT_EQUALS, value]:
                            if email.reply_to != value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.REPLY_TO_NOT_PATTERN, value]:
                            if not re.match(value, email.reply_to):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.DOMAIN_EQUALS, value]:
                            if email.domain == value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.DOMAIN_PATTERN, value]:
                            if re.match(value, email.domain):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.DOMAIN_NOT_EQUALS, value]:
                            if email.domain != value.format(email=email):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.DOMAIN_NOT_PATTERN, value]:
                            if not re.match(value, email.domain):
                                matched_rules.append(rule.name)
                            continue
                        case [ConditionKind.SQL_QUERY, value]:
                            if not self.con:
                                raise RuntimeError("SQL conditions are not supported for this rules engine")
                            if not value.lower().startswith("select"):
                                raise RuntimeError("Only select statements are supported for SQL conditions")
                            if len(self.con.execute(value.format(email=email)).fetchall()) > 0:
                                matched_rules.append(rule.name)
                            continue
                        case _:
                            raise RuntimeError(
                                "Encountered an unknown condition type"
                            )
        return matched_rules
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Your recent edits invalidate J_H's review. Please don't change the code after it's already been reviewed. If you have new code to be reviewed, post as a new question. \$\endgroup\$
    – tdy
    Commented Jun 15 at 1:38

1 Answer 1

6
\$\begingroup\$

auto()

Idiomatic use of auto() in ConditionKind is very nice.

Repeating FOO and NOT_FOO seemed odd.

It looks like we want a (noun, verb) tuple, where noun is in {SUBJECT, BODY, REPLY, DOMAIN} and verb is in {EQUALS, PATTERN}, perhaps with negation. Though I don't really see the distinction there, since a regex pattern can trivially do a string literal EQUALS test.

It seems like the .value of SUBJECT should be "Subject:" rather than an integer.

Hmmm, interesting, regexes support a bunch of flags, such as re.MULTILINE, yet there's no re.NEGATED similar to grep -v. Perhaps you'd like to embellish the re module with such a flag.

Not sure why kw_only is helpful on a scored email message.

typing

Condition demands a value of type str. Yes, it's easy to turn things into strings. But for a pattern it seems more natural to re.compile(my_string) and pass in that object. For SQL we might possibly want to pass in a sqlalchemy text() object.

ToC

class EmailFilterEngine:
    def __init__( ... ):
        ...
        self.con = None
        if db_location:
            self.con = ...

Thank you for offering us a table of contents for object attributes. That is, thank you for ensuring the symbol .con always exists.

It's not obvious to me that this class should be responsible for maintaining the connection. Consider letting caller pass in an object which knows about the DB details.

Consider letting sqlalchemy worry about the connection to f"sqlite:///{db_file}", rather than forcing the DB vendor to be sqlite. Then you can easily switch to MariaDB or Postgres as the app grows.

nesting, helper

        for rule in self.rules:
            if email.score > rule.min_score:
                for condition in rule.conditions:

In evaluate_scored_email() we nest three levels down pretty quick. And then we have a ton of code. Recommend you bury that code in a private helper, if only so we can reset the indent level.

Also, the pattern of that code suggests the helper should be a predicate, used to decide whether caller will .append() the rule name. (It can also raise, as needed.)

There's not much logic in each clause. I will just observe that the standard library offers relevant functions such as __ne__, which you might pass in to a helper. And I've already mentioned that you may want to pass in a re.Pattern.

map

We don't need to match every possibility. You might find it convenient to construct a dict which maps from a condition kind to a lambda or def'd function that can evaluate the predicate.

.fetchall()

    if len(self.con.execute(value.format(email=email))
           .fetchall()) > 0:

We only care about the 0 rows versus 1 row distinction here. So there's no need to fetch all million of them. Also, you might tack on a " LIMIT 1" suffix to each query, so the backend query planner understands just what is needed.

Little Bobby Tables

Are there any security concerns with the way SQL queries are handled, especially with string formatting for SQL statements? How can SQL injection attacks be prevented?

I don't understand how you can even begin to ask that question. Or was it ChatGPT?

There's only a single query in the whole codebase, and we just saw that it does a str format() of an internet-provided email message to interpolate a SQL query. I think of internet-provided as meaning attacker controlled.

I don't tend to directly use the sqlite3 module you imported, as I let sqlalchemy work out the details, but I'm confident it offers a "bind variables" alternative to what you're doing in the OP code. Every DB interface supports that. Consult the docs already. You want to be passing in this dict: {"email": email}. And then let the DB layer substitute that into the query in a safe way.

connection pooling

What are the implications of opening a new SQLite connection for each instance of EmailFilterEngine?

If you were letting sqlalchemy's default Pool handle the details, this question wouldn't even arise.

\$\endgroup\$
1
  • \$\begingroup\$ A guiding light of agile development is YAGNI. Yogi Berra famously explained that it is difficult to make predictions, especially about the future. You didn't describe any extensions to be made in future. So let's pretend there was no Subject: requirement, and then it got added. Well, there's more than one component of an email the predicate can consult. So we might parameterize on that, perhaps as a dict key equal to "subject", perhaps as an attrgetter. \$\endgroup\$
    – J_H
    Commented Jun 14 at 2:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.