Store a blank optional unique field as NULL, and answer a duplicate with 409

A site reported "internal server error" adding a second business unit. It was
reproducible: create one with a blank code, create another with a blank code,
500.

A column that is unique and nullable accepts any number of NULLs - that is what
makes "optional but unique" work - and exactly ONE empty string. The form sent
'', so the first blank code saved and every one after it collided with it. The
field showed no asterisk because it genuinely is optional; the database just
behaved as though it were not.

This is not specific to business units. A dozen columns across core and the
plugins are unique and nullable - asset numbers, hostnames, item codes, subnet
names, gage-lab tags - and each was one blank form away from the same 500.
Fixing them an endpoint at a time would have left the next to be found by a
user, so a before_flush listener normalises blank to NULL on any unique nullable
text column. Listening on Session rather than on individual mappers covers
plugin models imported later, and avoids mapper-event semantics that differ
between SQLAlchemy versions.

A genuine duplicate is now a 409 with a readable message rather than a bare 500
with a traceback in the log: reusing a code that is taken is the caller's
mistake, not a server fault.

Verified against the development database: three business units with blank codes
all save, the blank stores as NULL, and a real duplicate code returns 409.
This commit is contained in:
cproudlock
2026-08-05 13:16:23 -04:00
parent 85ff25462e
commit 705dd771bd
2 changed files with 112 additions and 0 deletions

View File

@@ -88,6 +88,12 @@ def create_app(config_name: str = None) -> Flask:
# Initialize extensions
init_extensions(app)
# An optional unique column accepts any number of NULLs but exactly one
# empty string, so a second blank code collided and became a 500. Normalise
# blank to NULL once, at the mapper, rather than in each endpoint.
from .utils.blankunique import register_blank_unique_normaliser
register_blank_unique_normaliser()
# Initialize plugin manager
with app.app_context():
plugin_manager.init_app(app, db)
@@ -186,6 +192,31 @@ def register_error_handlers(app: Flask):
from .utils.responses import error_response, ErrorCodes
from .exceptions import ShopDBException
# A uniqueness violation is the caller's problem, not a server fault. Without
# this it surfaced as a bare 500 with a SQLAlchemy traceback in the log and
# nothing usable on screen - the operator saw "internal server error" for
# having reused a code that was already taken.
from sqlalchemy.exc import IntegrityError
@app.errorhandler(IntegrityError)
def handle_integrity_error(error):
from .extensions import db
db.session.rollback()
message = str(getattr(error, 'orig', error))
app.logger.warning('integrity error: %s', message)
if 'Duplicate entry' in message or 'UNIQUE constraint' in message:
return error_response(
ErrorCodes.CONFLICT,
'That value is already in use. Codes and identifiers must be unique.',
http_code=409)
if 'foreign key constraint' in message.lower():
return error_response(
ErrorCodes.VALIDATION_ERROR,
'That record refers to something that does not exist, or is still in use elsewhere.',
http_code=400)
return error_response(ErrorCodes.VALIDATION_ERROR,
'The database rejected that change.', http_code=400)
@app.errorhandler(ShopDBException)
def handle_shopdb_exception(error):
http_codes = {

View File

@@ -0,0 +1,81 @@
"""Store an empty optional unique field as NULL, not as an empty string.
A column that is `unique=True` and nullable accepts any number of NULLs, which
is what makes "optional but unique" work at all. It accepts exactly ONE empty
string. So the first record saved with a blank code succeeds and the second
fails with "Duplicate entry '' for key", surfacing as a 500 from an endpoint
that did nothing wrong.
That is not a business-unit problem. A dozen columns across core and the plugins
are unique and nullable - asset numbers, host names, item codes, subnet names -
and every one of them is a blank web form away from the same collision. Fixing
them one endpoint at a time would leave the next one to be discovered by a user.
So it is fixed once, at the mapper: before any insert or update, a string column
that is unique and nullable and has been set to '' or whitespace is stored as
NULL instead. An empty string carries no information in a unique column - there
is no case where two records both meaning "blank" should collide - so nothing is
lost by normalising it.
Registered from create_app. Applies to every model, including plugin models
loaded later, because it hooks the shared Mapper class rather than a list of
models known at import time.
"""
from sqlalchemy import event, String, inspect
from sqlalchemy.orm import Session
_CACHE = {}
def _blankable_columns(mapper):
"""Unique, nullable, string columns on this mapper - worked out once each."""
cached = _CACHE.get(mapper)
if cached is not None:
return cached
columns = []
for prop in mapper.column_attrs:
for column in prop.columns:
if not column.unique or not column.nullable:
continue
# Only text. A unique nullable integer cannot be handed '' by a form
# without failing type coercion long before it reaches here.
if not isinstance(column.type, String):
continue
columns.append(prop.key)
break
_CACHE[mapper] = columns
return columns
def _normalise_instance(obj):
try:
mapper = inspect(obj).mapper
except Exception:
return
for key in _blankable_columns(mapper):
value = getattr(obj, key, None)
if isinstance(value, str) and not value.strip():
setattr(obj, key, None)
def _before_flush(session, _flush_context, _instances):
# Both new and modified: editing a record to clear its code has to become
# NULL for the same reason creating one with a blank code does.
for obj in list(session.new) + list(session.dirty):
_normalise_instance(obj)
def register_blank_unique_normaliser():
"""Hook every session flush, for every model including plugins'.
Listening on Session rather than on individual mappers means models imported
later - which is every plugin model - are covered without registration, and
it avoids depending on mapper-level event semantics that differ across
SQLAlchemy versions.
"""
if getattr(register_blank_unique_normaliser, '_installed', False):
return
event.listen(Session, 'before_flush', _before_flush)
register_blank_unique_normaliser._installed = True