Refuse a value the database cannot store, instead of quietly changing it
Wave 2 of the remediation plan. Eight findings from the feature test run were the same defect twice: a write that reports success while storing something other than what was sent. The first half is a date parser that swallowed its own error. Two plugins carried an identical private _parse_date returning None on ValueError, so an unparseable date and a deliberately cleared one produced the same write. PUT /api/warranty/<id> with enddate '30-06-2027' answered 200 "Warranty updated" and stored NULL, dropping the record out of every expiry surface on the strength of a typo; the measuring-tool calibration date did the same and flipped the tool to 'unknown', out of the calibration report and the map overlay. shopdb/utils/datevalues.py now holds one strict parser on the contract surface: an explicitly empty value clears the field, anything that is not a date is refused with a 400 naming the field, and those are different intents with different answers. The vendor-lookup paths keep a lenient wrapper on purpose - a provider changing its date shape must not fail the refresh of an otherwise good record, and no person typed that value. The second half is why the truncation findings were possible at all. MySQL was running non-strict: sql_mode on the 5.6 server is NO_ENGINE_SUBSTITUTION with no STRICT_TRANS_TABLES, so it accepted writes it could not store and adjusted them. A 257-character username became its first 100 characters (and the next distinct one then collided with it); an over-long asset number was cut at 50; a malformed releasedate became '0000-00-00' and permanently broke every read of that application. All with a 201. The mode is now set on the CONNECTION rather than left to the server, so a site whose my.cnf disagrees still gets it. That trades silent corruption for a DataError, which uncaught is a 500 - and a 500 says the server is broken when the truth is that the value does not fit. An error handler maps it to a 400 naming the column where MySQL names one. Verified by hand against MySQL 5.6: POST /api/users with a 257-character username now answers 400 "That value is too long for username." and stores nothing, where it previously stored a truncated row and answered 201. Twelve tests. The date guard is verified by planting: restore the swallowing parser and four of them fail. The DataError tests raise the error directly because the suite runs on SQLite, which does not enforce VARCHAR length at all - there is no value that would trigger it there, which is precisely why this went unnoticed. Scope: product
This commit is contained in:
99
tests/test_core/test_strict_write_errors.py
Normal file
99
tests/test_core/test_strict_write_errors.py
Normal file
@@ -0,0 +1,99 @@
|
||||
"""A write the database refuses is the CALLER's error, not a server fault.
|
||||
|
||||
The engine now runs with STRICT_TRANS_TABLES (see
|
||||
shopdb/config.py: SQLALCHEMY_ENGINE_OPTIONS). Before that, MySQL silently
|
||||
adjusted a write it could not store: a 257-character username became the first
|
||||
100 characters, an unparseable date became '0000-00-00' and then broke every
|
||||
read of that row, an over-long asset number was cut at 50 and collided with a
|
||||
different one. All of them answered 200 or 201, because nothing in the stack
|
||||
knew the value had changed.
|
||||
|
||||
Strict mode turns those into errors, which is the point - but an uncaught
|
||||
DataError is a 500, and a 500 says "the server is broken" when the truth is
|
||||
"that value does not fit". This pins the mapping.
|
||||
|
||||
These tests raise the error directly rather than through a column, because the
|
||||
suite runs on SQLite, which does not enforce VARCHAR length at all: there is no
|
||||
value that would trigger it here. The behaviour against the real engine was
|
||||
checked by hand on MySQL 5.6 - `POST /api/users` with a 257-character username
|
||||
returns 400 "That value is too long for username." and stores nothing, where it
|
||||
previously stored a truncated row.
|
||||
"""
|
||||
import pytest
|
||||
from sqlalchemy.exc import DataError
|
||||
|
||||
from shopdb import _column_from_data_error
|
||||
|
||||
|
||||
# A private app, because routes can only be mounted before the first request
|
||||
# and the session-wide one has already served plenty.
|
||||
@pytest.fixture(scope='module')
|
||||
def raising_app():
|
||||
from shopdb import create_app
|
||||
|
||||
application = create_app('testing')
|
||||
application.add_url_rule(
|
||||
'/__test__/raise', 'raise_data_error',
|
||||
lambda: (_ for _ in ()).throw(
|
||||
_data_error(application.config['TEST_DATA_ERROR_MESSAGE'])))
|
||||
return application
|
||||
|
||||
|
||||
def _raise(app, message):
|
||||
"""Answer a request whose route raises DataError, so the app's own handler
|
||||
is what produces the response."""
|
||||
app.config['TEST_DATA_ERROR_MESSAGE'] = message
|
||||
return app.test_client().get('/__test__/raise')
|
||||
|
||||
|
||||
def _data_error(message):
|
||||
return DataError('INSERT INTO x', {}, Exception(message))
|
||||
|
||||
|
||||
def test_a_value_too_long_is_a_400_naming_the_column(raising_app):
|
||||
response = _raise(
|
||||
raising_app,
|
||||
"(1406, \"Data too long for column 'username' at row 1\")")
|
||||
assert response.status_code == 400
|
||||
body = response.get_json()
|
||||
assert body['data']['error']['code'] == 'VALIDATION_ERROR'
|
||||
assert 'too long' in body['data']['error']['message']
|
||||
assert 'username' in body['data']['error']['message']
|
||||
|
||||
|
||||
def test_a_bad_date_is_a_400_naming_the_column(raising_app):
|
||||
response = _raise(
|
||||
raising_app,
|
||||
"(1292, \"Incorrect date value: 'not-a-date' for column "
|
||||
"'releasedate' at row 1\")")
|
||||
assert response.status_code == 400
|
||||
assert 'releasedate' in response.get_json()['data']['error']['message']
|
||||
|
||||
|
||||
def test_an_unrecognised_data_error_is_still_a_400_not_a_500(raising_app):
|
||||
"""The point is the CLASS of answer. A message shape nobody anticipated
|
||||
must not fall back to 'internal server error'."""
|
||||
response = _raise(raising_app, '(9999, "something new")')
|
||||
assert response.status_code == 400
|
||||
assert response.get_json()['data']['error']['code'] == 'VALIDATION_ERROR'
|
||||
|
||||
|
||||
@pytest.mark.parametrize('message,expected', [
|
||||
("Data too long for column 'assetnumber' at row 1", 'assetnumber'),
|
||||
("Incorrect date value: 'x' for column 'enddate' at row 1", 'enddate'),
|
||||
('no column mentioned at all', None),
|
||||
('', None),
|
||||
(None, None),
|
||||
])
|
||||
def test_the_column_name_is_pulled_out_when_the_engine_names_one(message,
|
||||
expected):
|
||||
assert _column_from_data_error(message) == expected
|
||||
|
||||
|
||||
def test_the_strict_sql_mode_is_actually_set_on_the_mysql_connection():
|
||||
"""The whole class of fix rests on this one connect_arg, and it is exactly
|
||||
the kind of line a later edit drops without noticing."""
|
||||
from shopdb.config import Config
|
||||
|
||||
connect_args = Config.SQLALCHEMY_ENGINE_OPTIONS['connect_args']
|
||||
assert 'STRICT_TRANS_TABLES' in connect_args['init_command']
|
||||
Reference in New Issue
Block a user