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