DEV Community: Code Review Doctor

The JSON trick 25% of Python devs don't know about

Code Review Doctor — Wed, 09 Feb 2022 01:06:13 +0000

Reading and writing JSON files is a common chore in Python. In fact our analysis of 888 codebases found that 17% of repositories either read from or wrote to JSON files. Of those 150 codebases 25% were making their lives harder than necessary by not using a built-in JSON feature. A key part of our job as devs is to reduce the complexity of the code we write, so this is a good area of improvement for one in four Python devs.

At the end of this article is a link to the stats. You may want to check the stats. You never know, your repo might be one of the 888 we checked!

Many of the occurrences were in big open source projects, so this is not just a rookie mistake. For fun we listed some notable examples near the end of the article.

Reading JSON files

Have you ever wondered why json.loads ends with a "s"? The reason is because that method is specifically for working on a string, and there is another related method for working on files: json.load

json.loads is for deserialising a string containing JSON, while json.load (no "s") is for deserialising a file containing JSON.

The methods are very related. In fact, json.load is a wrapper around json.loads. It's a method provided out of the box by Python to simplify the task of reading JSON from file-like objects. For example, these result in the same outcome:

# bad
with open('some/path.json', 'r') as f:
    content = json.loads(f.read())

# good
with open('some/path.json', 'r') as f:
    content = json.load(f)

So why not choose the easiest to read and write?

Writing JSON files

Similarly to json.loads, json.dumps also ends with an "s" because it's specifically for working on a string and there is another related method: json.dump

json.dumps is for JSON serialising an object to a string, while json.dump (no "s") is for JSON serialising an object to a file.

Again similar to the relationship between json.load and json.loads, json.dump is a wrapper around json.dumps. It's a method provided out of the box by Python to simplify the task of writing JSON to a file-like object.

For example, these result in the same outcome:

# bad
with open('some/path.json', 'w') as f:
    f.write(json.dumps({'foo': 'bar'}))

# good
with open('some/path.json', 'w') as f:
    json.dump({'foo': 'bar'}, f)

So why not choose the easiest to read and write?

Reduce complexity

Python core developers are very careful about what features are included in the Python built-in modules because once they expose an API to devs it's very hard for them to remove it. So we've been given some tools that simplify our handling of JSON files so it makes sense to utilise the convenience methods and not reinvent the wheel. For that reason when Code Review Doctor sees devs not using json.load and json.dump this advice is given:

Examples

Many of the occurrences were in big open source projects, so this is not just a mistake rookies make, but rather is a often overlooked feature of Python:

Stats

You can read the gists:
Reading JSON: json.load vs json.loads
Writing JSON: json.dump vs json.dumps

How did we find this statistic? At CodeReview.doctor we run static analysis checks on codebases to find bugs and mistakes most humans miss. During this process we collect stats on the public repos. We tracked every line we could find where developers interacted with JSON files. We were surprised by the results!

If you would like to check if you're affected by issues like this one, check your codebase instantly for free at CodeReview.doctor.

11% of Python files are not closed! Affecting Google, Python.org, Ansible, Tensorflow ...

Code Review Doctor — Mon, 31 Jan 2022 20:30:51 +0000

It’s a problem that 11% of files are not closed properly as leaving files open degrades application performance and increases the risk of unexpected side effects.

It's not just a mistake that rookies make. Many of the offenders are big names:

Those are just 4 of the >500 examples we found in the 5409 open(…) calls we analyzed over 666 repositories.

How did we find this statistic? At CodeReview.doctor we run static analysis checks on codebases, and we collect stats on the public repos. We tracked every line we could find where developers opened files via open(...) but never closed them and compared that with every line we could find where developers "did it right” (either explicitly calling close() or implicitly via context manager usage).

At the end of this article is a link and embed of the stats.

The problem

To recap: leaving files open results in degradation of application performance and increases the risk of unexpected side effects.

Computers have finite resources, including limits on how many files can be simultaneously open. Therefore leaving files open consumes more finite file handles than necessary. Under extreme conditions this could exhaust the number of allowed open files causing an “IOError: Too many open files" exception to be raised. Additionally, leaving files open consumes finite memory for longer than is necessary. Therefore for performance of the application it is preferable to closing file after finished with them.

Python’s garbage collection will eventually close dangling unclosed files, but "eventually" should raise a red flag if the goal is to write predictable software. Indeed, Python3 and Cpython might implicitly close the file the instant they’re no longer being used, while others might not for a long time. Under some conditions it might not happen at all. When pairing this problem with the zen of Python's Explicit is better than implicit, it’s clear that it’s better to not leave files open.

Python can also be fuzzy around exactly when file changes are committed to disk: the data may not be written to disk until the file is closed:

