From 5f580161cb3e7b57582e026f7071567f74b8a2dc Mon Sep 17 00:00:00 2001 From: Mehdi Abaakouk Date: Fri, 6 Jan 2017 09:03:15 +0100 Subject: [PATCH] allow required=True when patching resource type We have currently two limitations about resource type update. * We cannot set required=True on a new attribute * We cannot update a attribute This change introduces the notion of 'options' for the description of a new attribute or an attribute to update. And implements the first item on top of this "option". The option added is 'fill' to known to fill all row of existing resources with a default value. And 'fill' is required if 'required' is set to allow new attribute with required=True. Change-Id: If0bd609ed586b6fbe4fe7877ece237e55baa7d45 --- doc/source/rest.yaml | 5 + gnocchi/indexer/sqlalchemy.py | 19 +- gnocchi/indexer/sqlalchemy_extension.py | 26 ++- gnocchi/resource_type.py | 60 +++++- gnocchi/rest/__init__.py | 10 +- .../tests/gabbi/gabbits/resource-type.yaml | 177 ++++++++++++++++++ gnocchi/tests/test_indexer.py | 7 +- ...-required-attributes-f446c220d54c8eb7.yaml | 6 + 8 files changed, 287 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/resource-type-required-attributes-f446c220d54c8eb7.yaml diff --git a/doc/source/rest.yaml b/doc/source/rest.yaml index 4d95c4603..396576eeb 100644 --- a/doc/source/rest.yaml +++ b/doc/source/rest.yaml @@ -455,6 +455,11 @@ "path": "/attributes/awesome-stuff", "value": {"type": "bool", "required": false} }, + { + "op": "add", + "path": "/attributes/required-stuff", + "value": {"type": "bool", "required": true, "options": {"fill": true}} + }, { "op": "remove", "path": "/attributes/prefix" diff --git a/gnocchi/indexer/sqlalchemy.py b/gnocchi/indexer/sqlalchemy.py index bcb037dc2..cc2aab0bc 100644 --- a/gnocchi/indexer/sqlalchemy.py +++ b/gnocchi/indexer/sqlalchemy.py @@ -397,6 +397,7 @@ class SQLAlchemyIndexer(indexer.IndexerDriver): try: with self.facade.independent_writer() as session: + engine = session.connection() rt = self._get_resource_type(session, name) with self.facade.writer_connection() as connection: @@ -407,12 +408,22 @@ class SQLAlchemyIndexer(indexer.IndexerDriver): for attr in del_attributes: batch_op.drop_column(attr) for attr in add_attributes: - # TODO(sileht): When attr.required is True, we - # have to pass a default. rest layer current - # protect us, requied = True is not yet allowed + server_default = attr.for_filling( + engine.dialect) batch_op.add_column(sqlalchemy.Column( attr.name, attr.satype, - nullable=not attr.required)) + nullable=not attr.required, + server_default=server_default)) + + # We have all rows filled now, we can remove + # the server_default + if server_default is not None: + batch_op.alter_column( + column_name=attr.name, + existing_type=attr.satype, + existing_server_default=server_default, + existing_nullable=not attr.required, + server_default=None) rt.state = "active" rt.updated_at = utils.utcnow() diff --git a/gnocchi/indexer/sqlalchemy_extension.py b/gnocchi/indexer/sqlalchemy_extension.py index 058d31b2e..bc4d84181 100644 --- a/gnocchi/indexer/sqlalchemy_extension.py +++ b/gnocchi/indexer/sqlalchemy_extension.py @@ -20,19 +20,37 @@ import sqlalchemy_utils from gnocchi import resource_type -class StringSchema(resource_type.StringSchema): +class SchemaMixin(object): + def for_filling(self, dialect): + # NOTE(sileht): This must be used only for patching resource type + # to fill all row with a default value and then switch back the + # server_default to None + if self.fill is None: + return None + + # NOTE(sileht): server_default must be converted in sql element + return sqlalchemy.literal(self.fill) + + +class StringSchema(resource_type.StringSchema, SchemaMixin): @property def satype(self): return sqlalchemy.String(self.max_length) -class UUIDSchema(resource_type.UUIDSchema): +class UUIDSchema(resource_type.UUIDSchema, SchemaMixin): satype = sqlalchemy_utils.UUIDType() + def for_filling(self, dialect): + if self.fill is None: + return False # Don't set any server_default + return sqlalchemy.literal( + self.satype.process_bind_param(self.fill, dialect)) -class NumberSchema(resource_type.NumberSchema): + +class NumberSchema(resource_type.NumberSchema, SchemaMixin): satype = sqlalchemy.Float(53) -class BoolSchema(resource_type.BoolSchema): +class BoolSchema(resource_type.BoolSchema, SchemaMixin): satype = sqlalchemy.Boolean diff --git a/gnocchi/resource_type.py b/gnocchi/resource_type.py index ad1bcddb6..73b755648 100644 --- a/gnocchi/resource_type.py +++ b/gnocchi/resource_type.py @@ -55,24 +55,63 @@ class InvalidResourceAttributeValue(InvalidResourceAttribute): self.max = max +class InvalidResourceAttributeOption(InvalidResourceAttribute): + """Error raised when the resource attribute name is invalid.""" + def __init__(self, name, option, reason): + super(InvalidResourceAttributeOption, self).__init__( + "Option '%s' of resource attribute %s is invalid: %s" % + (option, str(name), str(reason))) + self.name = name + self.option = option + self.reason = reason + + +# NOTE(sileht): This is to store the behavior of some operations: +# * fill, to set a default value to all existing resource type +# +# in the future for example, we can allow to change the length of +# a string attribute, if the new one is shorter, we can add a option +# to define the behavior like: +# * resize = trunc or reject +OperationOptions = { + voluptuous.Optional('fill'): object +} + + class CommonAttributeSchema(object): meta_schema_ext = {} schema_ext = None - def __init__(self, type, name, required): + def __init__(self, type, name, required, options=None): if (len(name) > 63 or name in INVALID_NAMES or not VALID_CHARS.match(name)): raise InvalidResourceAttributeName(name) self.name = name self.required = required + self.fill = None + + # options is set only when we update a resource type + if options is not None: + fill = options.get("fill") + if fill is None and required: + raise InvalidResourceAttributeOption( + name, "fill", "must not be empty if required=True") + elif fill is not None: + # Ensure fill have the correct attribute type + try: + self.fill = voluptuous.Schema(self.schema_ext)(fill) + except voluptuous.Error as e: + raise InvalidResourceAttributeOption(name, "fill", e) @classmethod - def meta_schema(cls): + def meta_schema(cls, for_update=False): d = { voluptuous.Required('type'): cls.typename, voluptuous.Required('required', default=True): bool } + if for_update: + d[voluptuous.Required('options', default={})] = OperationOptions if callable(cls.meta_schema_ext): d.update(cls.meta_schema_ext()) else: @@ -94,12 +133,12 @@ class StringSchema(CommonAttributeSchema): typename = "string" def __init__(self, min_length, max_length, *args, **kwargs): - super(StringSchema, self).__init__(*args, **kwargs) if min_length > max_length: raise InvalidResourceAttributeValue(min_length, max_length) self.min_length = min_length self.max_length = max_length + super(StringSchema, self).__init__(*args, **kwargs) meta_schema_ext = { voluptuous.Required('min_length', default=0): @@ -131,12 +170,11 @@ class NumberSchema(CommonAttributeSchema): typename = "number" def __init__(self, min, max, *args, **kwargs): - super(NumberSchema, self).__init__(*args, **kwargs) if max is not None and min is not None and min > max: raise InvalidResourceAttributeValue(min, max) - self.min = min self.max = max + super(NumberSchema, self).__init__(*args, **kwargs) meta_schema_ext = { voluptuous.Required('min', default=None): voluptuous.Any( @@ -182,9 +220,21 @@ class ResourceTypeSchemaManager(stevedore.ExtensionManager): } }) + type_schemas = tuple([ext.plugin.meta_schema(for_update=True) + for ext in self.extensions]) + self._schema_for_update = voluptuous.Schema({ + "name": six.text_type, + voluptuous.Required("attributes", default={}): { + six.text_type: voluptuous.Any(*tuple(type_schemas)) + } + }) + def __call__(self, definition): return self._schema(definition) + def for_update(self, definition): + return self._schema_for_update(definition) + def attributes_from_dict(self, attributes): return ResourceTypeAttributes( self[attr["type"]].plugin(name=name, **attr) diff --git a/gnocchi/rest/__init__.py b/gnocchi/rest/__init__.py index 05514021d..5204fae95 100644 --- a/gnocchi/rest/__init__.py +++ b/gnocchi/rest/__init__.py @@ -755,7 +755,7 @@ class ResourceTypeController(rest.RestController): # Validate that the whole new resource_type is valid schema = pecan.request.indexer.get_resource_type_schema() try: - rt_json_next = voluptuous.Schema(schema, required=True)( + rt_json_next = voluptuous.Schema(schema.for_update, required=True)( rt_json_next) except voluptuous.Error as e: abort(400, "Invalid input: %s" % e) @@ -776,14 +776,6 @@ class ResourceTypeController(rest.RestController): except resource_type.InvalidResourceAttribute as e: abort(400, "Invalid input: %s" % e) - # TODO(sileht): Add a default field on an attribute - # to be able to fill non-nullable column on sql side. - # And obviousy remove this limitation - for attr in add_attrs: - if attr.required: - abort(400, ValueError("Adding required attributes is not yet " - "possible.")) - try: return pecan.request.indexer.update_resource_type( self._name, add_attributes=add_attrs, diff --git a/gnocchi/tests/gabbi/gabbits/resource-type.yaml b/gnocchi/tests/gabbi/gabbits/resource-type.yaml index 786cf27e1..b41326bef 100644 --- a/gnocchi/tests/gabbi/gabbits/resource-type.yaml +++ b/gnocchi/tests/gabbi/gabbits/resource-type.yaml @@ -348,6 +348,47 @@ tests: required: False min_length: 0 max_length: 255 + - op: add + path: /attributes/newfilled + value: + type: string + required: False + min_length: 0 + max_length: 255 + options: + fill: "filled" + - op: add + path: /attributes/newbool + value: + type: bool + required: True + options: + fill: True + - op: add + path: /attributes/newint + value: + type: number + required: True + min: 0 + max: 255 + options: + fill: 15 + - op: add + path: /attributes/newstring + value: + type: string + required: True + min_length: 0 + max_length: 255 + options: + fill: "foobar" + - op: add + path: /attributes/newuuid + value: + type: uuid + required: True + options: + fill: "00000000-0000-0000-0000-000000000000" - op: remove path: /attributes/foobar status: 200 @@ -385,6 +426,62 @@ tests: required: False min_length: 0 max_length: 255 + newfilled: + type: string + required: False + min_length: 0 + max_length: 255 + newstring: + type: string + required: True + min_length: 0 + max_length: 255 + newbool: + type: bool + required: True + newint: + type: number + required: True + min: 0 + max: 255 + newuuid: + type: uuid + required: True + + - name: post a new resource attribute with missing fill + url: /v1/resource_type/my_custom_resource + method: patch + request_headers: + x-roles: admin + content-type: application/json-patch+json + data: + - op: add + path: /attributes/missing + value: + type: bool + required: True + options: {} + status: 400 + response_strings: + - "Invalid input: Option 'fill' of resource attribute missing is invalid: must not be empty if required=True" + + - name: post a new resource attribute with incorrect fill + url: /v1/resource_type/my_custom_resource + method: patch + request_headers: + x-roles: admin + content-type: application/json-patch+json + data: + - op: add + path: /attributes/incorrect + value: + type: number + required: True + options: + fill: "a-string" + status: 400 + response_strings: + - "Invalid input: Option 'fill' of resource attribute incorrect is invalid: expected Real" - name: get the new custom resource type url: /v1/resource_type/my_custom_resource @@ -422,6 +519,65 @@ tests: required: False min_length: 0 max_length: 255 + newfilled: + type: string + required: False + min_length: 0 + max_length: 255 + newstring: + type: string + required: True + min_length: 0 + max_length: 255 + newbool: + type: bool + required: True + newint: + type: number + required: True + min: 0 + max: 255 + newuuid: + type: uuid + required: True + + - name: control new attributes of existing resource + GET: /v1/resource/my_custom_resource/d11edfca-4393-4fda-b94d-b05a3a1b3747 + request_headers: + content-type: application/json + status: 200 + response_json_paths: + $.id: d11edfca-4393-4fda-b94d-b05a3a1b3747 + $.name: foo + $.newstuff: null + $.newfilled: "filled" + $.newbool: true + $.newint: 15 + $.newstring: foobar + $.newuuid: "00000000-0000-0000-0000-000000000000" + + - name: control new attributes of existing resource history + GET: /v1/resource/my_custom_resource/d11edfca-4393-4fda-b94d-b05a3a1b3747/history?sort=revision_end:asc-nullslast + request_headers: + content-type: application/json + response_json_paths: + $.`len`: 2 + $[0].id: d11edfca-4393-4fda-b94d-b05a3a1b3747 + $[0].name: bar + $[0].newstuff: null + $[0].newfilled: "filled" + $[0].newbool: true + $[0].newint: 15 + $[0].newstring: foobar + $[0].newuuid: "00000000-0000-0000-0000-000000000000" + $[1].id: d11edfca-4393-4fda-b94d-b05a3a1b3747 + $[1].name: foo + $[1].newstuff: null + $[1].newfilled: "filled" + $[1].newbool: true + $[1].newint: 15 + $[1].newstring: foobar + $[1].newuuid: "00000000-0000-0000-0000-000000000000" # Invalid patch @@ -476,6 +632,27 @@ tests: required: False min_length: 0 max_length: 255 + newfilled: + type: string + required: False + min_length: 0 + max_length: 255 + newstring: + type: string + required: True + min_length: 0 + max_length: 255 + newbool: + type: bool + required: True + newint: + type: number + required: True + min: 0 + max: 255 + newuuid: + type: uuid + required: True - name: delete/add the same resource attribute url: /v1/resource_type/my_custom_resource diff --git a/gnocchi/tests/test_indexer.py b/gnocchi/tests/test_indexer.py index 80d0d0e6f..a87f59dc3 100644 --- a/gnocchi/tests/test_indexer.py +++ b/gnocchi/tests/test_indexer.py @@ -1120,7 +1120,9 @@ class TestIndexerDriver(tests_base.TestCase): # Update the resource type add_attrs = mgr.resource_type_from_dict("indexer_test", { "col2": {"type": "number", "required": False, - "max": 100, "min": 0} + "max": 100, "min": 0}, + "col3": {"type": "number", "required": True, + "max": 100, "min": 0, "options": {'fill': 15}} }, "creating").attributes self.index.update_resource_type("indexer_test", add_attributes=add_attrs) @@ -1128,6 +1130,7 @@ class TestIndexerDriver(tests_base.TestCase): # Check the new attribute r = self.index.get_resource("indexer_test", rid) self.assertIsNone(r.col2) + self.assertEqual(15, r.col3) self.index.update_resource("indexer_test", rid, col2=10) @@ -1139,6 +1142,8 @@ class TestIndexerDriver(tests_base.TestCase): self.assertEqual(2, len(rl)) self.assertIsNone(rl[0].col2) self.assertEqual(10, rl[1].col2) + self.assertEqual(15, rl[0].col3) + self.assertEqual(15, rl[1].col3) # Deletion self.assertRaises(indexer.ResourceTypeInUse, diff --git a/releasenotes/notes/resource-type-required-attributes-f446c220d54c8eb7.yaml b/releasenotes/notes/resource-type-required-attributes-f446c220d54c8eb7.yaml new file mode 100644 index 000000000..a91c8176c --- /dev/null +++ b/releasenotes/notes/resource-type-required-attributes-f446c220d54c8eb7.yaml @@ -0,0 +1,6 @@ +--- +features: + - When updating a resource attribute, it's now possible to pass the option + 'fill' for each attribute to fill existing resources. + - required=True is now supported when updating resource type. This requires + the option 'fill' to be set.