CodeSOD: Caught a Mistake

Daniel recently started a new job. His first task was to fetch some data from the database and render it to the user. Easy enough, and there were already wrapper functions around the database to make it easy. He called execute_read, passed it a query, and checked the results.

There were no results. But the query definitely should have returned results. What was going on?

def execute_read(conn, query, params, only_one=False):

    result = None

    cursor = None

    try:

        start_time = time.time()

        cursor = conn.cursor()

        cursor.execute(query, params)



        if only_one:

            result = cursor.fetchone()

        else:

            result = cursor.fetchall()



        end_time = time.time()

        time_taken = end_time - start_time



        if env.is_production():

            if time_taken > 0.4:

                logger.critical("long query", query=query, time_taken=time_taken)

        else:

            if time_taken > 0.2:

                logger.warning("long query", query=query, time_taken=time_taken)



    except Exception as err:  # pragma: no cover

        logger.exception("execute_read exception", exception_msg=err, query=query)



    finally:

        logger.debug("execute_read debug", query=query, params=params, only_one=only_one)

        if not result:

            if only_one:

                result = {}

            else:

                result = []

        if cursor:

            cursor.close()



    return result

There are a lot of things I don't like about this function. The only_one parameter, for starters. Note how the database library actually breaks that behavior out as different functions- that's a much more appropriate model, especially since you have wildly different return types depending on how that flag is set.

Similarly, checking env.is_production() to check a timing threshold is itself pretty awful. I can sympathize with wanting different timing constraints based on what environment you're in- but if that's the case, the timing constraint is the parameter. env.long_query_threshold should be the configuration parameter. Also, your database should be able to alert you to these kinds of things, so that it doesn't live in your code anyway.

But the WTF here is the promiscuous exception handler, which catches all errors and simply logs them. This created a situation where Daniel sent a query to the database and got no results. He didn't go straight to the logs and tried to debug it more directly, so it took him quite some time to find the execute_read exception log line which told him what was wrong: his SQL query had a syntax error.

Daniel writes: "I can't imagine the disaster that this causes if there's a network hiccup in production." Failing silently and returning empty results sets definitely is inviting a lot of confusion.

[Advertisement]
Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.