2
\$\begingroup\$

I have been trying to improve python combined with Postgres.

At this moment im only sending two values to database which are etc:

link = "https://www.mathem.se/varor/vegetarisk-fars/vegofars-fairtrade-1000g-anamma"
store = "mathem" 

and I do have few options that I want to be able to do with Postgres and that is to fetch all, commit and count.

And here I do have few options such as to check if the link exists already, if its has been deactivated from before, delete the link, get the ID from the product etc etc:

#!/usr/bin/python3
# -*- coding: utf-8 -*-
from datetime import datetime

import psycopg2

DATABASE_CONNECTION = {
    "host": "testing.com",
    "database": "test",
    "user": "test",
    "password": "test"
}


def execute_fetch_all(query):
    """
    Fetch all queries
    :param query:
    :return:
    """
    try:
        connection = psycopg2.connect(**DATABASE_CONNECTION)
        cursor = connection.cursor()
        cursor.execute(query)
        response = cursor.fetchall()
        cursor.close()
        connection.close()
        return response

    except (Exception, psycopg2.DatabaseError) as error:
        logger.exception(f"Database error: {error}")
        return


def execute_commit(query):
    """
    Commit database
    :param query:
    :return:
    """
    try:
        connection = psycopg2.connect(**DATABASE_CONNECTION)
        cursor = connection.cursor()
        cursor.execute(query)
        connection.commit()
        cursor.close()
        connection.close()
        return True

    except (Exception, psycopg2.DatabaseError) as error:
        logger.exception(f"Database error: {error}")
        return


def execute_count(query):
    """
    Count elements in database
    :param query:
    :return:
    """
    try:
        connection = psycopg2.connect(**DATABASE_CONNECTION)
        cursor = connection.cursor()
        cursor.execute(query)
        if cursor.rowcount:
            cursor.close()
            connection.close()
            return True
        else:
            cursor.close()
            connection.close()
            return False

    except (Exception, psycopg2.DatabaseError) as error:
        logger.exception(f"Database error: {error}")
        return


def check_if_link_exists(store, link):
    """
    Check if link exists
    :param store:
    :param link:
    :return:
    """
    if execute_count(f"SELECT DISTINCT link FROM public.store_items WHERE store='{store.lower()}' AND link='{link}';"):
        return True
    else:
        return False


def check_if_links_deactivated(store, link):
    """
    Check if link is deactivated
    :param store:
    :param link:
    :return:
    """
    if execute_count(f"SELECT DISTINCT link FROM public.store_items WHERE store='{store.lower()}' AND link='{link}' AND visible='no';"):
        return True
    else:
        return False

def delete_manual_links(store, link):
    """
    Delete given link
    :param store:
    :param link:
    :return:
    """
    execute_commit(f"DELETE FROM public.manual_urls WHERE store='{store.lower()}' AND link='{link}';")
    return True


def get_product_id(store, link):
    """
    Get id from database for specific link
    :param store:
    :param link:
    :return:
    """
    product = execute_fetch_all(f"SELECT DISTINCT id, store, link FROM public.store_items WHERE store='{store.lower()}' AND link='{link}' AND visible='yes';")
    return {"id": product[0][0], "store": product[0][1], "link": product[0][2]}

def get_all_links(store):
    """
    Return all links in database
    :param store:
    :return:
    """
    cur = execute_fetch_all(f"SELECT DISTINCT id, link FROM public.store_items WHERE store='{store.lower()}' AND visible='yes';")
    return [{"id": links[0], "link": links[1]} for links in cur]



def check_if_store_exists(store):
    """
    Check if the store exists in database
    :param store:
    :return:
    """
    if execute_count(f"SELECT DISTINCT store FROM public.store_config WHERE store='{store.lower()}';"):
        return True
    else:
        return False


def register_store(store):
    """
    Register the store
    :param store:
    :return:
    """
    if check_if_store_exists(store=store) is False:
        execute_commit(f"INSERT INTO public.store_config (store) VALUES ('{store.lower()}');")
        return True
    else:
        return False

I wonder if there is a way to maybe even short the code by alot or to also improve when using postgres combined with Python since it is new for me still but I do see potential that I might be able to shorter the code quite alot here

If there is any missing information, please let me know in comments and I will try my best to give the information that I might have forgot to add here