# bad: maybe not written to disk yet as the file was not closed
open('foo.txt', 'w').write('foo')
# also bad: maybe not written to disk as the file was not closed
f = open('bar.txt', 'w')
f.write('bar')
# better: written to disk when the file closed
f = open('baz.txt', 'w')
f.write('baz')
f.close()
# good: written to disk when the file closed and code is neater
with open('qux.txt', 'w') as f:
    f.write('qux')

Windows (the Operating System) locks files opened by Python. This could mean leaving files open is not thread safe as one thread will lock the file preventing other threads from opening it — even if the other thread just wants to read from it.
That’s why we give this advice:

The stats

Here’s a link to the 4812 examples of "doing it right": all the files were closed either by explicitly calling close() or by opening the file using the context manager approach.

Here's a link to the 597 examples of not “doing it right”: the files were opened and read from but never closed.

Check your repo

Check your repo for tech debt like this at codereview.doctor, or even review your GitHub PRs.

5% of 666 Python repos had comma typo bugs (including V8, Sentry, Tensorflow, and PyTorch)

Code Review Doctor — Thu, 06 Jan 2022 19:22:25 +0000

We found that 5% of the 666 Python open source GitHub repositories had the following three comma-related bugs caused by typos:

Too many commas

Typo accidentally adding a comma to end of a value, turning it into a tuple

As far as the Python parser is concerned the parentheses are optional for non-empty tuples. According to the documentation: it is actually the comma which makes a tuple, not the parentheses. The parentheses are optional, except in the empty tuple case.

value = (1)  # evaluates to int
value = 1,  # evaluates to tuple with one element
value = (1,)  # evaluates to tuple with one element
value = 1, 2  # evaluates to tuple with two elements

By accidentally suffixing a comma on value = 1, expect to get a TypeError when the code later attempts to perform integer operations on a variable that evaluated to tuple.

Too few commas

Accidentally missed comma from string in list/tuple/set, resulting in unwanted string concatenation.

Implicit string concatenation that resulted from a typo can change the behaviour of the application. Take for example:

def is_positive(word):
    words = (
        'yes',
        'correct',
        'affirmative'
        'agreed',
    )
    return word in words

is_positive('agreed') is True  # evaluates to False

is_positive('agreed') evaluates to False because a typo resulted in the comma being missed from the end of 'affirmative', resulting in 'affirmative' and 'agreed' being implicitly concatenated to 'affirmativeagreed'.

Accidentally missed comma from 1 element tuple, making it a str instead of a tuple.

Detecting the bugs

We did not go through the repositories line by line. Instead we ran the repositories through our suite of static analysis checks which we run on AWS Lambda, taking a grand total of 90 seconds.

The main difficulty was reducing false positives. Syntactically there is no difference between a missing comma in a list and implicit string concatenation done on purpose. Indeed, when we first wrote the "probably missing a comma" checker we found that 95% of the problems were actually false positives: there are perfectly cromulent reasons a developer would do implicit string concatenation spanning multiple lines:

User agent string
file path
URL
CSV file contents
Very long message
HTML
Sha hash
SQL
JSON

After checking the 666 codebases we understood the key drivers of false positives and added allowances for implicit string concatenation for those types of data. After we gave allowances for these valid reasons to do implicit string concatenation the false positive rate went down to negligible non-annoying level. At this point we were left with a list of 24 repositories that had real bugs that slipped through code review process.

How common?

Overall we detected these bugs in 24 of the 666 repositories - and many of them were big open source projects. We raised the issues and helped fix over the course of a very busy day of managing tickets and PRs. Here's the most interesting ones:

5% of repositories had one of these three bugs, and most of them were "big" well known and well-used projects with many contributors and very robust code review processes. This highlights that for manual processes like code review to detect 100% of bugs 100% of the time then 100% of humans must perform 100% perfectly during code review 100% of the time. See the cognitive dissonance? We do code review because we expect human error when writing the code, but then expect no human error when reviewing the code. There are just some things a human can do well, and there are some things a bot can do better. Noticing commas in the wrong place is one of those things. For example:

Can you see the problem? Line 572 has implicit string concatenation!

It's missing a commas, so should be
'"()<>[]:,;@\\"!#$%&\'-/=?^_{}| ~.a"@example.org', '" "@example.org',

I'm not surprised that was missed by a human!

The impact

Some of the missing commas have little to no impact, but some are rather impactful. Take the Sentry one as an example:

Notice the missing comma in the list:

There is a comma missing between "releases" and "discover" resulting in the two being implicitly concatenated together to form "releasesdiscover".

The impact is the test requesting "/releasesdiscover" which does not exist, and so instead of "/releases" and "/discover" being tested, instead the 404 page is tested. That means for all we know, the organisation switcher on /releases and /discover could be broken. There is some poor dev out there maintaining the organisation switcher relying on this test and using it as a quality gate to give them confidence they have not broken the product but the test is not really working. It's these kind of things that can harm a nights sleep! It ain't what you don't know that gets you into trouble. It's what you know for sure that just ain't so.

Protecting your codebase

