From 705dd771bd4cab2fc2d690406f98fa96b6d32572 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Wed, 5 Aug 2026 13:16:23 -0400 Subject: [PATCH] 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. --- shopdb/__init__.py | 31 ++++++++++++++ shopdb/utils/blankunique.py | 81 +++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 shopdb/utils/blankunique.py diff --git a/shopdb/__init__.py b/shopdb/__init__.py index 66e24d0..c858280 100644 --- a/shopdb/__init__.py +++ b/shopdb/__init__.py @@ -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 = { diff --git a/shopdb/utils/blankunique.py b/shopdb/utils/blankunique.py new file mode 100644 index 0000000..cd16c9e --- /dev/null +++ b/shopdb/utils/blankunique.py @@ -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