\$\endgroup\$
6
  • 1
    \$\begingroup\$ What about using an ORM like sqlalchemy? \$\endgroup\$
    – hjpotter92
    Nov 26 '20 at 13:11
  • \$\begingroup\$ Hi @hjpotter92 - Im actually not sure what is that? \$\endgroup\$ Nov 26 '20 at 13:15
  • 1
    \$\begingroup\$ sqlalchemy.org \$\endgroup\$
    – hjpotter92
    Nov 26 '20 at 13:23
  • \$\begingroup\$ @hjpotter92 - Hmm im not sure what that is, is it like a replacement of postgres? \$\endgroup\$ Nov 26 '20 at 13:28
  • \$\begingroup\$ @ProtractorNewbie ORM (like SQLAlchemy) is not a replacement, it is an abstraction layer over PostgreSQL or another db server \$\endgroup\$
    – n1k9
    Nov 26 '20 at 14:17
2
\$\begingroup\$

Injection

f"SELECT DISTINCT link FROM public.store_items WHERE store='{store.lower()}' AND link='{link}';"

is wide open to injection attacks. psycopg2 has excellent support for parameters in prepared statements; use that instead of string formatting.

Hard-coded credentials

DATABASE_CONNECTION needs to be externalized to somewhere secure in the operating system environment, for a handful of reasons including protecting the password, and keeping reconfiguration easy. This can take the form of environmental variables or a config file.

Context management

This block:

    connection = psycopg2.connect(**DATABASE_CONNECTION)
    cursor = connection.cursor()
    cursor.execute(query)
    response = cursor.fetchall()
    cursor.close()
    connection.close()

has a number of issues:

  • You're opening and closing a new connection every time you do a fetch. Connections are expensive and should be longer-lived.
  • You need to replace explicit close with the use of a context-management with statement.
  • Your except effectively swallows all exceptions and returns None if there's a failure. That behaviour should not be baked into execute_fetch_all and should exist outside, at the caller level instead. This is true for execute_count and execute_commit as well.

Explicit commit

execute_commit does not benefit from commit() in its current form. Read this for example - https://stackoverflow.com/questions/51880309/what-does-autocommit-mean-in-postgresql-and-psycopg2 - autocommit is the default.

Existence checks

SELECT DISTINCT

is not the best way to check for existence. Instead, consider use of the actual PostgreSQL exists clause.

Dictionary construction

Don't do this:

[{"id": links[0], "link": links[1]} for links in cur]

yourself.

Read https://www.psycopg.org/docs/extras.html#dictionary-like-cursor for the built-in alternative.

Booleans

if x is False:

should be

if not x:

and

if x:
    return True
else:
    return False

should be

return bool(x)

or, if you're comparing a count, more explicitly:

return x > 0
\$\endgroup\$
7
  • \$\begingroup\$ Hi! I forgot to mention that this script will only be locally PC and will not be uploaded anywhere in the web! So only me will be able to do anything with this script! :) - About the database connection, I was not sure what would be the best option, etc, if I run etc 100 scripts where each script has a import of database.py, Wouldn't that mean that all of them would be connected to the database at the same time and hit the maximium connection? Thats the reason I have a close() at this moment but might be wrong too! \$\endgroup\$ Nov 26 '20 at 18:34
  • 1
    \$\begingroup\$ this script will only be locally PC - For learning purposes, you should still strive for production-quality code. Get into good habits sooner rather than later. \$\endgroup\$
    – Reinderien
    Nov 26 '20 at 18:35
  • 1
    \$\begingroup\$ all of them would be connected to the database at the same time and hit the maximium connection - Sure; but that would still potentially happen even if you close your connection more often. It's still possible for there to be 100 concurrent calls to your fetch routine. The solution is not to close the connection more often; it's to centralize connection management into a smaller number of processes with connection pools. \$\endgroup\$
    – Reinderien
    Nov 26 '20 at 18:36
  • \$\begingroup\$ That is true. However im not sure how to solve that issue in that case where I can have a connection pool. What should it actually happend if you etc have 150 scripts where all of them are connected to the db, My idea and reason I did is that I just wanted to connect -> to my add/update/delete to the db -> close it. To never reach the connection issue. But I assume as you mentioned this is not the correct way and im not sure what else I can do from here code-wise :( \$\endgroup\$ Nov 26 '20 at 18:42
  • \$\begingroup\$ If you're running 150 scripts that all connect to the database at once, you have bigger problems. \$\endgroup\$
    – Reinderien
    Nov 26 '20 at 18:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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