You can add Code Review Doctor to GitHub so fixes for problems like these are suggested right inside your PR.

How we found and helped fix 24 bugs in 24 hours (in Tensorflow, Sentry, V8, PyTorch, Hue, and more)

Code Review Doctor — Wed, 05 Jan 2022 10:36:33 +0000

Code Review Doctor is a static analysis tool integrated into GitHub. Normally it suggests fixes in PRs - but during integration testing of new checks we run the checks against 666 open source GitHub repositories to ensure the quality of the checks (including weeding out false positives before our users see them).

During the integration testing this week we found 24 bugs in big open source projects, which we raised and helped fix over the course of a very busy day of managing tickets and PRs. Here's the most interesting ones:

The checks

A common thread is all the issues relate to commas: either too many commas or too few commas:

Too many commas

Typo accidentally adding a comma to end of a value, turning it into a tuple

Too few commas

Accidentally missed comma from string in list/tuple/set, resulting in unwanted string concatenation.

Accidentally missed comma from 1 element tuple, making it a str instead of a tuple.

The main difficult bit with this check was reducing false positives. Syntactically there is no difference between a missing comma in a list and implicit string concatenation done on purpose. Indeed, when we first wrote the checker we found that 95% of the "problems" were actually false positives: there are perfectly cromulent reasons a developer would do implicit string concatenation spanning multiple lines:

User agent string
file path
URL
CSV file contents
Very long message
HTML
Sha hash
SQL
JSON

How common?

After running those three new checkers against the 666 repositories we found that almost 5% of repositories had one of these three bugs, and most of them were "big" well known and well-used projects with many contributors and very robust code review processes. This highlights that for manual processes like code review to detect 100% of bugs 100% of the time then 100% of humans must perform 100% perfectly during code review 100% of the time. See the cognitive dissonance? We do code review because we expect human error when writing the code, but then expect no human error when reviewing the code. There are just some things a human can do well, and there are some things a bot can do better. Noticing commas in the wrong place is one of those things. For example:

Can you see the problem? Line 572 has implicit string concatenation!

It's missing a commas, so should be
'"()<>[]:,;@\\"!#$%&\'-/=?^_{}| ~.a"@example.org', '" "@example.org',

I'm not surprised that was missed by a human!

The impact

Some of the missing commas have little to no impact, but some are rather impactful. Take the Sentry one as an example:

Notice the missing comma in the list:

There is a comma missing between "releases" and "discover" resulting in the two being implicitly concatenated together to form "releasesdiscover".

Protecting your codebase

Add Code Review Doctor to GitHub and avoid problems like these being merged as Code Review Doctor will automatically suggest the fix for these kind of issues right inside your PR.

Django Doctor is expanding...and rebranding!

Code Review Doctor — Wed, 15 Dec 2021 12:23:53 +0000

Django Doctor is a code analysis tool that suggests improvements to your Django code right inside your pull request.

We're expanding the scope of our services to also check Python code in general. As a result we're soon renaming to Code Review Doctor.

Sneak peak: CodeReview.doctor

Our prices remain unchanged, so our valued customers will get more value.

Don't abuse Django's DEBUG setting

Code Review Doctor — Tue, 12 Oct 2021 16:33:30 +0000

It's tempting to treat settings.DEBUG as a feature flag. Many devs see it as analogous to is the app not running in prod. However, using settings.DEBUG to change the flow of code adds complexities to testing and maintaining the codebase.

As a concrete example, let's say we want to activate Django admin only in local development to reduce the surface area for hackers to attack in prod. We could do this:

from django.conf import settings
from django.contrib import admin

urlpatterns = [...]

if settings.DEBUG:
    # not exposing admin to prod so it can't be hacked
    urlpatterns.append(path('admin/', admin.site.urls))

This will work, but the code will be harder to test in isolation because the value of settings.DEBUG changes other Django behaviours including error handling (should we see detailed crash report in browser? Should the error be emailed to admins?). Therefore instead of using settings.DEBUG in this way consider using a feature flag:

from django.conf import settings
from django.contrib import admin

urlpatterns = [...]

if settings.IS_ADMIN_ENABLED:
    # not exposing admin to prod so it can't be hacked
    urlpatterns.append(path('admin/', admin.site.urls))

The reason for this complexity is the fact settings.DEBUG is for switching whether Django should run in debug mode. Indeed it's for switching behaviour of the framework and framework libraries, not application code. Using settings.DEBUG for also controlling the behaviour of the application code built on top of the framework makes the codebase harder to maintain and test. Mixing the layers and responsibilities like this adds complexity.

For example, DEBUG=True shows detailed error pages for use during local development, while DEBUG=False can result in the error instead being emailed to settings.ADMINS. Do you want to cause either of those behavioural changes while testing your code? Probably not, as this is inconvenient and also goes against the principle of least surprise.

