8
\$\begingroup\$

I was in two minds as whether to post this question on stackoverflow or dba stackexchange but because I am asking for review, I thought of posting here.

I am new to Python and looking for feedback on whether the below code will work for a high website traffic, especially from database perspective and suggestions to make code more clean.

Use case here is as follows:

  1. We are sending mass email to all customers, irrespective of their email subscription status.
  2. Email says click this button to check your subscription status.
  3. Once customer clicks the button, user_helper.is_user_subscribed_to_emails is called with email of customer and show the return value to customer on web page.
  4. Table where user's subscription status is supposed to contain is not complete. It has rows for very few users and the intent behind this whole exercise is to fill the table with information about all users. Because of this, if 100 users are clicking on the button at a same time, whose info doesn't exist in table, it should successfully insert their info in database without any issues.

database/database_util.py:

from sqlalchemy import create_engine
from sqlalchemy.exc import SQLAlchemyError

db_uri = "sqlite:///temp.db"

try:
    engine = create_engine(db_uri, echo=False, future=True)
except SQLAlchemyError as e:
    print("There was an error in creating the engine")

database/models.py:

from sqlalchemy import Column
from sqlalchemy import String
from sqlalchemy import Boolean
from sqlalchemy.orm import declarative_base
from .database_util import engine
from sqlalchemy.exc import SQLAlchemyError

Base = declarative_base()

class User(Base):
    __tablename__ = "user"
    # user ids are alphanumeric, so we are going with String.
    email = Column(String, primary_key=True)
    subscribed_to_emails = Column(Boolean, default=False)
    def __repr__(self):
        return f"User(id={self.email!r}, subscribed_to_emails={self.subscribed_to_emails!r}"

try:
    Base.metadata.create_all(engine)
except SQLAlchemyError as e:
    print("Failed to initialize database metadata")

user_helper.py:

from sqlalchemy.orm import Session
from database.database_util import engine
from database.models import User
from sqlalchemy import select

def is_user_subscribed_to_emails(user_email):
    with Session(engine) as session:
        stmt = select(User).where(User.email == user_email)
        for row in session.execute(stmt).scalars():
            if row.subscribed_to_emails == False:
                return True
            else:
                return False
        # We didn't find the user in table and that means, user is subscribed. Create a user and return true.
        session.add(User(email=user_email, subscribed_to_emails = False))
        session.commit()
        return True
\$\endgroup\$
5
  • 5
    \$\begingroup\$ Have you considered sending the e-mail in batches, many smaller sets over the course of multiple hours? This would reduce the risk of peak server load. \$\endgroup\$
    – Mast
    Sep 6, 2022 at 5:45
  • 1
    \$\begingroup\$ I worked on a similar problem recently, and I recommend that you be very careful with capitalization checks in the email. With your code, "[email protected]" and "[email protected]" would be two separate rows in the db. \$\endgroup\$
    – Joe
    Sep 6, 2022 at 22:28
  • 1
    \$\begingroup\$ I hope you make your e-mails quite distinct from phishing ones. One way to make things better: have a separate user table with just a couple of columns. Possibly prefill the table for all users. Then an actual user dialog will result in a simple UPDATE which scales far better than an INSERT. \$\endgroup\$
    – Joop Eggen
    Sep 7, 2022 at 10:30
  • \$\begingroup\$ What I don't understand: You already have a complete list of user-emails, some of which already appear in the table with the subscription status. Why can't you just join the two tables to make the subscription status complete? Why the need for User interaction? (This has of course nothing todo with the structure / quality of the code). \$\endgroup\$
    – AEF
    Sep 7, 2022 at 12:08
  • \$\begingroup\$ @JoopEggen That is a good suggestion. Thanks! \$\endgroup\$
    – javanoob
    Sep 7, 2022 at 16:52

2 Answers 2

7
\$\begingroup\$

Architecture

Regarding this concern:

Table where user's subscription status is supposed to contain is not complete. It has rows for very few users and the intent behind this whole exercise is to fill the table with information about all users. Because of this, if 100 users are clicking on the button at a same time, whose info doesn't exist in table, it should successfully insert their info in database without any issues.

There's usually a way around it. You could use some distributed task queue such as Celery. That simplifies the management of task distribution and processing and can be used as a mechanism to distribute work across threads or machines. A task queue’s input is a unit of work called a task. Dedicated worker processes constantly monitor task queues for new work to perform.

Celery communicates via messages, usually using a broker to mediate between clients and workers. To initiate a task, the client adds a message to the queue, and the broker then delivers that message to a worker. RabbitMQ is one of the brokers transports completely supported by Celery.

Am not sure from your question if the app is built on top of FastAPI or Flask but from my experience I've been able to easily integrate RabbitMQ and Celery within my FastAPI projects (FastAPI also comes with async by default).

To create a better picture, here's how that would work:

  1. The client sends a request to your FastAPI/Flask application (that would be your user clicking on the button).
  2. FastAPI/Flask app sends the task message to the message broker.
  3. Celery workers consume the messages from the message broker. After the task finishes, it saves the result to the Result Backend and updates the task status.
  4. After sending the task to the message broker, the FastAPI/Flask app can also monitor the status of the task from the Result Backend.

For a simple implementation have a look here

Code

try:
    Base.metadata.create_all(engine)
except SQLAlchemyError as e:
    print("Failed to initialize database metadata")

It's bad practice to only print such critical issues. You should be raising an error and add logging beforehand.

if row.subscribed_to_emails == False:
    return True
else:
    return False

Can be rewritten as:

if row.subscribed_to_emails == False:
    return True
return False

There might be cases when this:

select(User).where(User.email == user_email)

could raise exceptions (something bad happened to that table / db connection etc) so I would advise you wrap that within a try/except block.

\$\endgroup\$
1
  • 10
    \$\begingroup\$ Actually, rather than comparing against False, it may be better to simply return not row.subscribed_to_emails. \$\endgroup\$ Sep 6, 2022 at 14:11
6
\$\begingroup\$

You may have heard the adage about separation of concerns, and that each function should do only one thing, and do it well.

Consider the function is_user_subscribed_to_emails:

def is_user_subscribed_to_emails(user_email):
    with Session(engine) as session:
        stmt = select(User).where(User.email == user_email)
        for row in session.execute(stmt).scalars():
            if row.subscribed_to_emails == False:
                return True
            else:
                return False
        # We didn't find the user in table and that means, user is subscribed. Create a user and return true.
        session.add(User(email=user_email, subscribed_to_emails = False))
        session.commit()
        return True

It does two different things:

  • check that a user exists and whether it is subscribed
  • add that user to a table, optionally

Its name is misleading because it does more than what the name might suggest. I recommend to split those two tasks.

Also, the wording is inconsistent. If the user is not subscribed, then the function should tell False, not True.

First of all, fetching a user record by primary key (ID, or in your case E-mail) can be done in a more straightforward way. Something like this should do:

with Session(engine) as session:
    result = session.query(User).filter(User.email == user_email).first()

This should get you just one row. And then you can look at the value of result.subscribed_to_emails right away. If you don't get any row, then add the user accordingly if needed.

You can either use .first() or .one(), depending on the intended results. Have a look here for some explanations and the official doc too. You should not be doing a loop here, when you are expecting zero or one row.

Basically, the subscription status is just one user property along with name, E-mail etc, so I don't think it warrants a function of its own.


This expression can be simplified since .subscribed_to_emails is already boolean:

if row.subscribed_to_emails == False:
    return True
else:
    return False

to:

return not row.subscribed_to_emails

Misc

I recommend to provide a fully qualified path for the database, instead of a relative path so there is no ambiguity:

db_uri = "sqlite:///temp.db"

That usually goes into a config file where you can define other constants for your application. Otherwise you might easily end with several DB files scattered across directories if you reorganize your application structure later on.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.