A discord.py bot that logs every messages sent in a discord server

Question

Hello Code Review community !

I'm making a bot with discord.py that listens to every message sent in a server and stores it in a database that could be retrieved by admin/mod later.

Here's the structure of the database currently

guild_id = guild.id
statements = [
    f"""
    CREATE TABLE guild{guild_id}_members (
        id BIGSERIAL PRIMARY KEY NOT NULL,
        user_id bigint UNIQUE NOT NULL
    );
    """,
    f"""
    CREATE TABLE guild{guild_id}_messages (
        id BIGSERIAL PRIMARY KEY NOT NULL,
        member_id INTEGER NOT NULL,
        at_time TIMESTAMP NOT NULL,
        message VARCHAR(2000) NOT NULL,

        CONSTRAINT fk_memberid
            FOREIGN KEY(member_id)
            REFERENCES guild{guild_id}_members(id)
            ON UPDATE CASCADE
            ON DELETE CASCADE
    );
    """
    ]
sql_queries(self.conn, statements)

(sql_queries basically is just a function for executing each statement one after the other then commit)

Then i use on_message event from discord.py that executes this part of the code:

@commands.Cog.listener()
async def on_message(self, message):
    if message.content == '': #or message.author.id == self.logbot.user.id:
        return
    guild_id = message.guild.id
    author_id = message.author.id
    at_time = message.created_at
    statement = f"""
    INSERT INTO guild{guild_id}_members (user_id)
    VALUES ('{author_id}')
    ON CONFLICT DO NOTHING;
    """
    sql_query(self.conn, statement)
    statement = f"""
    SELECT id FROM guild{guild_id}_members
    WHERE user_id = '{author_id}';
    """
    member_id = int(sql_select(self.conn, statement)[0])
    print(f'{member_id}')
    
    statement = f"""
    INSERT INTO guild{guild_id}_messages (member_id, at_time, message)
    VALUES ('{member_id}', '{at_time}', '{message.content}')
    """
    sql_query(self.conn, statement)

To explain further, first i create a user identity in my guild{guild_id}_members (with unique constraint) then i select the id through a select query to use it to insert the message with the id i just got from the select query.

I know it's optimizable but i don't know how.

Thank you for reading me. :)

EDIT : Here's my sql_query and my sql_select functions : Sql query :

def sql_query(conn, statement):
    cur = conn.cursor()
    cur.execute(str(statement))
    conn.commit()

Sql Select :

def sql_select(conn, statement):
    cur = conn.cursor()
    cur.execute(str(statement))
    return cur.fetchone()

Answer 1

BIGSERIAL 

is a PostgreSQL extension and non-standard. Consider instead GENERATED ALWAYS AS IDENTITY.

Otherwise, the biggest problem with this code is that it's directly vulnerable to injection attacks. message is a discord.Message (you should typehint it as such) - I don't know how much validation it gets nor how robust it is, so the severity of this vulnerability is unclear.

In PsycoPG there is a feature for prepared statements that allows for parameters to be passed safely into queries; quoting the docs:

Never, never, NEVER use Python string concatenation (+) or string parameters interpolation (%) to pass variables to a SQL query string. Not even at gunpoint.

Rule number one of database security is - do not format your own query parameters into a string; tell the connector to do that for you. Your sql_query and sql_select are doing more harm than good, since they can only accept a static string.

Your second query can be avoided entirely by adding a RETURNING clause to your first query.

Also it seems like you have a dynamically-generated, separate table for every different "guild". This is unwise. Include the guild ID as a column on a single table instead. When this column is properly indexed, there will be no advantage to having separated tables.

Answer 2

Also i'm creating a table by guilds because each guild could have a giant amount of data in, why creating a table per guild is that bad ?

Well, first of all it adds complexity for no real benefit. The database design is poor. There should be a single table, guild_id should simply be an additional column, and very probably you will want to index it too (unless the table is going to contain a small number of messages, in which case an index may not improve performance). With proper indexing and optimizing the table size should not matter. Also do learn about primary keys.

In fact, that's the reason why you are doing dynamic SQL whereas you should be using prepared queries instead. So this is one reason why it's "bad". And in 2021 there is no excuse for writing code that is vulnerable to SQL injections.

I realize you have a steep learning curve ahead, but consider using an ORM, rather than doing plain SQL. For V2 perhaps.

When doing multiple, related inserts, use transactions for data integrity.

Avoid assumptions - if you are going to retrieve a row from a SELECT statement, make sure that the statement has succeeded and that it actually returns rows. In fact, you are expecting just one row, no more no less, anything else is an anomaly and should trigger an exception.

But this SELECT statement is nonsense, because you are trying to retrieve a bit of information that should already be available.

I am not familiar with psycopg but every database wrapper normally has a function to return the last row id, in Postgress this could be achieved with a RETURNING statement but please do your own research.

Example:

>>> cursor.execute("INSERT INTO users (username) VALUES ('hello') RETURNING id;")
>>> cursor.fetchone()
(11,)

Also have a look here for more details: https://zetcode.com/python/psycopg2/

It's possible and even likely that an ORM like SQL Alchemy already implements the feature for you, thus easing your development efforts.