Aside from the complexities in testing, Twelve factor App suggests aiming for dev/prod parity. Using settings.DEBUG to control which block of code the application runs prevents dev/prod parity because most developers will not test or run their local environment with DEBUG=False. To solve this instead consider adding a feature flag specifically for this one check, which you can switch easily during unit tests and with none of the other side effects inherent with settings.DEBUG.

If we spot this issue in your GitHub pull request we give this advice:

Feature flag implimentation

12 factor app suggests feature flags can be switched on or off using environment variables. You could install a library to handle the parsing of environment variables or do something like:

# settings.py
from ast import iteral_eval
from os import getenv

IS_ADMIN_ENABLED = literal_eval(getenv('IS_ADMIN_ENABLED', 'False'))

ast.iteral_eval is that helpful function you always needed but never knew was provided out of the box by Python. It can convert string "False" to boolean False and string "True" to boolean True.

Does your Django codebase have tech debt like this?

Django Doctor can find and fix over 40 types of common Django tech debt, and can even review your GitHub Pull Requests for you.

The Django feature that can slow your website to a crawl

Code Review Doctor — Sun, 10 Oct 2021 23:29:15 +0000

Ordering Django querysets via order_by causes Django to ask the database to order the rows by a given column.

As long as the number of rows being ordered is not too large then this operation should be quick. However, there is a situation where using the build-in order_by can trigger a series of very expensive and slow operations: getting a random row from the database.

Read one random row inefficiently

Django's build in way to get a random record is via order_by('?')[0]. Note though that the Django docs themselves give this warning:

order_by('?') queries may be expensive and slow

The reason for the poor performance is the fact Django generates the following inefficient SQL:

SELECT * FROM entry ORDER BY RAND() LIMIT 1

Let's break that SQL down:

SELECT * FROM entry

This tells the database to include all columns from the entry table.

ORDER BY RAND()

This tells the database to generate a random number for every row (depending on database used. Some do it differently).

LIMIT 1

This tell the database to scan the generated numbers for the smallest one. This rows is then read.

Let that sink in: if we have 100,000 rows then 100,000 random numbers are first be generated, then all 100,000 are scanned for the smallest one. Generating random numbers is slow, and scanning is not quick. For perspective this whole operation smells a lot like mining Bitcoin but ultimately all we get out of it is we read one row from the database! A lot of work must be done for small pay-off.

If there are a lot of rows in the database and order_by('?')[0] is used then expect bad performance.

Read one random row efficiently

In short, databases are not good at random. Applications are though. So consider splitting responsibilities between the database and the application:

At database level determine the number of rows
At application level get a random number between 0 and that count.
At database level select the row at that index.

This will mean two database reads are performed, but those will are very efficient operations and so significantly quicker than the database copying the table and ordering all the rows randomly.

With that in mind, that is why Django Doctor gives this advice when reviewing Pull Requests and it spots order_by('?')[0] being used:

Self-contained

Consider improving the layering of the solution by moving the logic to a custom model manager like so:

from random import randint

from django.db import models, transaction


class RandomManager(models.Manager):

    @transaction.atomic
    def random(self):
        index = randint(0, self.count()-1)
        return self.all()[index]

class Entry(models.Model):
     objects = RandomManager()

random_item = Entry.objects.random()

Notice the use of transaction.actomic to make both the count and the row read happen in the same transaction, preventing a rare race conditions occurring if:

the last row in the table was deleted after the count was read but before the random row was read.
the random number generated was the the last row in the table.

Without a atomic transaction under very rare circumstances an IndexError could occur.

Does your Django codebase have tech debt like this?

Django Doctor can find and fix over 40 types of common Django tech debt, and can even review your GitHub Pull Requests for you.

GUI for Python using React and distributed with pip

Code Review Doctor — Thu, 20 May 2021 20:01:06 +0000

Django Doctor analyses codebases and suggests improvements. It used to be SaaS only, but today we released it as an offline command line tool. This blog post explains how we used React to make a nice UI for our Python command line interface:

For our command line tool we wanted a nice UI. We use React for our website so we considered how to pass data from Python to React and back again, in a way that can easily be distributed via pip install django-doctor. We didn't use Django or Flask as we wanted an apple, not a gorilla holding an apple.

The code

The following React Component is a form wizard that accepts a list of items and allows the user to choose a subset, then that subset of items is posted to the server:

// App.jsx
export default function({ messages }) {
  const [httpState, setHttpState] = React.useState({
    isInProgress: false,
    isComplete: false,
  })

  function handleSave(selectedMessages) {
    setHttpState({isInProgress: true, isComplete: false })
    fetch('/done/', {
      method: "POST",
      headers: {"Content-Type": "application/json"},
      body: JSON.stringify(selectedMessages)
    }).then(result => {
      setHttpState({isInProgress: false, isComplete: true })
    }).catch(result => {
      setHttpState({isInProgress: false, isComplete: false })
    })
  }
  if (httpState.isComplete) {
    return <Done />
  } else {
  return (
    <Wizard
      handleSave={handleSave}
      messages={messages}
      httpState={httpState}
    />
}

We can pass into the component some data provided by a Python script by doing the following:

// index.js
import React from 'react';
import { render } from "react-dom";

import './App.css';
import './App.jsx';


const contextElement = document.getElementById('context-messages')
const messages = JSON.parse(contextElement.textContent)
render(<App messages={messages} />, rootElement);

So index.js expects the HTML page it's being served from to contain an element with ID context-messages to contain some JSON serialized data. Now is where the Python comes in. We serve the HTML file using features provided by Python's build in wsgiref library:

# wsgi.py

import json
import mimetypes
import pathlib
import threading
from wsgiref.simple_server import make_server
from wsgiref.util import FileWrapper

# a folder containing the built React app, which we trick python into working with by adding an __init__.py to it
import django_doctor.wizard


static_dir = pathlib.Path(django_doctor.wizard.__path__[0])

with open(static_dir / 'index.html', 'r') as f:
    home_template_body = f.read()


def home_handler(environ, respond, app):
    # exposing data to the HTML template using an approach inspired by https://docs.djangoproject.com/en/3.1/ref/templates/builtins/#json-script
    messages = json.dumps(app.messages)
    body = home_template_body.replace(
        '<head>',
        f'<head><script id="context-messages" type="application/json">{messages}</script>'
    )
    body = response_body.encode('utf-8')
    respond('200 OK', [('Content-Type', 'text/html'), ('Content-Length', str(len(body)))])
    return [body]


def static_handler(environ, respond, app):
    # serve the css/js/png/etc files
    content_type = mimetypes.guess_type(environ['PATH_INFO'])[0]
    path = static_dir / environ['PATH_INFO'][1:]
    respond('200 OK', [('Content-Type', content_type)])
    return FileWrapper(open(path, "rb"))


def submit_handler(environ, respond, app):
    body_size = int(environ.get('CONTENT_LENGTH', 0))
    request_body = environ['wsgi.input'].read(body_size)
    selected_messages = json.loads(request_body)
    # TODO: do something with selected_messages
    respond('200 OK', [('Content-Type', 'text/plain')])
    # make the server kill itself after the response is sent
    threading.Timer(0.5, app.server.shutdown).start()
    return [b'']


class Application:
    def __init__(self, messages):
        self.messages = messages

    def __call__(self, environ, respond):
        if environ.get('PATH_INFO') == '/':
            return home_handler(environ=environ, respond=respond, app=self)
        elif environ.get('PATH_INFO') == '/done/':
            return submit_handler(environ=environ, respond=respond, app=self)
        elif environ.get('PATH_INFO').startwith('/static/'):
            return static_handler(environ=environ, respond=respond)



def create(messages):
    app = Application(messages=messages)
    server = make_server(host='localhost', port='9000', app=app)
    app.server = server
    return server

Then we can create some command line tool that calls wsgi.create:

import argparse

from django_doctor import check_codebase, wsgi


parser = argparse.ArgumentParser(prog='Django Doctor')

parser.add_argument('-d', '--directory', default='.')


def handle(argv=sys.argv[1:]):
    options = parser.parse_args(argv)
    messages = check_codebase(project_root=options.directory)
    wsgi.create(messages=messages)

So now we have bi-directional communication with react and python:

A python command line script that runs check_codebase then passes messages to the wsgi app
A wsgi app that renders a HTML file containing messages, and (not shown) a <script> tag that serves the build react js
A React app that hydrates the json and then passes it to form wizard, then ultimately posts the selected items back to /done/.
a wsgi handler that sees data posted to /done/ and does soemthing with it

Pretty cool. To make it cooler we can replace the http post request and rendering of html with a websocket. Less hacky. Maybe we will eventually use that at Django Doctor.

distributing via pip install

setup.py is great at distributing Python files, but for this to work we need to make setup.py create a distribution containing Python files and .js and .png and .html etc.

We do that by copying the build react app to ./wizard, add __init__.py to it, then write the setup.py like so:

setup(
    name="django_doctor",
    url="https://django.doctor",
    packages=find_packages(include=['django_doctor.wizard',]),
    include_package_data=True,
    classifiers=[
        "Development Status :: 5 - Production/Stable",
        "Environment :: Web Environment",
        "Framework :: Django",
        "Intended Audience :: Developers",
        "Natural Language :: English",
        "Operating System :: OS Independent",
        "Programming Language :: Python",
        "Topic :: Software Development :: Libraries :: Python Modules",
    ],
)

The meat is in packages - making sure wizard package is included, and include_package_data to make sure the non python files are distributed too.

Does your Django codebase have room for improvement?

Use our command line interface tool to check. pip install django-doctor then django_doctor fix.

Why we made a Django release notes comparison tool

Code Review Doctor — Tue, 16 Feb 2021 14:45:27 +0000

Django release notes are great. Very detailed. However, when updating across multiple releases such as 3.1.3 to 3.1.6 one would need to open the release notes for all 3 releases to get an idea of the changes:

That's why we made the Django release notes comparison productivity tool:

At a glance we can see the total changes.

Parsed to JSON

To achieve this we parsed Django's release notes to JSON.

An advantage of this functionality is the data is nice and machine readable. During the parsing we pull out metadata such as how many Django bugs are there in a particular Django Release:

This gives some context when we see a new Django version is available: do we need to upgrade now or can it wait.

Have a look: https://django.doctor/compare-django-release-notes/3.0.0/3.1.6

Fixing Django anti-patterns in Sentry

Code Review Doctor — Tue, 19 Jan 2021 09:32:21 +0000

Django Doctor audits and auto fixes Django anti-patterns. We audited the Sentry codebase for problems hindering maintainability to see how we can help, and ultimately created a Pull Request to fix some of them. In this post we will break down the most impactful issues, and how they can avoided.

Missing reverse migration

Django data migrations have two aspects: forwards and backwards. If a developer wants to undo migrations then backwards must be specified. However, many projects have migrations that miss this because by default the skeleton data migration generated by Django only shows the forwards handler, and hence like 22% of the Django codebases we audited, Sentry has some data migrations that do not support undoing the migration.

Unfortunately it only takes one missing reverse to make all migrations fail when attempting to undo the migrations. This does not leave the developer many options if they want to reverse migrations due to discovering a bug in prod that requires rolling back to the last good release.

Thus in all the affected data migrations in the Sentry Pull Request we suggested the following:

These migration changes will allow a developer to migrate backwards without Django throwing an exception, and one day may save a developer great headache when they are trying to recovery from a disaster in prod that requires rolling back and undoing a migration.

Checking queryset truthiness:

Querysets are lazily evaluated - meaning the records are not read from the database until code interact with the data. That's why we can do queryset.all() without downloading every records from the database.

However, if some code does if queryset then the queryset is evaluated immediately: all records in the queryset are read from the database. Perhaps tens, perhaps thousands, perhaps more. Checking if a queryset is truthy/falsey is much less efficient than checking queryset.exists().

Like 50% of the codebases we checked, Sentry suffers this anti-pattern. However, the fix is simple once the problem is identifier: in the Sentry Pull Request we suggested the following:

Also noteworthy is there were cases when the queryset was being evaluated for performance gains: it's perfectly valid to evaluate the queryset if the data in the queryset will be consumed by some code later on: no need to perform two database calls then one can suffice.

URL anti-patterns

Like 40% of the Django projects we checked, Sentry have some URL related anti-patterns:

duplicate URL names read more
hard-coded URLs in templates instead of using url template tag read more
hard-coded static asset URLs instead of using the static template tag read more

Duplicate URL name:

Django provides a very convenient method of generating URLs: giving each URL a name and then allowing the developer to use the {% url template tag or reverse to generate the URL.

But what happens if two URLs share the same name? For example here and here both have the name 'sentry-project-event-redirect'

It will mean half of the time templates will be linking to the wrong place: if a template tries to link to the login view via {% url "sentry-project-event-redirect" can you be sure it's going to the right place? No.

How did this happen? Well it's easily missed when reading the code. The urls.py is over 600 lines of very dense code. Easy to not see the needle in the haystack without tools to help.

Hard-coded URL

Hard-coding URLs in templates makes changing URLs harder, and makes linking to the wrong page easier - so harms maintainability. Sentry had one case of hard-coding a URL, which is very good: only one case in the huge codebase and that only was was pointing to the homepage, which is unlikely to change.

Hard-coded static asset URLs

Django documentation suggests not hard-coding static URLs, and instead using the {% static template tag. {% static ... returns the path the browser can use to request the file. At it's simplest, that would return a path that looks up the file on your local file system. That's fine for your local dev environment but in prod we will likely use third-party libraries such as whitenoise or django-storages to improve performance of the production web server.

We saw three instances of hard-coded static URLs, all of them in the same file. These hinder maintainability in a few ways. The key one is if the STORAGE_BACKEND is changed to one optimizing for speed of static file delivery then the following can happen:

File revving can change the filename
Serving from S3 (or similar) can change the domain the file is served from

Therefore hard-coding the static URL would break if the STORAGE_BACKEND was changed to e.g, whitenoise or django-storages

This issue is discussed on greater detail here.

Does your Django codebase have these issues?

Over time it's easy for tech debt to slip into your codebase. I can check that for you at django.doctor, or can review your GitHub PRs:

Or try out Django refactor challenge.

53% of Django projects have redundant code

Code Review Doctor — Thu, 14 Jan 2021 17:36:46 +0000

Django Doctor audits code and auto fixes Django anti-patterns. We checked 666 Django projects for problems hindering maintainability and found that 53% of the Django projects had redundant settings and model field kwargs

33% of settings.py had redundant values read more
29% had fields in models.py with redundant kwargs read more

There were some intersections - so some projects fell into more than one camp.

By "redundant" we mean the values did not need to be explicitly stated because they're only specifying the value Django would have used by default. Like a driving instructor saying "instead of doing 20mph, please do 20mph." This redundancy makes it hard to spot the signal in the noise - so it's harder for developers to zone in on the important code.

But is redundancy noise? Probably, and in the sample we checked it looks like the redundancy was not on purpose. Some justify it with the Zen of Python "Explicit is better than implicit" - but that does not seem to be the reason why 33% over half of projects did this, because if this was the motivation then shouldn't they be explicitly defining all the Django defaults and all the default field kwargs, not just one or two? Additionally, the Zen of Python also suggests:

simple is better than complex
beautiful is better than ugly.
readability counts
sparse is better than dense

So the the reason for having redundant settings seems to often be:

a result of copying and pasting settings from other projects; or
historically non-default values were later updated, and instead of deleting and using the Django default the dev simply changed True to False.
Things slip though the net during code review. Most people do not know every single Django default setting value as they change over the many versions of Django.

How would you reduce noise? Try our Django models.py refactor challenge.

1. Redundant settings

Django ships with default settings. When django.conf.settings is imported we're not getting a module, we're getting an instance of LazySettings. This object is a wrapper around a "holder" of the actual settings. During the normal running of Django the holder first uses the default settings then ingests the values in the file defined by the environment variable DJANGO_SETTINGS_MODULE.

So explicitly setting Django's default values in settings.py does not change how Django operates, but it is additional lines of code the developers must read when maintaining the code. The cost of each extra line is non-zero.

An implication of this is when we define values in our settings.py that are already defined in Django's default settings then the action is idempotent. To Django, the value may as well not be there.

It's easy for such things to slip through the net when a human is reviewing the code because this is a nice example of "searching for a needle in a hackstack", but Django Doctor is a code review bot so cannot miss it:

2. Redundant field kwargs

Similar to Django's default settings, Django's model fields have default kwargs that can be overridden when models area defined. Most of the points highlighted in the previous section are valid here so no need to cover them again.

29% of the projects we checked had explicitly defined kwargs, with null=False being the overwhelming most common one, but null is False by default.

So instead of

class SomeModel(models.Model):
    some_field = models.CharField(blank=False, null=False)

We can simplify things and do:

class SomeModel(models.Model):
    some_field = models.CharField()

Models are often dense enough even without redundant kwargs. Less dense models will improve the readability of the file and make developers' lives that little bit better.

So like Don't Repeat Yourself (DRY), we can say Don't Repeat Django (DRD).

Could your Django be simplified?

Half of Django codebases can be simplified, and it’s easy to miss something during code review. I can check that for you at django.doctor, or can review your GitHub PRs:

Or try out Django challenges.

48% of Django projects could simplify models.py in these 3 ways

Code Review Doctor — Tue, 12 Jan 2021 16:12:45 +0000

Django Doctor audits code and auto fixes Django anti-patterns. We checked 666 Django projects for problems hindering maintainability and found that 48% of the Django projects could simplify their models.py

22% used Charfield with huge max_length when a TextField would be better read more
7% used deprecated NullBooleanField read more
40% had string fields that allowed null but not blank or vice versa read more

There were some intersections - so some projects fell into more than one camp. Note there are valid usecases for null and blank differing, and we go through that in depth later.

How would you simply Models? Try our Django models.py refactor challenge.

1. CharField with huge max_length

When a user needs to enter a string that may be very long then it's quick and easy to use CharField(max_length=5001), but that has some problems:

5001 is big, but is it big enough? What if the a user wants more? Yes max_length can be increased, but bug fixes are a pain, as is the database migration to facilitate the change.
Years from now a developer dusts off your code and reads the number 5001. Do they infer there's something special about 5001? Maybe. Devs in the future will be very busy with complicated future things so let's not add ambiguity when maintaining your "old" code.

TextField is better here as there's really no need for a length check, so users will not be presented with a validation error. A nice rule of thumb cane be if the field does not need minimum length check then it probably does not need a maximum length check. For that reason when I see this happening I suggest using TextField:

So why use CharField if TextField is so great? Historically efficient database space usage was a key consideration. But now storage is practically free. Plus, for Postgres at least, using TextField has the same performance as CharField, so database storage performance is not a key consideration.

There are valid cases for CharField with a huge length though: just like an ISBN is always 10 or 13 characters, there are some very long codes. Storing QR codes? CharField. Working with geometry and geo spacial? CharField. Django GIS has a 2048 long VARCHAR that Django represents as a CharField(max_length=2048).

2. Deprecated NullBooleanField

For four years the documentation for NullBooleanField was two sentences and one of was "yeah…don't use it". As of 3.1 the axe has fallen and the hint of future deprecation has been replaced with an actual deprecation warning. Instead of using NullBooleanField() use BooleanField(null=True).

For that reason when I see NullBooleanField I suggest using BooleanField(null=True):

On the face of it, the existence of NullBooleanField seems odd. Why have an entire class that can be achieved with a null keyword argument? We don't see NullCharField or NullDateField. Indeed, for those Django expects us to do CharField(null=True) and DateField(null=True). So what's was so special about NullBooleanField and why is it now deprecated?

Enter NullBooleanField

NullBooleanField renders a NullBooleanSelect widget which is a <select> containing options "Unknown" (None), "Yes" (True) and "No" (False). The implication is NullBooleanField was intended for when explicitly stating no answer is known yet. Indeed, in many contexts it would be useful to clarify "is it False because the user has set it False, or because the user has not yet answered?". To facilitate that, the database column must allow NULL (None at Python level).

Unfortunately time has shown a great deal of room for confusion: StackOverflow has many questions that are answered with "use NullBooleanField instead of BooleanField" and vice versa. If one of the reasons for separating BooleanField and NullBooleanField was to give clarity then instead the opposite occurred for many.

Exit NullBooleanField

Until Django 2.1 in 2018, null was not permitted in BooleanField because (obviously) None is not in a bool value. Why would we expect None to be used in a field that says it's for boolean values only? On the other hand None is not a str either but CharField(null=True) was supported and None is not an int, but IntegerField(null=True) was also acceptable.

So in the deprecation of NullBooleanField there is an argument for consistency with how the other fields handle null. If we're aiming for consistency the choice is to either add NullCharField, NullIntegerField, NullDateField and so on or to rename NullBooleanField to BooleanField and call it a day, even though NullBooleanField was a more accurate name.

With this deprecation three classes are impacted:

django.models.fields.NullBooleanField
django.forms.fields.BooleanField
django.forms.widgets.NullBooleanSelect

These three have slightly different handling of "empty" values, so for some the swap from NullBooleanField to BooleanField will need some careful testing:

from django.forms.fields import NullBooleanField

field = NullBooleanField()

assert field.clean("True") is True
assert field.clean("") is None
assert field.clean(False) is False

from django.forms.fields import BooleanField

field = BooleanField(required=False)

assert field.clean(True) is True
assert field.clean("") is False
assert field.clean(False) is False

from django.db.models import fields

field = fields.BooleanField(null=True, blank=True)

assert field.clean(True, "test") is True
assert field.clean("", "test") is None
assert field.clean(False, "test") is False

3. Null and blank out of sync

Expect the unexpected if null and blank are different values: null controls if the the database level validation allows no value for the field, while blank controls if the application level validation allows no value for the field.

If blank=True then the field model validation allows an empty value such as "" to be inputted by users. If blank=False then the validation will prevent empty values being inputted.

On the other hands, null informs the database if the database column for the field can be left empty, resulting in the database setting either NULL or NOT NULL on the column. If the database encounters an empty NOT NULL column then it will raise an IntegrityError.

blank is used during during field validation. Form, ModelForm, and ModelSerializer each trigger field level validation. For a concrete example, ModelForm calls the model instance's full_clean method during form validation, and full_clean then calls clean_fields, which in turn may raise a ValidationError.

For that reason when I see this happening I suggest the following:

So normally do we want null and blank to the same value? When would we want to have null=False and blank=True or even null=True and blank=False?

null=False, blank=True

This facilitates using sensible default values for string fields: the field may have a default value like name = CharField(null=False, blank=True, default=""). This is useful if the field is optional, but we also want to prevent the database column from having inconsistent data types. Sometimes being None, sometimes being "", and other times being a non-empty string causes extra complexity in code and in ORM: if we wanted to find all users with no name:

Foo.objects.filter(name="") | Foo.objects.filter(name__isnull=True)

Compare that with the case for when the value in the database column will always be a string:

Foo.objects.filter(name="")

null=True, blank=False

This scenario is more to keep the database happy. If using the django.db.backends.oracle database engine then this may be needed because Oracle forces empty strings to NULL, even if an empty string was submitted in the form, so name = CharField(null=True, blank=False) would be needed.

Zero downtime deployment strategies may required NULL on the database column, even though business requirements dictate the user must enter a value in the form. During blue/green deployments both the new codebase and the old codebase run against the same database at the same. If the new codebase adds a new fields and there is no sensible default value for it then null=True is needed to avoid the database throwing an IntegrityError while the instance of your website running the old codebase interacts with the database.

While the database column can accept null, form validation can prevent the end users inputting no value, so data type consistency is assured? No - this required the form validation to actually run. If a developer is creating or updating via the shell then the validation will not run unless the developer calls instance.full_clean() or instance.clean_fields(). This strategy is simplified if a sane default value can be used instead of setting null=True.

Could your models.py be simplified?

I can check that for you at django.doctor, or can review your GitHub PRs:

Or try out Django refactor challenges.