SEBA-420 rename varchar to text; validation on max_length
Change-Id: I71791d27024260572e552936d39cb1f07ddaab38
diff --git a/lib/xos-genx/xos-genx-tests/test_swagger.py b/lib/xos-genx/xos-genx-tests/test_swagger.py
index 1886a76..5b6d1f9 100644
--- a/lib/xos-genx/xos-genx-tests/test_swagger.py
+++ b/lib/xos-genx/xos-genx-tests/test_swagger.py
@@ -47,7 +47,7 @@
required manytoone node->Node:instances = 10 [db_index = True, null = False, blank = False];
required int32 numberCores = 11 [help_text = "Number of cores for instance", default = 0, null = False, db_index = False, blank = False];
required manytoone flavor->Flavor:instance = 12 [help_text = "Flavor of this instance", null = False, db_index = True, blank = False];
- optional string userData = 13 [help_text = "user_data passed to instance during creation", null = True, db_index = False, blank = True, varchar = True];
+ optional string userData = 13 [help_text = "user_data passed to instance during creation", null = True, db_index = False, blank = True, text = True];
required string isolation = 14 [default = "vm", choices = "(('vm', 'Virtual Machine'), ('container', 'Container'), ('container_vm', 'Container In VM'))", max_length = 30, blank = False, null = False, db_index = False];
optional string volumes = 15 [help_text = "Comma-separated list of directories to expose to parent context", null = True, db_index = False, blank = True];
optional manytoone parent->Instance:instance = 16 [help_text = "Parent Instance for containers nested inside of VMs", null = True, db_index = True, blank = True];
diff --git a/lib/xos-genx/xos-genx-tests/test_validator.py b/lib/xos-genx/xos-genx-tests/test_validator.py
new file mode 100644
index 0000000..1a50082
--- /dev/null
+++ b/lib/xos-genx/xos-genx-tests/test_validator.py
@@ -0,0 +1,125 @@
+# Copyright 2017-present Open Networking Foundation
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+from __future__ import absolute_import
+import unittest
+import os
+from xosgenx.validator import XProtoValidator
+from xosgenx.generator import XOSProcessor, XOSProcessorArgs
+from mock import patch
+import yaml
+
+# Generate other formats from xproto
+
+
+class XProtoValidatorTest(unittest.TestCase):
+ def test_suggested_max_length(self):
+ args = XOSProcessorArgs()
+ args.files = ["/tmp/testvalidator.xproto"]
+
+ open("/tmp/testvalidator.xproto", "w").write("""
+ option app_label = "test";
+
+ message Port (XOSBase){
+ required string foo = 1 [max_length=254];
+ }
+ """)
+ args.target = "modeldefs.xtarget"
+
+ with patch.object(XProtoValidator, "print_errors", autospec=True) as print_errors:
+ print_errors.return_value = None
+
+ output = XOSProcessor.process(args)
+
+ self.assertEqual(print_errors.call_count, 1)
+ validator = print_errors.call_args[0][0]
+
+ self.assertEqual(len(validator.errors), 1)
+ self.assertEqual(validator.errors[0]["severity"], "WARNING")
+ self.assertEqual(validator.errors[0]["message"], "max_length of 254 is close to suggested max_length of 256")
+
+ def test_max_length_okay(self):
+ args = XOSProcessorArgs()
+ args.files = ["/tmp/testvalidator.xproto"]
+
+ open("/tmp/testvalidator.xproto", "w").write("""
+ option app_label = "test";
+
+ message Port (XOSBase){
+ required string foo = 1 [max_length=256];
+ }
+ """)
+ args.target = "modeldefs.xtarget"
+
+ with patch.object(XProtoValidator, "print_errors", autospec=True) as print_errors:
+ print_errors.return_value = None
+
+ output = XOSProcessor.process(args)
+
+ self.assertEqual(print_errors.call_count, 0)
+
+ def test_max_length_zero(self):
+ args = XOSProcessorArgs()
+ args.files = ["/tmp/testvalidator.xproto"]
+
+ open("/tmp/testvalidator.xproto", "w").write("""
+ option app_label = "test";
+
+ message Port (XOSBase){
+ required string foo = 1 [max_length=0];
+ }
+ """)
+ args.target = "modeldefs.xtarget"
+
+ with patch.object(XProtoValidator, "print_errors", autospec=True) as print_errors:
+ print_errors.return_value = None
+
+ output = XOSProcessor.process(args)
+
+ self.assertEqual(print_errors.call_count, 1)
+ validator = print_errors.call_args[0][0]
+
+ self.assertEqual(len(validator.errors), 1)
+ self.assertEqual(validator.errors[0]["severity"], "ERROR")
+ self.assertEqual(validator.errors[0]["message"], "max_length should not be zero")
+
+
+ def test_charfield_missing_max_length(self):
+ args = XOSProcessorArgs()
+ args.files = ["/tmp/testvalidator.xproto"]
+
+ open("/tmp/testvalidator.xproto", "w").write("""
+ option app_label = "test";
+
+ message Port (XOSBase){
+ required string foo = 1 [];
+ }
+ """)
+ args.target = "modeldefs.xtarget"
+
+ with patch.object(XProtoValidator, "print_errors", autospec=True) as print_errors:
+ print_errors.return_value = None
+
+ output = XOSProcessor.process(args)
+
+ self.assertEqual(print_errors.call_count, 1)
+ validator = print_errors.call_args[0][0]
+
+ self.assertEqual(len(validator.errors), 1)
+ self.assertEqual(validator.errors[0]["severity"], "ERROR")
+ self.assertEqual(validator.errors[0]["message"], "String field should have a max_length or text=True")
+
+if __name__ == "__main__":
+ unittest.main()
diff --git a/lib/xos-genx/xosgenx/generator.py b/lib/xos-genx/xosgenx/generator.py
index 707f87b..d1dc99a 100644
--- a/lib/xos-genx/xosgenx/generator.py
+++ b/lib/xos-genx/xosgenx/generator.py
@@ -52,6 +52,7 @@
) # If neither include_models nor include_apps is specified, then all models will
default_include_apps = [] # be included.
default_strict_validation = False
+ default_lint = False
def __init__(self, **kwargs):
# set defaults
@@ -68,6 +69,7 @@
self.include_models = XOSProcessorArgs.default_include_models
self.include_apps = XOSProcessorArgs.default_include_apps
self.strict_validation = XOSProcessorArgs.default_strict_validation
+ self.lint = XOSProcessorArgs.default_lint
# override defaults with kwargs
for (k, v) in kwargs.items():
@@ -267,19 +269,6 @@
else:
raise Exception("[XosGenX] No inputs provided!")
- if not operator:
- operator = args.target
- template_path = XOSProcessor._get_template(operator)
- else:
- template_path = operator
-
- [template_folder, template_name] = os.path.split(template_path)
- os_template_loader = jinja2.FileSystemLoader(searchpath=[template_folder])
- os_template_env = jinja2.Environment(loader=os_template_loader)
- os_template_env = XOSProcessor._load_jinja2_extensions(
- os_template_env, args.attic
- )
- template = os_template_env.get_template(template_name)
context = XOSProcessor._add_context(args)
parser = plyxproto.ProtobufAnalyzer()
@@ -337,9 +326,27 @@
if validator.errors:
if args.strict_validation or (args.verbosity >= 0):
validator.print_errors()
- if args.strict_validation:
+ fatal_errors = [x for x in validator.errors if x["severity"] == "ERROR"]
+ if fatal_errors and args.strict_validation:
sys.exit(-1)
+ if args.lint:
+ return ""
+
+ if not operator:
+ operator = args.target
+ template_path = XOSProcessor._get_template(operator)
+ else:
+ template_path = operator
+
+ [template_folder, template_name] = os.path.split(template_path)
+ os_template_loader = jinja2.FileSystemLoader(searchpath=[template_folder])
+ os_template_env = jinja2.Environment(loader=os_template_loader)
+ os_template_env = XOSProcessor._load_jinja2_extensions(
+ os_template_env, args.attic
+ )
+ template = os_template_env.get_template(template_name)
+
if args.output is not None and args.write_to_file == "model":
rendered = {}
for i, model in enumerate(v.models):
diff --git a/lib/xos-genx/xosgenx/jinja2_extensions/base.py b/lib/xos-genx/xosgenx/jinja2_extensions/base.py
index 7e10e7d..fd4808e 100644
--- a/lib/xos-genx/xosgenx/jinja2_extensions/base.py
+++ b/lib/xos-genx/xosgenx/jinja2_extensions/base.py
@@ -318,15 +318,13 @@
def xproto_string_type(xptags):
- # FIXME: this try/except block assigns but never uses max_length?
- # try:
- # max_length = eval(xptags["max_length"])
- # except BaseException:
- # max_length = 1024
-
- if "varchar" not in xptags:
+ if "text" not in xptags:
+ # String fields have a mandatory maximum length.
+ # They are intended for relatively short strings.
return "string"
else:
+ # Text fields have an optional maximuim length.
+ # They are intended for long, potentially multiline strings.
return "text"
diff --git a/lib/xos-genx/xosgenx/jinja2_extensions/django.py b/lib/xos-genx/xosgenx/jinja2_extensions/django.py
index 6fac823..613462c 100644
--- a/lib/xos-genx/xosgenx/jinja2_extensions/django.py
+++ b/lib/xos-genx/xosgenx/jinja2_extensions/django.py
@@ -13,7 +13,7 @@
# limitations under the License.
from __future__ import absolute_import, print_function
-from .base import unquote
+from .base import unquote, xproto_string_type
import re
import sys
from six.moves import map
@@ -41,17 +41,23 @@
def django_string_type(xptags):
- try:
- max_length = eval(xptags["max_length"])
- except BaseException:
- max_length = 1024 * 1024
-
- if "content_type" in xptags:
- return django_content_type_string(xptags)
- elif int(max_length) < 1024 * 1024:
- return "CharField"
- else:
+ # xproto_string_type will return "string" if the options.text=False or "text" if options.text=True
+ xtype = xproto_string_type(xptags)
+ if xtype == "string":
+ if "content_type" in xptags:
+ return django_content_type_string(xptags)
+ else:
+ # TODO(smbaker): This is a workaround for incorrect xproto in many services. Prior behavior was to
+ # toggle between Charfield and Textfield when max_length was unspecified, rather than to require
+ # max_length to be specified. Remove this workaround as soon as services have been migrated.
+ if "max_length" in xptags:
+ return "CharField"
+ else:
+ return "TextField"
+ elif xtype == "text":
return "TextField"
+ else:
+ raise Exception("Unknown xproto_string type %s" % xtype, xptags=xptags)
def xproto_django_type(xptype, xptags):
diff --git a/lib/xos-genx/xosgenx/validator.py b/lib/xos-genx/xosgenx/validator.py
index bf73b84..d95c5d7 100644
--- a/lib/xos-genx/xosgenx/validator.py
+++ b/lib/xos-genx/xosgenx/validator.py
@@ -32,7 +32,7 @@
"feedback_state", "unique", "unique_with"]
# Options that must be either "True" or "False"
-BOOLEAN_OPTIONS = ["blank", "db_index", "feedback_state", "gui_hidden", "null", "tosca_key", "unique", "varchar"]
+BOOLEAN_OPTIONS = ["blank", "db_index", "feedback_state", "gui_hidden", "null", "tosca_key", "unique", "text"]
class XProtoValidator(object):
@@ -45,7 +45,7 @@
self.line_map = line_map
self.errors = []
- def error(self, model, field, message):
+ def error(self, model, field, message, severity="ERROR"):
if field and field.get("_linespan"):
error_first_line_number = field["_linespan"][0]
error_last_line_number = field["_linespan"][1]
@@ -61,7 +61,8 @@
error_filename = fn
error_line_offset = start_line
- self.errors.append({"model": model,
+ self.errors.append({"severity": severity,
+ "model": model,
"field": field,
"message": message,
"filename": error_filename,
@@ -69,6 +70,9 @@
"last_line_number": error_last_line_number - error_line_offset,
"absolute_line_number": error_first_line_number})
+ def warning(self, *args, **kwargs):
+ self.error(*args, severity="WARNING", **kwargs)
+
def print_errors(self):
# Sort by line number
for error in sorted(self.errors, key=lambda error: error["absolute_line_number"]):
@@ -83,12 +87,13 @@
else:
linestr = "%d" % first_line_number
- print("[ERROR] %s:%s %s.%s (Type %s): %s" % (os.path.basename(error["filename"]),
- linestr,
- model.get("name"),
- field.get("name"),
- field.get("type"),
- message), file=sys.stderr)
+ print("[%s] %s:%s %s.%s (Type %s): %s" % (error["severity"],
+ os.path.basename(error["filename"]),
+ linestr,
+ model.get("name"),
+ field.get("name"),
+ field.get("type"),
+ message), file=sys.stderr)
def is_option_true(self, field, name):
options = field.get("options")
@@ -185,7 +190,30 @@
self.check_modifier_consistent(model, field)
self.allow_options(model, field,
["blank", "choices", "content_type", "db_index", "default",
- "max_length", "modifier", "null", "varchar"])
+ "max_length", "modifier", "null", "text"])
+
+ # max_length is a mandatory argument of CharField.
+ if (content_type in [None]) and \
+ (not self.is_option_true(field, "text")) and \
+ ("max_length" not in field["options"]):
+ self.error(model, field, "String field should have a max_length or text=True")
+
+ if "max_length" in field["options"]:
+ max_length = field["options"]["max_length"]
+ try:
+ max_length = int(max_length)
+ if (max_length == 0):
+ self.error(model, field, "max_length should not be zero")
+
+ if 0 < abs(256-max_length) < 3:
+ self.warning(model, field,
+ "max_length of %s is close to suggested max_length of 256" % max_length)
+
+ if 0 < abs(1024-max_length) < 3:
+ self.warning(model, field,
+ "max_length of %s is close to suggested max_length of 1024" % max_length)
+ except ValueError:
+ self.error(model, field, "max_length must be a number")
def validate_field_bool(self, model, field):
self.check_modifier_consistent(model, field)
diff --git a/lib/xos-genx/xosgenx/xosgen.py b/lib/xos-genx/xosgenx/xosgen.py
index 608cae7..e223660 100755
--- a/lib/xos-genx/xosgenx/xosgen.py
+++ b/lib/xos-genx/xosgenx/xosgen.py
@@ -116,6 +116,14 @@
default=XOSProcessorArgs.default_checkers,
help="Comma-separated list of static checkers",
)
+group.add_argument(
+ "--lint",
+ dest="lint",
+ action="store_true",
+ default=XOSProcessorArgs.default_lint,
+ help="Parse the xproto but don't execute any xtargets",
+)
+
parse.add_argument(
"files",
@@ -135,6 +143,7 @@
CHECK = 1
GEN = 2
+LINT = 3
class XosGen:
@@ -148,17 +157,17 @@
if args.target:
op = GEN
subdir = "/targets/"
+ operators = [args.target]
elif args.checkers:
op = CHECK
subdir = "/checkers/"
+ operators = args.checkers
+ elif args.lint:
+ op = LINT
+ subdir = None
+ operators = []
else:
- parse.error("At least one of --target and --checkers is required")
-
- operators = (
- args.checkers.split(",")
- if hasattr(args, "checkers") and args.checkers
- else [args.target]
- )
+ parse.error("At least one of --target, --checkers, or --lint is required")
for i in range(len(operators)):
if "/" not in operators[i]:
@@ -202,9 +211,12 @@
'--dest-extension requires --write-to-file to be set to "model"'
)
- else:
+ elif op == CHECK:
if args.write_to_file or args.dest_extension:
parse.error("Checkers cannot write to files")
+ elif op == LINT:
+ # no outputs, so nothing to check
+ pass
inputs = []
@@ -238,3 +250,5 @@
exit(1)
else:
print("%s: OK" % o)
+ elif op == LINT:
+ XOSProcessor.process(args, None)