From 44fb49b1286f589974c443393a05302604fdc525 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 15 Dec 2017 11:03:32 -0800 Subject: [PATCH] Improve user error messages for duplicate objects The error handling for duplicate records was not great, specifically health monitors would return a 500 or a 409 with a bad error message. This patch improves the errors the user gets back when creating a duplicate entity. Co-Authored-By: Jacky Hu Change-Id: I03ec527b42d67427541121e59e4433699e04aab8 --- octavia/api/v2/controllers/health_monitor.py | 5 ++--- octavia/api/v2/controllers/l7policy.py | 5 ++--- octavia/api/v2/controllers/l7rule.py | 7 +++---- octavia/api/v2/controllers/pool.py | 5 ++--- octavia/db/models.py | 5 +++++ octavia/tests/functional/api/v2/test_health_monitor.py | 6 ------ .../notes/fix-error-messages-ec817a66249e6666.yaml | 5 +++++ 7 files changed, 19 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/fix-error-messages-ec817a66249e6666.yaml diff --git a/octavia/api/v2/controllers/health_monitor.py b/octavia/api/v2/controllers/health_monitor.py index 7ef7ea9c20..a24292d21a 100644 --- a/octavia/api/v2/controllers/health_monitor.py +++ b/octavia/api/v2/controllers/health_monitor.py @@ -129,9 +129,8 @@ class HealthMonitorController(base.BaseController): try: return self.repositories.health_monitor.create( lock_session, **hm_dict) - except odb_exceptions.DBDuplicateEntry as de: - if ['id'] == de.columns: - raise exceptions.IDAlreadyExists() + except odb_exceptions.DBDuplicateEntry: + raise exceptions.DuplicateHealthMonitor() except odb_exceptions.DBError: # TODO(blogan): will have to do separate validation protocol # before creation or update since the exception messages diff --git a/octavia/api/v2/controllers/l7policy.py b/octavia/api/v2/controllers/l7policy.py index 59fa0e572f..6bde36babf 100644 --- a/octavia/api/v2/controllers/l7policy.py +++ b/octavia/api/v2/controllers/l7policy.py @@ -107,9 +107,8 @@ class L7PolicyController(base.BaseController): try: return self.repositories.l7policy.create(lock_session, **l7policy_dict) - except odb_exceptions.DBDuplicateEntry as de: - if ['id'] == de.columns: - raise exceptions.IDAlreadyExists() + except odb_exceptions.DBDuplicateEntry: + raise exceptions.IDAlreadyExists() except odb_exceptions.DBError: # TODO(blogan): will have to do separate validation protocol # before creation or update since the exception messages diff --git a/octavia/api/v2/controllers/l7rule.py b/octavia/api/v2/controllers/l7rule.py index f355c188e7..70e75b755f 100644 --- a/octavia/api/v2/controllers/l7rule.py +++ b/octavia/api/v2/controllers/l7rule.py @@ -116,10 +116,9 @@ class L7RuleController(base.BaseController): def _validate_create_l7rule(self, lock_session, l7rule_dict): try: return self.repositories.l7rule.create(lock_session, **l7rule_dict) - except odb_exceptions.DBDuplicateEntry as de: - if ['id'] == de.columns: - raise exceptions.IDAlreadyExists() - except odb_exceptions.DBError as de: + except odb_exceptions.DBDuplicateEntry: + raise exceptions.IDAlreadyExists() + except odb_exceptions.DBError: # TODO(blogan): will have to do separate validation protocol # before creation or update since the exception messages # do not give any information as to what constraint failed diff --git a/octavia/api/v2/controllers/pool.py b/octavia/api/v2/controllers/pool.py index 9bc4d8fc70..470d8ff717 100644 --- a/octavia/api/v2/controllers/pool.py +++ b/octavia/api/v2/controllers/pool.py @@ -114,9 +114,8 @@ class PoolsController(base.BaseController): return self.repositories.create_pool_on_load_balancer( lock_session, pool_dict, listener_id=listener_id) - except odb_exceptions.DBDuplicateEntry as de: - if ['id'] == de.columns: - raise exceptions.IDAlreadyExists() + except odb_exceptions.DBDuplicateEntry: + raise exceptions.IDAlreadyExists() except odb_exceptions.DBError: # TODO(blogan): will have to do separate validation protocol # before creation or update since the exception messages diff --git a/octavia/db/models.py b/octavia/db/models.py index bc0c5ecc5c..e27b2001ac 100644 --- a/octavia/db/models.py +++ b/octavia/db/models.py @@ -218,6 +218,11 @@ class HealthMonitor(base_models.BASE, base_models.IdMixin, __v2_wsme__ = health_monitor.HealthMonitorResponse + __table_args__ = ( + sa.UniqueConstraint('pool_id', + name='uq_health_monitor_pool'), + ) + type = sa.Column( sa.String(36), sa.ForeignKey("health_monitor_type.name", diff --git a/octavia/tests/functional/api/v2/test_health_monitor.py b/octavia/tests/functional/api/v2/test_health_monitor.py index 0a9e91dfb2..b1e9e1b083 100644 --- a/octavia/tests/functional/api/v2/test_health_monitor.py +++ b/octavia/tests/functional/api/v2/test_health_monitor.py @@ -746,12 +746,6 @@ class TestHealthMonitor(base.BaseAPITest): hm_op_status=constants.OFFLINE) def test_duplicate_create(self): - # TODO(rm_work): I am fairly certain this is the same issue as we see - # in test_repositories.py where PySqlite commits too early and can't - # roll back, causing things to get out of whack. This runs fine solo. - # It would be useful to test this *in reality* and see if it breaks. - self.skipTest("PySqlite transaction handling is broken. We can unskip" - "this when `test_sqlite_transactions_broken` fails.") self.create_health_monitor( self.pool_id, constants.HEALTH_MONITOR_HTTP, 1, 1, 1, 1) self.set_lb_status(self.lb_id) diff --git a/releasenotes/notes/fix-error-messages-ec817a66249e6666.yaml b/releasenotes/notes/fix-error-messages-ec817a66249e6666.yaml new file mode 100644 index 0000000000..c875f15765 --- /dev/null +++ b/releasenotes/notes/fix-error-messages-ec817a66249e6666.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Improves error messages returned to the user, such as errors for attempting + to add a second health monitor to a pool.