705dd771bd
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.
82 lines
3.1 KiB
Python
82 lines
3.1 KiB
Python
"""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
|