)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":16237,"name":"venkatamahesh","email":"venkatamaheshkotha@gmail.com","username":"venkatamahesh"},"change_message_id":"3056a013f8ec5195cb22e29afaae3b3aa8b2f810","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"ApiImpact: For image create/update 400 response will be returned."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Partial-Bug: 1465510"},{"line_number":18,"context_line":"Change-Id: I75f3316a1b9f433abf049fbc69aa5f597398ca50"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"9a1a9d01_42dfe368","line":17,"updated":"2015-09-29 17:00:09.000000000","message":"Partial-Bug: #1465510","commit_id":"16cf83ded856a802a50e27eecb286882c541835e"}],"glance/api/common.py":[{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"e0643a6a4b802ca89c819716b7372f1d8d410545","unresolved":false,"context_lines":[{"line_number":20,"context_line":"from oslo_log import log as logging"},{"line_number":21,"context_line":"from oslo_utils import excutils"},{"line_number":22,"context_line":"from oslo_utils import units"},{"line_number":23,"context_line":"from webob.exc import HTTPBadRequest"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"from glance.common import exception"},{"line_number":26,"context_line":"from glance.common import utils"}],"source_content_type":"text/x-python","patch_set":11,"id":"3a29b11f_ebae6097","line":23,"updated":"2015-10-20 09:12:22.000000000","message":"Looks like it contradict OS hacking guidelines: http://docs.openstack.org/developer/hacking/#imports.","commit_id":"e43d23701dd29cbc2ff98c0390fb4592a05196ed"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"9e5cee0cf62a48e3fe942117b5f7fb89d095fbba","unresolved":false,"context_lines":[{"line_number":20,"context_line":"from oslo_log import log as logging"},{"line_number":21,"context_line":"from oslo_utils import excutils"},{"line_number":22,"context_line":"from oslo_utils import units"},{"line_number":23,"context_line":"from webob.exc import HTTPBadRequest"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"from glance.common import exception"},{"line_number":26,"context_line":"from glance.common import utils"}],"source_content_type":"text/x-python","patch_set":11,"id":"3a29b11f_e5a6f60d","line":23,"in_reply_to":"3a29b11f_ebae6097","updated":"2015-10-23 06:41:22.000000000","message":"Done","commit_id":"e43d23701dd29cbc2ff98c0390fb4592a05196ed"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"e97a8d71920fc155e4d479b9b654c055409dd264","unresolved":false,"context_lines":[{"line_number":143,"context_line":"    return users_quota - usage"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"def validate_image_property_key_length(properties):"},{"line_number":147,"context_line":"    for prop in properties:"},{"line_number":148,"context_line":"        try:"},{"line_number":149,"context_line":"            utils.validate_string_length(prop, \"Key %s\" % prop,"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_bb3d742c","line":146,"updated":"2015-11-19 23:01:42.000000000","message":"Why don\u0027t we keep this in utils.py too? \nI\u0027m trying to understand if there\u0027s a specific reason to keep the \u0027validate_string_length\u0027 function in utils.py and have utils.py imported here?","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"4c5e459b893e60b20b675f219c23c5a056811360","unresolved":false,"context_lines":[{"line_number":143,"context_line":"    return users_quota - usage"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"def validate_image_property_key_length(properties):"},{"line_number":147,"context_line":"    for prop in properties:"},{"line_number":148,"context_line":"        try:"},{"line_number":149,"context_line":"            utils.validate_string_length(prop, \"Key %s\" % prop,"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_fc044015","line":146,"in_reply_to":"ba8a016a_bb3d742c","updated":"2015-11-20 11:56:37.000000000","message":"I have kept it in glance/api/common module, because validate_image_property_key_length method is getting called only from v1 and v2 api, which are in glance/api module only.","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"e97a8d71920fc155e4d479b9b654c055409dd264","unresolved":false,"context_lines":[{"line_number":146,"context_line":"def validate_image_property_key_length(properties):"},{"line_number":147,"context_line":"    for prop in properties:"},{"line_number":148,"context_line":"        try:"},{"line_number":149,"context_line":"            utils.validate_string_length(prop, \"Key %s\" % prop,"},{"line_number":150,"context_line":"                                         min_length\u003d1,"},{"line_number":151,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":152,"context_line":"        except exception.InvalidInput as e:"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_9e997ec6","line":149,"updated":"2015-11-19 23:01:42.000000000","message":"Let\u0027s say multiple properties violate the max length criteria. As of now, this change would barf on the very first property with a 400. When the user corrects that particular property name and re-tries the request, it barfs on the next one. And, so on.\n\nSo, from a user experience point-of-view, isn\u0027t it better that we catch all violations at once as opposed to reporting them one at a time?\n\nHow about we validate all properties and report all violations in one go?\nSomething like this..\n\n invalid_props \u003d list()\n for prop in properties:\n \ttry:\n \t\tutils.validate_string_length(prop, \"Key %s\" % prop,\n \t\t\t\t\t     min_length\u003d1,\n \t\t\t\t\t     max_length\u003dutils.MAX_STRING_LENGTH)\n \texcept exception.InvalidInput as e:\n \t\tinvalid_props.append(prop)\n \n if invalid_props:\n \tmsg \u003d _(\"One or more invalid image properties: %s. Property name can\u0027t \n                 exceed %s in length\u0027 % (\u0027,\u0027.join(invalid_props), utils.MAX_STRING_LENGTH))\n \traise webob.exc.HTTPBadRequest(explanation\u003dmsg)","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"4c5e459b893e60b20b675f219c23c5a056811360","unresolved":false,"context_lines":[{"line_number":146,"context_line":"def validate_image_property_key_length(properties):"},{"line_number":147,"context_line":"    for prop in properties:"},{"line_number":148,"context_line":"        try:"},{"line_number":149,"context_line":"            utils.validate_string_length(prop, \"Key %s\" % prop,"},{"line_number":150,"context_line":"                                         min_length\u003d1,"},{"line_number":151,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":152,"context_line":"        except exception.InvalidInput as e:"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_bc59d8fe","line":149,"in_reply_to":"ba8a016a_9e997ec6","updated":"2015-11-20 11:56:37.000000000","message":"1) As per your suggestion, if we catch all violations at once, in that case the error message would be very large.\n\nfor example: If there are 10 violation, where each key length is more than 255 characters, the error message to user would be very large (having more than 2550 characters).\n\n2) If we catch all violations at once, then it might happen that a malicious user can put unnecessary load on api controller by sending large number of create image request with hundreds of invalid keys with each request.\n\n3) I have taken reference of cinder project where it is implemented the same way by catching only one violation at a time. pl refer:\nhttps://github.com/openstack/cinder/blob/master/cinder/api/contrib/types_extra_specs.py#L81","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"91af84cdd5db251a79b5362bb596f6403866a08e","unresolved":false,"context_lines":[{"line_number":146,"context_line":"def validate_image_property_key_length(properties):"},{"line_number":147,"context_line":"    for prop in properties:"},{"line_number":148,"context_line":"        try:"},{"line_number":149,"context_line":"            utils.validate_string_length(prop, \"Key %s\" % prop,"},{"line_number":150,"context_line":"                                         min_length\u003d1,"},{"line_number":151,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":152,"context_line":"        except exception.InvalidInput as e:"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_77e1309a","line":149,"in_reply_to":"ba8a016a_bc59d8fe","updated":"2015-11-20 17:12:24.000000000","message":"Fair points here. Thanks for the response!","commit_id":"9140694104b971f962eb34119e193762d31323e3"}],"glance/api/v1/images.py":[{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"477a5ee7358716dd1f9964df80a24ecfe00e69bb","unresolved":false,"context_lines":[{"line_number":883,"context_line":""},{"line_number":884,"context_line":"        self._enforce_image_property_quota(image_meta, req\u003dreq)"},{"line_number":885,"context_line":""},{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_1a300c56","line":886,"updated":"2015-08-03 17:07:02.000000000","message":"To me, this belongs more in the deserializer. What do you think? Can we move this into the deserializer if you agree?","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"9a246d4768b8011b23fe69c8acec3bb03bc547ed","unresolved":false,"context_lines":[{"line_number":883,"context_line":""},{"line_number":884,"context_line":"        self._enforce_image_property_quota(image_meta, req\u003dreq)"},{"line_number":885,"context_line":""},{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_738844e8","line":886,"in_reply_to":"1a4dcd0f_1a300c56","updated":"2015-08-04 07:01:00.000000000","message":"In v1 api schema validation is not done in deserializer as it is done in v2 api. So IMO it is better place to do the validation.\n\nplease refer: \n[1] https://github.com/openstack/glance/blob/master/glance/api/v1/images.py#L1207\n\n[2] https://github.com/openstack/glance/blob/master/glance/api/v2/images.py#L362","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"42dccb2a93d5faeb89a893024ef51fc2b7815463","unresolved":false,"context_lines":[{"line_number":883,"context_line":""},{"line_number":884,"context_line":"        self._enforce_image_property_quota(image_meta, req\u003dreq)"},{"line_number":885,"context_line":""},{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_2d323911","line":886,"in_reply_to":"1a4dcd0f_738844e8","updated":"2015-08-04 15:13:17.000000000","message":"But we are trying to validate image meta in the deserializer [1].\nIn fact, we are indeed validating image name in a similar way [2]\n\nLet me know if I\u0027m missing anything here.\n\n[1] https://github.com/openstack/glance/blob/master/glance/api/v1/images.py#L1179\n\n[2] https://github.com/openstack/glance/blob/master/glance/api/v1/images.py#L108-L110","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"477a5ee7358716dd1f9964df80a24ecfe00e69bb","unresolved":false,"context_lines":[{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""},{"line_number":890,"context_line":"        image_meta \u003d self._reserve(req, image_meta)"},{"line_number":891,"context_line":"        id \u003d image_meta[\u0027id\u0027]"},{"line_number":892,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_d8b38c09","line":889,"updated":"2015-08-03 17:07:02.000000000","message":"Why are we passing \u0027Key\u0027 as the name? The error message may not be super intuitive. Why not just include the property name? It may be longer but won\u0027t it be more helpful? What do you think?","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"42dccb2a93d5faeb89a893024ef51fc2b7815463","unresolved":false,"context_lines":[{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""},{"line_number":890,"context_line":"        image_meta \u003d self._reserve(req, image_meta)"},{"line_number":891,"context_line":"        id \u003d image_meta[\u0027id\u0027]"},{"line_number":892,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_dcd109cf","line":889,"in_reply_to":"1a4dcd0f_2ed9f3f3","updated":"2015-08-04 15:13:17.000000000","message":"This is precisely the reason why I mentioned we shouldn\u0027t raise HTTPBadRequest from the utility method. If the utility method raises something like a InvalidStringLength excepion, the caller can then handle it and throw an appropriate error message to the user depending upon what\u0027s being validated, whether it\u0027s a key or value. If it\u0027s a key, we can also include the name in error message. If it\u0027s a value, we can say what property\u0027s value is too long.\n\nAs is, the error message is going to look like \"Key has more than 255 characters.\" This isn\u0027t very helpful to the user. Also, a \u0027key\u0027 may mean nothing to the user. It\u0027s better to be keep our error messages as clear and helpful as possible.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"9a246d4768b8011b23fe69c8acec3bb03bc547ed","unresolved":false,"context_lines":[{"line_number":886,"context_line":"        for property in image_meta[\u0027properties\u0027]:"},{"line_number":887,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":888,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":889,"context_line":""},{"line_number":890,"context_line":"        image_meta \u003d self._reserve(req, image_meta)"},{"line_number":891,"context_line":"        id \u003d image_meta[\u0027id\u0027]"},{"line_number":892,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_2ed9f3f3","line":889,"in_reply_to":"1a4dcd0f_d8b38c09","updated":"2015-08-04 07:01:00.000000000","message":"It is passed to make validate_string_length method generic, so that it can be used to validate key as well as values.\n\nBy looking at the message, it will be much more to user whether he need to modify key or value, which exceeds the limit.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"477a5ee7358716dd1f9964df80a24ecfe00e69bb","unresolved":false,"context_lines":[{"line_number":1004,"context_line":"                                           orig_image_meta\u003dorig_image_meta,"},{"line_number":1005,"context_line":"                                           purge_props\u003dpurge_props,"},{"line_number":1006,"context_line":"                                           req\u003dreq)"},{"line_number":1007,"context_line":"        for property in new_keys:"},{"line_number":1008,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":1009,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":1010,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_78c59859","line":1007,"updated":"2015-08-03 17:07:02.000000000","message":"Also, what if we have multiple properties that are longer than 255 characters. This is going to report only one property right? Do we want to report all properties that are longer than 255 characters or do we want to do that one-by-one?","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"9a246d4768b8011b23fe69c8acec3bb03bc547ed","unresolved":false,"context_lines":[{"line_number":1004,"context_line":"                                           orig_image_meta\u003dorig_image_meta,"},{"line_number":1005,"context_line":"                                           purge_props\u003dpurge_props,"},{"line_number":1006,"context_line":"                                           req\u003dreq)"},{"line_number":1007,"context_line":"        for property in new_keys:"},{"line_number":1008,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":1009,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":1010,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_cef2cf9a","line":1007,"in_reply_to":"1a4dcd0f_78c59859","updated":"2015-08-04 07:01:00.000000000","message":"We want to do it one-by-one, because there is no limit on number of key/value attributes that can be attached to an image.\n\nIf we are going to report all properties at once, the log message will be much bigger in case of a lot of properties.\n\nAnd it will flood log file unnecessary, if the same is done with multiple simultaneous requests.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"42dccb2a93d5faeb89a893024ef51fc2b7815463","unresolved":false,"context_lines":[{"line_number":1004,"context_line":"                                           orig_image_meta\u003dorig_image_meta,"},{"line_number":1005,"context_line":"                                           purge_props\u003dpurge_props,"},{"line_number":1006,"context_line":"                                           req\u003dreq)"},{"line_number":1007,"context_line":"        for property in new_keys:"},{"line_number":1008,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":1009,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":1010,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_c73616e7","line":1007,"in_reply_to":"1a4dcd0f_cef2cf9a","updated":"2015-08-04 15:13:17.000000000","message":"I understand your concern about log messages but I was thinking from a user experience point-of-view. It maybe a bad user experience.\n\nLet\u0027s see if the API WG has an opinion on this.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"477a5ee7358716dd1f9964df80a24ecfe00e69bb","unresolved":false,"context_lines":[{"line_number":1006,"context_line":"                                           req\u003dreq)"},{"line_number":1007,"context_line":"        for property in new_keys:"},{"line_number":1008,"context_line":"            utils.validate_string_length(property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":1009,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":1010,"context_line":""},{"line_number":1011,"context_line":"        try:"},{"line_number":1012,"context_line":"            if location:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_da1064ab","line":1009,"updated":"2015-08-03 17:07:02.000000000","message":"Same. This looks like a good candidate to move into the deserializer. Let me know if I\u0027m missing anything here","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"ac91d8fc3cbe81337d90b817bbb1d4a0515fc47a","unresolved":false,"context_lines":[{"line_number":123,"context_line":"                                         min_length\u003d1,"},{"line_number":124,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":125,"context_line":"        except exception.InvalidInput as e:"},{"line_number":126,"context_line":"            raise HTTPBadRequest(explanation\u003de.msg)"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    return values"},{"line_number":129,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_d4233b04","line":126,"updated":"2015-08-17 22:21:48.000000000","message":"I see that the other HTTPBadRequest exceptions pass requests to users. So it seems reasonable to do the same here.","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"ba0168d234fba39627993a669b580c68eb6164b1","unresolved":false,"context_lines":[{"line_number":123,"context_line":"                                         min_length\u003d1,"},{"line_number":124,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":125,"context_line":"        except exception.InvalidInput as e:"},{"line_number":126,"context_line":"            raise HTTPBadRequest(explanation\u003de.msg)"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    return values"},{"line_number":129,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_74194c21","line":126,"in_reply_to":"fa1b9901_d4233b04","updated":"2015-08-21 07:01:52.000000000","message":"I have checked and found that it doesn\u0027t change anything with or without passing request parameter to exception, so IMO it is not required.\n\nEven if we log this message, in that case also it wouldn\u0027t make any difference from user\u0027s point of view.\n\nIt is not even consistent across the module, there are occurrences where it is not passed.","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"},{"author":{"_account_id":7549,"name":"Ekaterina Chernova","email":"katyafervent@yandex.ru","username":"efedorova"},"change_message_id":"2f2453dc9f7968ddd9ac493037ae1bd535060e40","unresolved":false,"context_lines":[{"line_number":117,"context_line":"               checksum)"},{"line_number":118,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    for property in values[\u0027properties\u0027]:"},{"line_number":121,"context_line":"        try:"},{"line_number":122,"context_line":"            utils.validate_string_length(property, \"Key %s\" % property,"},{"line_number":123,"context_line":"                                         min_length\u003d1,"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_ca71c177","line":120,"updated":"2015-09-17 10:25:43.000000000","message":"values.get(\u0027properties\u0027, [])","commit_id":"27b93354b96138826162a3c14a374ee4018cd5ff"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"9b50c69ba74d7cccf3b0f5ee41e6d14fb650047c","unresolved":false,"context_lines":[{"line_number":118,"context_line":"               checksum)"},{"line_number":119,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    for property in values.get(\u0027properties\u0027, []):"},{"line_number":122,"context_line":"        try:"},{"line_number":123,"context_line":"            utils.validate_string_length(property, \"Key %s\" % property,"},{"line_number":124,"context_line":"                                         min_length\u003d1,"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_3f80a0f4","line":121,"updated":"2015-10-09 10:37:22.000000000","message":"nit: could you use `prop` instead of `property` ? The later is a builtin decorator","commit_id":"6a6de02610afc0a8490cef55a2a6d11214e3fc1f"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"07d6d3eb75426bd36ba2bf6f443fd5faa71f9582","unresolved":false,"context_lines":[{"line_number":119,"context_line":"               checksum)"},{"line_number":120,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    common.validate_image_metadata(values.get(\u0027properties\u0027, {}))"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return values"},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3a29b11f_b35919ff","line":122,"updated":"2015-10-23 12:54:07.000000000","message":"validate_image_metadata inside validate_image_meta function looks like a bad naming.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"07d6d3eb75426bd36ba2bf6f443fd5faa71f9582","unresolved":false,"context_lines":[{"line_number":120,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    common.validate_image_metadata(values.get(\u0027properties\u0027, {}))"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return values"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3a29b11f_73a14193","line":123,"updated":"2015-10-23 12:54:07.000000000","message":"Also should properties be a dictionary(I see dict by default)? If yes then it seems that validate_image_metadata expects a list and we should get an error.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"9a2e6e85cc15d86e21b639c9081489d1a6018bf6","unresolved":false,"context_lines":[{"line_number":120,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    common.validate_image_metadata(values.get(\u0027properties\u0027, {}))"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return values"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_91977947","line":123,"in_reply_to":"1a26ad4f_a063dcb0","updated":"2015-11-03 07:56:47.000000000","message":"Yep, you are right, needed more coffee when did previous review:)","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"0d5b4ad0dcdced3d29ea50db4b209b8de943f618","unresolved":false,"context_lines":[{"line_number":120,"context_line":"        raise HTTPBadRequest(explanation\u003dmsg, request\u003dreq)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    common.validate_image_metadata(values.get(\u0027properties\u0027, {}))"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    return values"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_a063dcb0","line":123,"in_reply_to":"3a29b11f_73a14193","updated":"2015-10-28 07:00:15.000000000","message":"validate_image_metadata expects dictionary and it iterates on dictionary keys and calls validate_string_length method with appropriate args.\n\nIMO because of for loop, it seems like it takes list as argument. right ??","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"}],"glance/api/v2/images.py":[{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"226915946c830cea9b0a49d14feaa451f22018aa","unresolved":false,"context_lines":[{"line_number":384,"context_line":"        for property in properties:"},{"line_number":385,"context_line":"            try:"},{"line_number":386,"context_line":"                utils.validate_string_length("},{"line_number":387,"context_line":"                    property, \u0027Key\u0027, min_length\u003d1,"},{"line_number":388,"context_line":"                    max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":389,"context_line":"            except exception.InvalidInput as e:"},{"line_number":390,"context_line":"                raise webob.exc.HTTPBadRequest(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_99c73c64","line":387,"updated":"2015-08-08 17:02:42.000000000","message":"Why are you using `\u0027Key\u0027` here? Don\u0027t we have the property name?","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"226915946c830cea9b0a49d14feaa451f22018aa","unresolved":false,"context_lines":[{"line_number":502,"context_line":"            partial_image \u003d {path_root: [change[\u0027value\u0027]]}"},{"line_number":503,"context_line":""},{"line_number":504,"context_line":"        try:"},{"line_number":505,"context_line":"            utils.validate_string_length(path_root, \u0027Key\u0027, min_length\u003d1,"},{"line_number":506,"context_line":"                                         max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":507,"context_line":"        except exception.InvalidInput as e:"},{"line_number":508,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_b9c4785f","line":505,"updated":"2015-08-08 17:02:42.000000000","message":"ditto","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"9b50c69ba74d7cccf3b0f5ee41e6d14fb650047c","unresolved":false,"context_lines":[{"line_number":384,"context_line":"            except KeyError:"},{"line_number":385,"context_line":"                pass"},{"line_number":386,"context_line":""},{"line_number":387,"context_line":"        for property in properties:"},{"line_number":388,"context_line":"            try:"},{"line_number":389,"context_line":"                utils.validate_string_length("},{"line_number":390,"context_line":"                    property, \"Key %s\" % property, min_length\u003d1,"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_bf92704a","line":387,"updated":"2015-10-09 10:37:22.000000000","message":"ditto\n\n\nAre we sure this is needed here? Aren\u0027t custom properties handled using jsonschema? I\u0027d expect this to be verifiable through jsonschema directly.\n\nAlso, this code is the same for both v1 and v2. Any chance we can move it to a common place?","commit_id":"6a6de02610afc0a8490cef55a2a6d11214e3fc1f"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"e97a8d71920fc155e4d479b9b654c055409dd264","unresolved":false,"context_lines":[{"line_number":25,"context_line":"import six.moves.urllib.parse as urlparse"},{"line_number":26,"context_line":"import webob.exc"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"from glance.api import common"},{"line_number":29,"context_line":"from glance.api import policy"},{"line_number":30,"context_line":"from glance.common import exception"},{"line_number":31,"context_line":"from glance.common import location_strategy"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_deb4e682","line":28,"updated":"2015-11-19 23:01:42.000000000","message":"We don\u0027t have to import common here if our validation method resides in utils.py.","commit_id":"9140694104b971f962eb34119e193762d31323e3"}],"glance/common/artifacts/declarative.py":[{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"ac91d8fc3cbe81337d90b817bbb1d4a0515fc47a","unresolved":false,"context_lines":[{"line_number":548,"context_line":"        self.type_description \u003d type_description"},{"line_number":549,"context_line":"        self.endpoint \u003d endpoint or type_name.lower()"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self._validate_string(self.type_name, \u0027Type name\u0027, min_length\u003d1,"},{"line_number":552,"context_line":"                              max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":553,"context_line":"        self._validate_string(self.type_display_name, \u0027Type display name\u0027,"},{"line_number":554,"context_line":"                              max_length\u003dutils.MAX_STRING_LENGTH)"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_36ab9c8b","line":551,"updated":"2015-08-17 22:21:48.000000000","message":"IMO, we have code duplication in _validate_string method and utils method you propose. The best solution would be combine these two methods together as one single method and use it in the other parts. I would also recommend to rename the exception for InvalidInput to something more reasonable (StringValidationFailed or sm else.)","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"ba0168d234fba39627993a669b580c68eb6164b1","unresolved":false,"context_lines":[{"line_number":548,"context_line":"        self.type_description \u003d type_description"},{"line_number":549,"context_line":"        self.endpoint \u003d endpoint or type_name.lower()"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self._validate_string(self.type_name, \u0027Type name\u0027, min_length\u003d1,"},{"line_number":552,"context_line":"                              max_length\u003dutils.MAX_STRING_LENGTH)"},{"line_number":553,"context_line":"        self._validate_string(self.type_display_name, \u0027Type display name\u0027,"},{"line_number":554,"context_line":"                              max_length\u003dutils.MAX_STRING_LENGTH)"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_14dfb03a","line":551,"in_reply_to":"fa1b9901_36ab9c8b","updated":"2015-08-21 07:01:52.000000000","message":"Done","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"}],"glance/common/utils.py":[{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"477a5ee7358716dd1f9964df80a24ecfe00e69bb","unresolved":false,"context_lines":[{"line_number":84,"context_line":"    \"\"\""},{"line_number":85,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":86,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % name"},{"line_number":87,"context_line":"        raise exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    if len(value) \u003c min_length:"},{"line_number":90,"context_line":"        msg \u003d _(\"%(name)s has a minimum character requirement of \""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_586c5c9c","line":87,"updated":"2015-08-03 17:07:02.000000000","message":"I\u0027m not sure if this method should raise a \u0027HttpBadRequest\u0027 exception. This method is tasked with validating strings and that is all it should do. In the process it may throw it\u0027s own exceptions related to string validation like \u0027InvalidStringType\u0027 or \u0027InvalidStringLength\u0027 but not \u0027HttpBadRequest\u0027. Doing so, we let the caller handle the exceptions appropriately, which could be to raise \u0027HttpBadRequest\u0027 after all. But, as a utility method itself, I don\u0027t think it should raise a \u0027HttpBadRequest\u0027 exception. Here are my two reasons:\n\n1. This may limit the reusability of this utility method because as a caller I may or may not want to raise a HttpBadRequest. All I want you to tell me is that whether or not a string is valid.\n\n2. This also suggests that your utility method knows about the API behavior. We may not want that, right?\n\nLet me know what you think.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"9a246d4768b8011b23fe69c8acec3bb03bc547ed","unresolved":false,"context_lines":[{"line_number":84,"context_line":"    \"\"\""},{"line_number":85,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":86,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % name"},{"line_number":87,"context_line":"        raise exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    if len(value) \u003c min_length:"},{"line_number":90,"context_line":"        msg \u003d _(\"%(name)s has a minimum character requirement of \""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_8e63671a","line":87,"in_reply_to":"1a4dcd0f_586c5c9c","updated":"2015-08-04 07:01:00.000000000","message":"Hi,\n\nI have reffered code in below link from glance/common/utils.py where HttpBadRequest is raised.\n\nhttps://github.com/openstack/glance/blob/master/glance/common/utils.py#L300","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"42dccb2a93d5faeb89a893024ef51fc2b7815463","unresolved":false,"context_lines":[{"line_number":84,"context_line":"    \"\"\""},{"line_number":85,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":86,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % name"},{"line_number":87,"context_line":"        raise exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    if len(value) \u003c min_length:"},{"line_number":90,"context_line":"        msg \u003d _(\"%(name)s has a minimum character requirement of \""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_6d7841f3","line":87,"in_reply_to":"1a4dcd0f_8e63671a","updated":"2015-08-04 15:13:17.000000000","message":"Thanks for the link. I see HttpBadRequest being raised there. There maybe a few occurrences here and there. Some may make sense. However, for this case, I don\u0027t recommend raising a HttpBadRequest.\n\nPlease refer to my comment on v1/images.py for one reason why we shouldn\u0027t raise HttpBadRequest.","commit_id":"22616ad7266e26b3c561bbdf9eb8fbd3efcb487b"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"226915946c830cea9b0a49d14feaa451f22018aa","unresolved":false,"context_lines":[{"line_number":73,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def validate_string_length(value, name, min_length\u003d0, max_length\u003dNone):"},{"line_number":77,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    :param value: the value of the string"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_59a0d4a5","line":76,"updated":"2015-08-08 17:02:42.000000000","message":"I\u0027m not a fan of passing the `name` just to have it in the logs since it\u0027s not really useful for this function.","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"6209ed93d9fc69bcca9b90a8172f6f164e134576","unresolved":false,"context_lines":[{"line_number":73,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def validate_string_length(value, name, min_length\u003d0, max_length\u003dNone):"},{"line_number":77,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    :param value: the value of the string"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_1af2239b","line":76,"in_reply_to":"1a4dcd0f_59a0d4a5","updated":"2015-08-10 09:26:49.000000000","message":"I have made this as generic function which can be used to validate any string value, so that log message contain enough information to decide whether the invalid value is key or value or any other string property.\n\nfor eg. if first if condition fails: \n1) If you want to validate only key, in that case the \u0027name\u0027 param will be equal to \u0027Key \u003ckey_name\u003e\u0027 and log message will be like:\n\n\"Key \u003ckey_name\u003e is not a string or unicode.\"\n\n2) If you want to validate only value, in that case the \u0027name\u0027 param will be equal to \u0027Value of key \u003ckey_name\u003e\u0027 and log message will be like:\n\n\"Value for key \u003ckey_name\u003e is not a string or unicode.\"","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"226915946c830cea9b0a49d14feaa451f22018aa","unresolved":false,"context_lines":[{"line_number":76,"context_line":"def validate_string_length(value, name, min_length\u003d0, max_length\u003dNone):"},{"line_number":77,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    :param value: the value of the string"},{"line_number":80,"context_line":"    :param name: the name of the string"},{"line_number":81,"context_line":"    :param min_length: the min_length of the string"},{"line_number":82,"context_line":"    :param max_length: the max_length of the string"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_79a590b3","line":79,"updated":"2015-08-08 17:02:42.000000000","message":"\"of the property\" ?","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"226915946c830cea9b0a49d14feaa451f22018aa","unresolved":false,"context_lines":[{"line_number":77,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    :param value: the value of the string"},{"line_number":80,"context_line":"    :param name: the name of the string"},{"line_number":81,"context_line":"    :param min_length: the min_length of the string"},{"line_number":82,"context_line":"    :param max_length: the max_length of the string"},{"line_number":83,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_19aa4c86","line":80,"updated":"2015-08-08 17:02:42.000000000","message":"ditto","commit_id":"a7ab6ce37040a446f6f07b360a7b88d6d752ad4b"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"ac91d8fc3cbe81337d90b817bbb1d4a0515fc47a","unresolved":false,"context_lines":[{"line_number":68,"context_line":"                      \u0027x-image-meta-virtual_size\u0027]"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"GLANCE_TEST_SOCKET_FD_STR \u003d \u0027GLANCE_TEST_SOCKET_FD\u0027"},{"line_number":71,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_56aad0d5","line":71,"updated":"2015-08-17 22:21:48.000000000","message":"I think we need a better explanation of this global variable. Could you please add some comment about it and what cause this value to be 255? It is important because this variable is used in many places but the dependency is not obvious.","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"ba0168d234fba39627993a669b580c68eb6164b1","unresolved":false,"context_lines":[{"line_number":68,"context_line":"                      \u0027x-image-meta-virtual_size\u0027]"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"GLANCE_TEST_SOCKET_FD_STR \u003d \u0027GLANCE_TEST_SOCKET_FD\u0027"},{"line_number":71,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_74f06cc9","line":71,"in_reply_to":"fa1b9901_56aad0d5","updated":"2015-08-21 07:01:52.000000000","message":"Done","commit_id":"86eb40e59c0d7193381e6cd276554526f93f4e3c"},{"author":{"_account_id":7549,"name":"Ekaterina Chernova","email":"katyafervent@yandex.ru","username":"efedorova"},"change_message_id":"2f2453dc9f7968ddd9ac493037ae1bd535060e40","unresolved":false,"context_lines":[{"line_number":84,"context_line":"    \"\"\""},{"line_number":85,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":86,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % property_name"},{"line_number":87,"context_line":"        raise exception.InvalidInput(message\u003dmsg)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    if len(value) \u003c min_length:"},{"line_number":90,"context_line":"        msg \u003d _(\"%(property_name)s has a minimum character requirement of \""}],"source_content_type":"text/x-python","patch_set":7,"id":"ba15a1d1_6a2d15a7","line":87,"updated":"2015-09-17 10:25:43.000000000","message":"it seems to me \u0027reason\u0027 should be here instead of message regarding you message attribute of new InvalidInput. Please, correct me I\u0027m wrong","commit_id":"27b93354b96138826162a3c14a374ee4018cd5ff"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"07d6d3eb75426bd36ba2bf6f443fd5faa71f9582","unresolved":false,"context_lines":[{"line_number":70,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"},{"line_number":74,"context_line":"                           max_length\u003dNone):"},{"line_number":75,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":76,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3a29b11f_b39a7960","line":73,"updated":"2015-10-23 12:54:07.000000000","message":"I am also thinking if there is any reason to require property name.\nIf we can generate some default value for it then it can be used not for property validation only.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"9a2e6e85cc15d86e21b639c9081489d1a6018bf6","unresolved":false,"context_lines":[{"line_number":70,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"},{"line_number":74,"context_line":"                           max_length\u003dNone):"},{"line_number":75,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":76,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_d1b17102","line":73,"in_reply_to":"1a26ad4f_c01db867","updated":"2015-11-03 07:56:47.000000000","message":"Yep, you are right, I got fooled by property_name naming (though it is always should be image property name.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"0d5b4ad0dcdced3d29ea50db4b209b8de943f618","unresolved":false,"context_lines":[{"line_number":70,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"},{"line_number":74,"context_line":"                           max_length\u003dNone):"},{"line_number":75,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":76,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_c01db867","line":73,"in_reply_to":"3a29b11f_b39a7960","updated":"2015-10-28 07:00:15.000000000","message":"property_name is passed from where validate_string_length method is called. It will be of following format:\n\nIf we add key/value pair as below:\n{\u0027abc\u0027, 123, \u0027xyz\u0027: 456}\n\nIn our case, property_name will be \u0027Key abc\u0027 and \u0027Key xyz\u0027 when checking length of key of property. \n\nIt will be helpful in debugging log messages and get to know easily which keys are not valid.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"07d6d3eb75426bd36ba2bf6f443fd5faa71f9582","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    :param min_length: the min_length of the string"},{"line_number":80,"context_line":"    :param max_length: the max_length of the string"},{"line_number":81,"context_line":"    \"\"\""},{"line_number":82,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":83,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % property_name"},{"line_number":84,"context_line":"        raise exception.InvalidInput(reason\u003dmsg)"},{"line_number":85,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3a29b11f_73db817c","line":82,"updated":"2015-10-23 12:54:07.000000000","message":"I am wondering if the name of the type will be useful when debugging application.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"9a2e6e85cc15d86e21b639c9081489d1a6018bf6","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    :param min_length: the min_length of the string"},{"line_number":80,"context_line":"    :param max_length: the max_length of the string"},{"line_number":81,"context_line":"    \"\"\""},{"line_number":82,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":83,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % property_name"},{"line_number":84,"context_line":"        raise exception.InvalidInput(reason\u003dmsg)"},{"line_number":85,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_310c65ec","line":82,"in_reply_to":"1a26ad4f_1d7557ca","updated":"2015-11-03 07:56:47.000000000","message":"I mean that it would be useful to have written the type of property in exception. Something like %s property expected to be string got \u003ctype_name\u003e","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"0d5b4ad0dcdced3d29ea50db4b209b8de943f618","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    :param min_length: the min_length of the string"},{"line_number":80,"context_line":"    :param max_length: the max_length of the string"},{"line_number":81,"context_line":"    \"\"\""},{"line_number":82,"context_line":"    if not isinstance(value, six.string_types):"},{"line_number":83,"context_line":"        msg \u003d _(\"%s is not a string or unicode\") % property_name"},{"line_number":84,"context_line":"        raise exception.InvalidInput(reason\u003dmsg)"},{"line_number":85,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a26ad4f_1d7557ca","line":82,"in_reply_to":"3a29b11f_73db817c","updated":"2015-10-28 07:00:15.000000000","message":"If we will use property_name in logs, then we will get to know which property key is invalid.","commit_id":"b66ff295fb52d9a918799e934a9983605a044632"},{"author":{"_account_id":8158,"name":"Hemanth Makkapati","email":"makstacker+osgerrit@gmail.com","username":"hemanth.makkapati"},"change_message_id":"e97a8d71920fc155e4d479b9b654c055409dd264","unresolved":false,"context_lines":[{"line_number":70,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"},{"line_number":74,"context_line":"                           max_length\u003dNone):"},{"line_number":75,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":76,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_b13d932c","line":73,"updated":"2015-11-19 23:01:42.000000000","message":"Personally, I don\u0027t like the argument property_name here to indicate if it\u0027s a key/type name/type display name. From this method\u0027s perspective, it\u0027s irrelevant what it is trying to validate. All it cares about is, whether or not a given piece of data is a string under certain length. If it\u0027s not, it should raise an appropriate exception. \n\nI see that the reason you are accepting the property_name argument is to come up with a meaningful error message for the end-user. But, I don\u0027t think that is the responsibility of this utility function. We should let the caller formulate the appropriate error message that the end-user would see like in the code sample I provided in common.py.\n\nThat way, this function would be more crisp and avoids confusion too. What do you think?","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":10795,"name":"Rajesh Tailor","email":"tailor.rajesh88@gmail.com","username":"rajeshtailor"},"change_message_id":"c1a1ee2436f753740e9432a93e53cd01c10ea711","unresolved":false,"context_lines":[{"line_number":70,"context_line":"MAX_STRING_LENGTH \u003d 255"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def validate_string_length(value, property_name, min_length\u003d0,"},{"line_number":74,"context_line":"                           max_length\u003dNone):"},{"line_number":75,"context_line":"    \"\"\"Check the length of specified string."},{"line_number":76,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ba8a016a_a8687322","line":73,"in_reply_to":"ba8a016a_b13d932c","updated":"2015-11-23 06:54:10.000000000","message":"Hi Hemanth,\n\nI have made this as generic function which can be used to validate any string value, so that log message contain enough information to decide whether the invalid value is key or value or any other string property.\n\nfor eg. if first if condition fails: 1) If you want to validate only key, in that case the \u0027name\u0027 param will be equal to \u0027Key \u003ckey_name\u003e\u0027 and log message will be like:\n\n\"Key \u003ckey_name\u003e is not a string or unicode.\"\n\n2) If you want to validate only value, in that case the \u0027name\u0027 param will be equal to \u0027Value of key \u003ckey_name\u003e\u0027 and log message will be like:\n\n\"Value for key \u003ckey_name\u003e is not a string or unicode.\"\n\nI have already replied the same to Flavio for his comment in PS #3.\n\npl refer: https://review.openstack.org/#/c/203948/3/glance/common/utils.py","commit_id":"9140694104b971f962eb34119e193762d31323e3"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"02e7780538f3cc639a0073b72b58d2a596bea0ae","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                                   min_length\u003d1,"},{"line_number":104,"context_line":"                                   max_length\u003dMAX_STRING_LENGTH)"},{"line_number":105,"context_line":"        except exception.InvalidInput as e:"},{"line_number":106,"context_line":"            raise exc.HTTPBadRequest(explanation\u003de.msg)"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"def chunkreadable(iter, chunk_size\u003d65536):"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba8a016a_58f5f1c8","line":106,"updated":"2015-11-23 17:34:01.000000000","message":"I saw your discussion with Hemanth on an earlier patch set about not throwing the exception until all the properties\u0027 lengths had been checked.  It makes me wonder whether we really need to include the key in the message of this exception.  It would make the message more helpful, but on the other hand, the user should be able easily to figure out which is the bad property name, namely, start with the longest one!  Your argument on the earlier patch set about not including all the bad property names applies to a single enormous property name, too.  I\u0027d suggest that instead of using the message that comes back in the InvalidInput exception raised by validate_string_length(), in this situation replace the message with something more neutral such as, \"A property name may not exceed %(MAX_STRING_LENGTH) characters.\"","commit_id":"f50fa8ece1f048d8051e8b98ac2656cb785d119e"}],"glance/tests/unit/v1/test_api.py":[{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"b42ff583fa0ba61014334949daf39e56cee46fca","unresolved":false,"context_lines":[{"line_number":4612,"context_line":"        output \u003d another_request.get_response(self.api)"},{"line_number":4613,"context_line":"        self.assertEqual(400, output.status_int)"},{"line_number":4614,"context_line":""},{"line_number":4615,"context_line":"    def test_update_image_with_max_property_name_limit(self):"},{"line_number":4616,"context_line":"        self.config(image_property_quota\u003d1)"},{"line_number":4617,"context_line":"        image_id \u003d self._create_admin_image()"},{"line_number":4618,"context_line":"        another_request \u003d unit_test_utils.get_fake_request("}],"source_content_type":"text/x-python","patch_set":8,"id":"ba15a1d1_ff6297d7","line":4615,"updated":"2015-09-22 13:06:03.000000000","message":"I am wondering if we need some tests for empty property values.\nf.e. the property value is \u0027\u0027.\nCould you please explain why it is absent here?\nIf there is some reason I will +1 to this patch.","commit_id":"63cd0bd9a526d17f7085ace9bb364549d477b771"}]}