)]}'
{"nova/objects/build_request.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"22f897dbbab2bf5e32cd5bb6fb1c4f4bafb34f34","unresolved":false,"context_lines":[{"line_number":219,"context_line":"                                  db_build_reqs)"},{"line_number":220,"context_line":""},{"line_number":221,"context_line":"    @base.remotable_classmethod"},{"line_number":222,"context_line":"    def get_by_filters(cls, context, filters, limit\u003dNone, marker\u003dNone,"},{"line_number":223,"context_line":"                       sort_keys\u003dNone, sort_dirs\u003dNone):"},{"line_number":224,"context_line":"        if limit \u003d\u003d 0:"},{"line_number":225,"context_line":"            return cls(context, objects\u003d[])"}],"source_content_type":"text/x-python","patch_set":1,"id":"3ac371cc_f84ce071","line":222,"updated":"2016-08-16 21:25:12.000000000","message":"This method is ridiculous, I know. I plan to split out exact filter matching, regex matching, and sorting. It was just easier to develop and test this way initially.","commit_id":"a3732e5bbbc734f77224035837e4fee01042ae2c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3766ebd222683e87a8376093b952e51a70d73705","unresolved":false,"context_lines":[{"line_number":235,"context_line":"                        for k, v in item.items():"},{"line_number":236,"context_line":"                            if (k not in instance.metadata or"},{"line_number":237,"context_line":"                                    v !\u003d instance.metadata[k]):"},{"line_number":238,"context_line":"                                keep_instance \u003d False"},{"line_number":239,"context_line":"                                break"},{"line_number":240,"context_line":"                else:"},{"line_number":241,"context_line":"                    for k, v in filter_val.items():"},{"line_number":242,"context_line":"                        if (k not in instance.metadata or"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_c874ab69","line":239,"range":{"start_line":238,"start_character":32,"end_line":239,"end_character":37},"updated":"2016-08-22 21:22:09.000000000","message":"So this will break out of the loop from L235 and continue the outer loop at L234. It feels like we need to just have multiple return points, unfortunately. Otherwise it seems like it would get complicated trying to exit this function early without doing extra loops.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"e894c3d1b4013dd3d848656e785d62c414ba3531","unresolved":false,"context_lines":[{"line_number":235,"context_line":"                        for k, v in item.items():"},{"line_number":236,"context_line":"                            if (k not in instance.metadata or"},{"line_number":237,"context_line":"                                    v !\u003d instance.metadata[k]):"},{"line_number":238,"context_line":"                                keep_instance \u003d False"},{"line_number":239,"context_line":"                                break"},{"line_number":240,"context_line":"                else:"},{"line_number":241,"context_line":"                    for k, v in filter_val.items():"},{"line_number":242,"context_line":"                        if (k not in instance.metadata or"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_8be7763f","line":239,"range":{"start_line":238,"start_character":32,"end_line":239,"end_character":37},"in_reply_to":"1ac06dbe_c874ab69","updated":"2016-08-25 15:10:19.000000000","message":"Returning here would be an optimization. Continuing the outer loop won\u0027t change the result since keep_instance can\u0027t be set back to True, but yeah we don\u0027t need to keep going here.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3766ebd222683e87a8376093b952e51a70d73705","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                            keep_instance \u003d False"},{"line_number":245,"context_line":"                            break"},{"line_number":246,"context_line":"            elif isinstance(filter_val, (list, tuple, set, frozenset)):"},{"line_number":247,"context_line":"                if not filter_val:"},{"line_number":248,"context_line":"                    break"},{"line_number":249,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_e8724710","line":248,"range":{"start_line":247,"start_character":16,"end_line":248,"end_character":25},"updated":"2016-08-22 21:22:09.000000000","message":"This will mean keep_instance \u003d True. Is that intended?","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"06afa0215e577ca1201ab90cb2f0d8b66955a0f1","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                            keep_instance \u003d False"},{"line_number":245,"context_line":"                            break"},{"line_number":246,"context_line":"            elif isinstance(filter_val, (list, tuple, set, frozenset)):"},{"line_number":247,"context_line":"                if not filter_val:"},{"line_number":248,"context_line":"                    break"},{"line_number":249,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_72d66a28","line":248,"range":{"start_line":247,"start_character":16,"end_line":248,"end_character":25},"in_reply_to":"1ac06dbe_4fe8e910","updated":"2016-08-25 15:17:25.000000000","message":"So I totally read this wrong in the original code. The current code \"short circuits\" if it would have done \"IN \u003cempty set\u003e\", which I thought was basically ignoring that filter. I didn\u0027t catch that it was doing a return there instead of a break, which is a complete bail on the whole thing. Instead of ignoring that filter it bails entirely and immediately signals \"no instances could possibly match this\". I definitely did not get that when I read it the first time and figured the same as laski here I guess.\n\nSo yeah, I guess instead of break here you too can bail hard to the caller.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"e894c3d1b4013dd3d848656e785d62c414ba3531","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                            keep_instance \u003d False"},{"line_number":245,"context_line":"                            break"},{"line_number":246,"context_line":"            elif isinstance(filter_val, (list, tuple, set, frozenset)):"},{"line_number":247,"context_line":"                if not filter_val:"},{"line_number":248,"context_line":"                    break"},{"line_number":249,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_4fe8e910","line":248,"range":{"start_line":247,"start_character":16,"end_line":248,"end_character":25},"in_reply_to":"1ac06dbe_e8724710","updated":"2016-08-25 15:10:19.000000000","message":"So I though it matched the behavior of what the instance query will do in http://git.openstack.org/cgit/openstack/nova/tree/nova/db/sqlalchemy/api.py#n2380 but after looking again I see that it does the opposite. Nice catch.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3766ebd222683e87a8376093b952e51a70d73705","unresolved":false,"context_lines":[{"line_number":246,"context_line":"            elif isinstance(filter_val, (list, tuple, set, frozenset)):"},{"line_number":247,"context_line":"                if not filter_val:"},{"line_number":248,"context_line":"                    break"},{"line_number":249,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"},{"line_number":252,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_9c0dd4ea","line":249,"updated":"2016-08-22 21:22:09.000000000","message":"Does this need to be something more like \"if instance has the filter key, then check if it\u0027s not in the values\" i.e.\n\n if filter_key in instance and instance[filter_key] not in filter_val:\n\nI\u0027m wondering about the case where the key is not in instance and the comparison becomes \"if None not in filter_val\" seems confusing.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"e894c3d1b4013dd3d848656e785d62c414ba3531","unresolved":false,"context_lines":[{"line_number":246,"context_line":"            elif isinstance(filter_val, (list, tuple, set, frozenset)):"},{"line_number":247,"context_line":"                if not filter_val:"},{"line_number":248,"context_line":"                    break"},{"line_number":249,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"},{"line_number":252,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_4f82a995","line":249,"in_reply_to":"1ac06dbe_9c0dd4ea","updated":"2016-08-25 15:10:19.000000000","message":"I was basing this off of http://git.openstack.org/cgit/openstack/nova/tree/nova/db/sqlalchemy/api.py#n2384 where the filter will still be checked even if the instance has no value for the filter_key.\n\nIf I wasn\u0027t attempting to copy current behavior then what you suggest would certainly be better.","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3766ebd222683e87a8376093b952e51a70d73705","unresolved":false,"context_lines":[{"line_number":250,"context_line":"                    keep_instance \u003d False"},{"line_number":251,"context_line":"                    break"},{"line_number":252,"context_line":"            else:"},{"line_number":253,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"},{"line_number":254,"context_line":"                    keep_instance \u003d False"},{"line_number":255,"context_line":"                    break"},{"line_number":256,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_7c9c585d","line":253,"updated":"2016-08-22 21:22:09.000000000","message":"Same comment","commit_id":"dfb2ed4c18f6cf3a7de6119536aea6e9cddf690c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ce521098e511a701dd3247ee2f2c621c9e6d5d11","unresolved":false,"context_lines":[{"line_number":241,"context_line":"                if not filter_val:"},{"line_number":242,"context_line":"                    # Special value to indicate that nothing will match."},{"line_number":243,"context_line":"                    return None"},{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"}],"source_content_type":"text/x-python","patch_set":9,"id":"1ac06dbe_8c74a29c","line":244,"range":{"start_line":244,"start_character":16,"end_line":244,"end_character":68},"updated":"2016-08-25 18:58:06.000000000","message":"I was thinking about this again and realized the existing code does in fact require that filter key is present before filtering value by the fact that it\u0027s done in two query.filter calls [1]. The first call will filter/keep only instances that match the key and the second call will filter/keep only instances that match the value. That is, only instances that match the key will make it past the first call, so it is required that both match to keep the instance.\n\nSo I think this does need to be something more like:\n\n if filter key in instance and instance[filter_key] not in filter_val:\n\n[1] https://github.com/openstack/nova/blob/51575f/nova/db/sqlalchemy/api.py#L2373-L2374","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"0f4efd877301f0aae43b537646571efd01c6b370","unresolved":false,"context_lines":[{"line_number":241,"context_line":"                if not filter_val:"},{"line_number":242,"context_line":"                    # Special value to indicate that nothing will match."},{"line_number":243,"context_line":"                    return None"},{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa7ab95a_45483c2e","line":244,"range":{"start_line":244,"start_character":16,"end_line":244,"end_character":68},"in_reply_to":"1ac06dbe_8c74a29c","updated":"2016-08-25 20:09:42.000000000","message":"That code lines up with lines 232/233 above. This lines up with lines 2384/2385 in sqlalchemy/api.py which checks that the key is valid on the db model and then tests the value. But it never checks that the key is set on the instance, just that it\u0027s a valid column. In this case if it was not a valid field we would get an exception just as that code would in getattr.","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5bdc4bda7018b657d6b3b82d4b7342d23e04887f","unresolved":false,"context_lines":[{"line_number":241,"context_line":"                if not filter_val:"},{"line_number":242,"context_line":"                    # Special value to indicate that nothing will match."},{"line_number":243,"context_line":"                    return None"},{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa7ab95a_11aaf6d4","line":244,"range":{"start_line":244,"start_character":16,"end_line":244,"end_character":68},"in_reply_to":"fa7ab95a_0b872510","updated":"2016-08-25 21:28:52.000000000","message":"I think I finally get this now -- the difference between the attribute being set vs existing at all. Thanks for your patience in explaining.","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"a36224fe8ab8b5fdb941e11e589dc8b8c150cc98","unresolved":false,"context_lines":[{"line_number":241,"context_line":"                if not filter_val:"},{"line_number":242,"context_line":"                    # Special value to indicate that nothing will match."},{"line_number":243,"context_line":"                    return None"},{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa7ab95a_770332b9","line":244,"range":{"start_line":244,"start_character":16,"end_line":244,"end_character":68},"in_reply_to":"fa7ab95a_11aaf6d4","updated":"2016-08-25 22:07:35.000000000","message":"Thanks for the thorough review. I appreciate the scrutiny to make sure I\u0027m not sneaking something in.\n\nI had to spend some time to convince myself it was right originally because it\u0027s really an odd check. I don\u0027t think it\u0027s intentionally supported to check against an unset attribute, but then again much of the sorting/filtering code doesn\u0027t seem intentional :)","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"338bc0083ad17e4ed59f9fdf8c4226ec9041ab4d","unresolved":false,"context_lines":[{"line_number":241,"context_line":"                if not filter_val:"},{"line_number":242,"context_line":"                    # Special value to indicate that nothing will match."},{"line_number":243,"context_line":"                    return None"},{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa7ab95a_0b872510","line":244,"range":{"start_line":244,"start_character":16,"end_line":244,"end_character":68},"in_reply_to":"fa7ab95a_45483c2e","updated":"2016-08-25 20:55:09.000000000","message":"Ugh, sorry about that","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ce521098e511a701dd3247ee2f2c621c9e6d5d11","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                if instance.get(filter_key, None) not in filter_val:"},{"line_number":245,"context_line":"                    return False"},{"line_number":246,"context_line":"            else:"},{"line_number":247,"context_line":"                if instance.get(filter_key, None) !\u003d filter_val:"},{"line_number":248,"context_line":"                    return False"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":9,"id":"1ac06dbe_8c82e250","line":247,"range":{"start_line":247,"start_character":16,"end_line":247,"end_character":64},"updated":"2016-08-25 18:58:06.000000000","message":"Same comment","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"}],"nova/tests/functional/db/test_build_request.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"45b89e4a945593d2fc4f1e70be6403d1953b8bdd","unresolved":false,"context_lines":[{"line_number":121,"context_line":"        self.assertEqual(2, len(req_list))"},{"line_number":122,"context_line":"        for i in range(len(req_list)):"},{"line_number":123,"context_line":"            self.assertEqual(reqs[i].instance_uuid, req_list[i].instance_uuid)"},{"line_number":124,"context_line":"            objects.base.obj_equal_prims(reqs[i].instance,"},{"line_number":125,"context_line":"                                         req_list[i].instance)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def test_get_all_filter_by_project_id(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"7faddb67_456ef600","line":124,"range":{"start_line":124,"start_character":25,"end_line":124,"end_character":40},"updated":"2019-08-12 15:37:05.000000000","message":"Here and throughout, this method doesn\u0027t assert, it only returns True or False, the calling test code has to do assertTrue or something.","commit_id":"5ba1ff9180019bfc71d4a6f44f10c2b4a85284be"}]}