openstack/congress
Code Issues Proposed changes

Distinguish between PUT and PATCH in API models

Browse Source 
At some point years ago, whether by design or by mistake,
the API models have conflated PUT (replace) and PATCH (update) [1].

This patch cleans up the models by making a clear distinction.
All the existing PUT implementations confusingly named update
are changed to replace.

[1] e81e63966b/congress/api/webservice.py (L203)

Change-Id: I6786c39b7555c3b854ef90cb49d0ccb23f6f5bb2
This commit is contained in:
Eric K 2018-01-16 16:13:59 -08:00
parent 8eebec49cd
commit 2af942ada8
11 changed files with 70 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
congress/api/library_policy_model.py
View File

@ -137,16 +137,16 @@ class LibraryPolicyModel(base.APIModel):
'delete_policy',
{'id_': id_})
def update_item(self, id_, item, params, context=None):
"""Update item with id\_ with new data.
def replace_item(self, id_, item, params, context=None):
"""Replace item with id\_ with new data.
:param: id\_: The ID of the item to be updated
:param: id\_: The ID of the item to be replaced
:param: item: The new item
:param: params: A dict-like object containing parameters
from the request query string and body.
:param: context: Key-values providing frame of reference of request
:returns: The updated item.
:returns: The new item after replacement.
:raises KeyError: Item with specified id\_ not present.
"""

2
congress/api/router.py
View File

@ -137,7 +137,7 @@ class APIRouterV1(object):
rows_path = "%s/rows" % table_path
row_collection_handler = webservice.CollectionHandler(
rows_path,
table_rows, allow_update=True)
table_rows, allow_replace=True)
resource_mgr.register_handler(row_collection_handler)
row_path = "%s/(?P<row_id>[^/]+)" % rows_path
row_element_handler = webservice.ElementHandler(row_path, table_rows)

14
congress/api/row_model.py
View File

@ -102,10 +102,10 @@ class RowModel(base.APIModel):
return {'results': result}
# Note(thread-safety): blocking function
def update_items(self, items, params, context=None):
"""Updates all data in a table.
def replace_items(self, items, params, context=None):
"""Replaces all data in a table.
:param: id\_: A table id for updating all row
:param: id\_: A table id for replacing all row
:param: items: A data for new rows
:param: params: A dict-like object containing parameters from
request query
@ -114,7 +114,7 @@ class RowModel(base.APIModel):
:raises KeyError: table id doesn't exist
:raises DataModelException: any error occurs during replacing rows.
"""
LOG.info("update_items(context=%s)", context)
LOG.info("replace_items(context=%s)", context)
# Note(thread-safety): blocking call
caller, source_id = api_utils.get_id_from_context(context)
# FIXME(threod-safety): in DSE2, the returned caller can be a
@ -129,14 +129,14 @@ class RowModel(base.APIModel):
args = {'table_id': table_id, 'source_id': source_id,
'objs': items}
# Note(thread-safety): blocking call
self.invoke_rpc(caller, 'update_entire_data', args)
self.invoke_rpc(caller, 'replace_entire_table_data', args)
except exception.CongressException as e:
LOG.exception("Error occurred while processing updating rows "
"for source_id '%s' and table_id '%s'",
source_id, table_id)
raise webservice.DataModelException.create(e)
LOG.info("finish update_items(context=%s)", context)
LOG.debug("updated table %s with row items: %s",
LOG.info("finish replace_items(context=%s)", context)
LOG.debug("replaced table %s with row items: %s",
table_id, str(items))
# TODO(thinrichs): It makes sense to sometimes allow users to create

43
congress/api/webservice.py
View File

@ -308,7 +308,7 @@ class ElementHandler(AbstractApiHandler):
id_ = self._get_element_id(request)
try:
item = self._parse_json_body(request)
self.model.update_item(id_, item, request.params,
self.model.replace_item(id_, item, request.params,
context=self._get_context(request))
except KeyError as e:
if (self.collection_handler and
@ -335,7 +335,7 @@ class ElementHandler(AbstractApiHandler):
updates = self._parse_json_body(request)
item.update(updates)
self.model.update_item(id_, item, request.params, context=context)
self.model.replace_item(id_, item, request.params, context=context)
return webob.Response(body="%s\n" % json.dumps(item),
status=httplib.OK,
content_type='application/json',
@ -374,7 +374,7 @@ class CollectionHandler(AbstractApiHandler):
def __init__(self, path_regex, model,
allow_named_create=True, allow_list=True, allow_create=True,
allow_update=False):
allow_replace=False):
"""Initialize a collection handler.
:param: path_regex: A regular expression matching the collection base
@ -389,7 +389,7 @@ class CollectionHandler(AbstractApiHandler):
self.allow_named_create = allow_named_create
self.allow_list = allow_list
self.allow_create = allow_create
self.allow_update = allow_update
self.allow_replace = allow_replace
def handle_request(self, request):
"""Handle a REST request.
@ -424,8 +424,8 @@ class CollectionHandler(AbstractApiHandler):
return self.list_members(request)
elif request.method == 'POST' and self.allow_create:
return self.create_member(request)
elif request.method == 'PUT' and self.allow_update:
return self.update_members(request)
elif request.method == 'PUT' and self.allow_replace:
return self.replace_members(request)
return NOT_SUPPORTED_RESPONSE
def _get_action_type(self, method):
@ -478,13 +478,13 @@ class CollectionHandler(AbstractApiHandler):
location="%s/%s" % (request.path, id_),
charset='UTF-8')
def update_members(self, request):
if not hasattr(self.model, 'update_items'):
def replace_members(self, request):
if not hasattr(self.model, 'replace_items'):
return NOT_SUPPORTED_RESPONSE
items = self._parse_json_body(request)
context = self._get_context(request)
try:
self.model.update_items(items, request.params, context)
self.model.replace_items(items, request.params, context)
except KeyError as e:
LOG.exception("Error occurred")
return error_response(httplib.BAD_REQUEST, httplib.BAD_REQUEST,
@ -580,6 +580,27 @@ class SimpleDataModel(object):
self.items.setdefault(cstr, {})[id_] = item
return item
def replace_item(self, id_, item, params, context=None):
"""Replace item with id\_ with new data.
:param: id\_: The ID of the item to be replaced
item: The new item
:param: params: A dict-like object containing parameters
from the request query string and body.
:param: context: Key-values providing frame of reference of request
:returns: The new item after replacement.
:raises KeyError: Item with specified id\_ not present.
:raises DataModelException: Replacement cannot be performed.
"""
cstr = self._context_str(context)
if id_ not in self.items.setdefault(cstr, {}):
raise KeyError("Cannot replace item with ID '%s': "
"ID does not exist" % id_)
self.items.setdefault(cstr, {})[id_] = item
return item
def delete_item(self, id_, params, context=None):
"""Remove item from model.
@ -597,8 +618,8 @@ class SimpleDataModel(object):
del self.items[cstr][id_]
return ret
def update_items(self, items, params, context=None):
"""Update items in the model.
def replace_items(self, items, params, context=None):
"""Replace items in the model.
:param: items: A dict-like object containing new data
:param: params: A dict-like object containing parameters

6
congress/datasources/datasource_driver.py
View File

@ -1221,7 +1221,7 @@ class PushedDataSourceDriver(DataSourceDriver):
self.state[table['tablename']] = table['tabledata']
# Note (thread-safety): blocking function
def update_entire_data(self, table_id, objs):
def replace_entire_table_data(self, table_id, objs):
LOG.info('update %s table in %s datasource', table_id, self.name)
translator = self.get_translator(table_id)
tablename = translator['table-name']
@ -1250,9 +1250,9 @@ class PushedDataSourceDriverEndpoints(data_service.DataServiceEndPoints):
super(PushedDataSourceDriverEndpoints, self).__init__(service)
# Note (thread-safety): blocking function
def update_entire_data(self, context, table_id, source_id, objs):
def replace_entire_table_data(self, context, table_id, source_id, objs):
# Note (thread-safety): blocking call
return self.service.update_entire_data(table_id, objs)
return self.service.replace_entire_table_data(table_id, objs)
class PollingDataSourceDriver(DataSourceDriver):

2
congress/datasources/push_driver.py
View File

@ -66,7 +66,7 @@ class PushDriver(datasource_driver.PushedDataSourceDriver):
'persist_data': constants.OPTIONAL}
return result
def update_entire_data(self, table_id, objs):
def replace_entire_table_data(self, table_id, objs):
LOG.info('update %s table in %s datasource', table_id, self.name)
tablename = 'data' # hard code
self.prior_state = dict(self.state)

22
congress/tests/api/test_library_policy_model.py
View File

@ -170,7 +170,7 @@ class TestLibraryPolicyModel(base.SqlTestCase):
self.assertRaises(webservice.DataModelException,
self.library_policy_model.add_item, test, {})
def test_update_item_without_name(self):
def test_replace_item_without_name(self):
test = {
"description": "test description",
"kind": "nonrecursive",
@ -178,10 +178,10 @@ class TestLibraryPolicyModel(base.SqlTestCase):
}
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'], test, {})
def test_update_item_with_long_abbreviation(self):
def test_replace_item_with_long_abbreviation(self):
test = {
"name": "test",
"description": "test description",
@ -190,7 +190,7 @@ class TestLibraryPolicyModel(base.SqlTestCase):
"rules": []
}
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'], test, {})
def test_delete_item(self):
@ -217,7 +217,7 @@ class TestLibraryPolicyModel(base.SqlTestCase):
{'rules': []}, {})
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'], {'rules': []}, {})
# policy with bad name
@ -225,14 +225,14 @@ class TestLibraryPolicyModel(base.SqlTestCase):
self.library_policy_model.add_item,
{'name': '7*7', 'rules': []}, {})
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'], {'name': '7*7', 'rules': []}, {})
self.assertRaises(webservice.DataModelException,
self.library_policy_model.add_item,
{'name': 'p(x) :- q(x)'}, {})
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'], {'name': 'p(x) :- q(x)'}, {})
# policy with invalid 'kind'
@ -241,12 +241,12 @@ class TestLibraryPolicyModel(base.SqlTestCase):
{'kind': 'nonexistent', 'name': 'alice',
'rules': []}, {})
self.assertRaises(webservice.DataModelException,
self.library_policy_model.update_item,
self.library_policy_model.replace_item,
self.policy['id'],
{'kind': 'nonexistent', 'name': 'alice',
'rules': []}, {})
def test_update_item(self):
def test_replace_item(self):
replacement_policy = {
"name": "new_name",
"description": "new test policy2 description",
@ -258,11 +258,11 @@ class TestLibraryPolicyModel(base.SqlTestCase):
# update non-existent item
self.assertRaises(KeyError,
self.library_policy_model.update_item, 'no_such_id',
self.library_policy_model.replace_item, 'no_such_id',
replacement_policy, {}, {})
# update existing item
self.library_policy_model.update_item(
self.library_policy_model.replace_item(
self.policy2['id'], replacement_policy, {}, {})
replacement_policy_w_id = copy.deepcopy(replacement_policy)

8
congress/tests/api/test_row_model.py
View File

@ -101,7 +101,7 @@ class TestRowModel(base.SqlTestCase):
self.assertRaises(webservice.DataModelException,
self.row_model.get_items, {}, context)
def test_update_items(self):
def test_replace_items(self):
context = {'ds_id': self.data.service_id,
'table_id': 'fake_table'}
objs = [
@ -110,14 +110,14 @@ class TestRowModel(base.SqlTestCase):
]
expected_state = (('id-1', 'name-1'), ('id-2', 'name-2'))
self.row_model.update_items(objs, {}, context=context)
self.row_model.replace_items(objs, {}, context=context)
table_row = self.data.state['fake_table']
self.assertEqual(len(expected_state), len(table_row))
for row in expected_state:
self.assertIn(row, table_row)
def test_update_items_invalid_table(self):
def test_replace_items_invalid_table(self):
context = {'ds_id': self.data.service_id,
'table_id': 'invalid_table'}
objs = [
@ -125,4 +125,4 @@ class TestRowModel(base.SqlTestCase):
{"id": 'id-2', "name": 'name-2'}
]
self.assertRaises(webservice.DataModelException,
self.row_model.update_items, objs, {}, context)
self.row_model.replace_items, objs, {}, context)

4
congress/tests/api/test_webservice.py
View File

@ -316,13 +316,13 @@ class TestCollectionHandler(base.TestCase):
self.assertEqual('application/json', response.content_type)
self.assertEqual(str(int(httplib.OK)) + " OK", response.status)
def test_update_members(self):
def test_replace_members(self):
collection_handler = webservice.CollectionHandler(r'/', '')
collection_handler.model = webservice.SimpleDataModel('test')
request = webob.Request.blank('/')
request.content_type = 'application/json'
request.body = '{"key1": "value1", "key2": "value2"}'.encode('utf-8')
response = collection_handler.update_members(request)
response = collection_handler.replace_members(request)
self.assertEqual('application/json', response.content_type)
self.assertEqual(str(int(httplib.OK)) + " OK", response.status)

6
congress/tests/datasources/test_datasource_driver.py
View File

@ -1844,7 +1844,7 @@ class TestPushedDriver(base.SqlTestCase):
{'id': 1, 'name': 'column1', 'status': 'up'},
{'id': 2, 'name': 'column2', 'status': 'down'}
]
test_driver.update_entire_data('test_translator', obj)
test_driver.replace_entire_table_data('test_translator', obj)
expected_state = set([
(1, 'column1', 'up'),
(2, 'column2', 'down')])
@ -1864,7 +1864,7 @@ class TestPushedDriver(base.SqlTestCase):
# test no persist if not enabled
test_driver = TestPushedDriver.TestDriver(
args={'ds_id': ds_id, 'persist_data': False})
test_driver.update_entire_data('test_translator', obj)
test_driver.replace_entire_table_data('test_translator', obj)
expected_state = set([
(1, 'column1', 'up'),
(2, 'column2', 'down')])
@ -1875,7 +1875,7 @@ class TestPushedDriver(base.SqlTestCase):
# test data persisted in DB
test_driver = TestPushedDriver.TestDriver(
args={'ds_id': ds_id, 'persist_data': True})
test_driver.update_entire_data('test_translator', obj)
test_driver.replace_entire_table_data('test_translator', obj)
expected_state = set([
(1, 'column1', 'up'),
(2, 'column2', 'down')])

2
congress/tests/datasources/test_doctor_driver.py
View File

@ -51,7 +51,7 @@ class TestDoctorDriver(base.TestCase):
@mock.patch.object(doctor_driver.DoctorDriver, 'publish')
def test_events_table(self, mocked_publish):
objs = self.generate_events_objects(3)
self.doctor.update_entire_data('events', objs)
self.doctor.replace_entire_table_data('events', objs)
self.assertEqual(3, len(self.doctor.state['events']))