)]}'
{"neutron/tests/unit/objects/test_base.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"da594e434ce640d238d776fecf3398f70628f29e","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f1a1f67_acc80974","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"updated":"2017-07-21 07:30:56.000000000","message":"I don\u0027t understand this action. If you find the unique keys constrain is violated, why do you call again the same function? Won\u0027t you create an endless loop?\n\nI though you were going to throw an exception to avoid clashes between unique keys.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"8d0dd3967a089716729cc8eaf38083b2ee22ec67","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_dd82d3c8","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"1f1a1f67_1eb5dbee","updated":"2017-07-24 10:16:50.000000000","message":"I would like to clarify again how this bug happens, because my explanation in the previous comment is wrong. \n\nWe have in total 3 steps involving create random UUIDs/objects in self.test_update_objects(). I will be using RouterL3AgentBinding as an example here. \n\n(1) In the setUp() phase, we generate 3 RouterL3AgentBinding objects with randomly generated UUIDs by self.get_random_object_fields(). But since it only generates UUIDs, no actual db/OVO object is stored in DB, which is to say we do not have Router or Agent objects at this moment. \n\n(2) Since for some of the fields, we require actual db/OVO objects, so we update the 3 UUIDs-only RouterL3AgentBinding objects with actual Router or Agent objects. In this test case, we have 1 router, 3 agents and 3 binding indexes. \n\nAfter this step, we will have 3 RouterL3AgentBinding records, let\u0027s call them (router, 1, agent_1), (router, 2, agent_2) and (router, 3, agent_3). \n\n(3) Then in \u003chttps://github.com/openstack/neutron/blob/master/neutron/tests/unit/objects/test_base.py#L1845\u003e, we get some randomly generated values to update our RouterL3AgentBinding records. In this case, the only updatable parameter is index (the reason behind is that router_id and agent_id are defined as primary keys in db models). \n\nIn the bug report, the randomly generated index happens to be 2, which violates the already existing (router, 1) unique key constraints. \n\nSome notes here:\nIn step (1), we add unique key combination to the tracker but since no actual db records will be created in this step, I do not think we need to compare the value generated in (2) with existing unique key combination. The only thing we need to do is to add unique key combination generated in (2) to self._unique_tracker. By doing so, when we enter step(3), the random value generated by self.get_random_object_fields() will be compared with self._unique_tracker, then we can avoid unique key violation.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"2c800dba6b5f27d51558b3bca21ebefacd37a16e","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f1a1f67_d2e2bc01","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"1f1a1f67_2c13d968","updated":"2017-07-21 08:30:11.000000000","message":"\"v\" could be a callable function, in other cases is just a value.\n\n\"Also we cannot simply throw an exception because we do need to generate these obj\"\nI know, but with this you are saying to the developer of those test the need of changing these specific value, in case \"v\" is not a function.\n\nAnyway, this solution solves the problem described in the bug [1] (values generated by get_random_object_fields), so I think it\u0027s valid.\n\nThanks for your answer.\n\n[1] \"This is because of self.update_obj_fields used in several test classes not updating unique_tracker in the base test class that makes sure no non-unique values are generated by get_random_object_fields. We probably need to make those two cooperate.\"","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"ed73f749ec6bf8e226cf775d11f4805ca1dd6b52","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f1a1f67_cf37b7e3","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"1f1a1f67_2c13d968","updated":"2017-07-21 07:50:04.000000000","message":"Also we cannot simply throw an exception because we do need to generate these objects.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"133bc1296f4d05e363d0f1b703330c2f628aadca","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f1a1f67_2c13d968","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"1f1a1f67_acc80974","updated":"2017-07-21 07:46:40.000000000","message":"The reason I jump back to the beginning of this function is that by doing so, new values would be generated in Line 603, because \"v\" is a callable function.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"f5b4ac854c4fecbe6c0621dc11367067a0a4fccd","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f1a1f67_1eb5dbee","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"1f1a1f67_d2e2bc01","updated":"2017-07-21 10:24:38.000000000","message":"This bug is a bit tricky. Let me try to clarify it. \n\nIt is actually a corner case of unique key violation generated within self.update_obj_fields(), i.e. the values_dict passed in already exists in database. \n\nAt the beginning of the test, we generate 3 objects through self.get_random_object_fields(). This is fine because all the unique violation will be detected and re-do within the method. \n\nself.update_obj_fields() deals with the case where we need actual OVO object to update specific fields. In the bug report\u0027s case, we need to create router object and l3_agent object in order to update RouterL3AgentBinding. \n\nThe composite unique keys in this case are (router_id, l3_agent_id) and (router_id, binding_index). values_dict in this case is {router_id, l3_agent_id, binding_index}. \n\nThe bug happens here, if we have already created a binding object (router_id_1, l3_agent_id_1, binding_index_blah) through self.get_random_object_fields() and there is a (slight) possibility that in Line 603, we generate (router_id_1, l3_agent_id_1, binding_index_blahblah). \n\nThen boom! Unique key violation occurs. \n\nWhat I am trying to do here is after every new obj value is generated, I check if the combination already exists. If it exists, I call the method itself so that we can generate a new val in Line 603, if v is a callable function. \n\nI have checked all the existing tests, neutron.tests.unit.objects.test_l3agent.RouterL3AgentBindingDbObjTestCase.test_update_objects is the only one passes composite unique keys to values_dict. \n\nThe current infinite loop happens because I overlooked the fact that primary_key is also considered as unique key. I will remove primary_key from self._test_class.unique_keys, then it should be all good.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"8d34edf46f371f0f8d47593e60dfb8b74f01bffe","unresolved":false,"context_lines":[{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    if unique_values in self._unique_tracker[keytup]:"},{"line_number":616,"context_line":"                        return self.update_obj_fields(values_dict)"},{"line_number":617,"context_line":"            if k in self.valid_field_filter:"},{"line_number":618,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":619,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_68bfa3fb","line":616,"range":{"start_line":616,"start_character":24,"end_line":616,"end_character":66},"in_reply_to":"ff346bd7_dd82d3c8","updated":"2017-07-24 10:31:49.000000000","message":"Sorry for the long post, but for references purpose, I need to add one more sentence. \n\nAnother necessary for this bug to occur is in step (3), the randomly generated router_id is the same as the only generated in step (2). At first, I do not get why this could happen because how can we get 2 router with the same ID? But then I realized the router_id generated in step (3) by self.get_random_object_fields() does not have actual db record.","commit_id":"316109094972b9c217928294e906f8db5b54b07e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"694fc66133f1c428e632f45cfe42999da9a8e62d","unresolved":false,"context_lines":[{"line_number":612,"context_line":"                for keys in self._test_class.unique_keys:"},{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    self._unique_tracker[keytup].add(unique_values)"},{"line_number":616,"context_line":"            if k in self.valid_field_filter:"},{"line_number":617,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":618,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ff346bd7_1ff3a50c","line":615,"range":{"start_line":615,"start_character":20,"end_line":615,"end_character":67},"updated":"2017-07-24 15:40:11.000000000","message":"I don\u0027t want to introduce more noise in this bug. I read your description and looks correct.\n\nBut in this line: why don\u0027t you check if \"unique_values\" is already in \"_unique_tracker\"? Like in previous PS. At this point you can overwrite an existing value...\n\nSorry, maybe I\u0027m missing something. I tried to follow the code you explained in the comments.","commit_id":"7ab58eb742416d88a1463f59b382907e5143e62e"},{"author":{"_account_id":17491,"name":"Lujin Luo","email":"luo.lujin@jp.fujitsu.com","username":"Lujin"},"change_message_id":"fed3b4e31432ad6115d73ad84d3706e8befd0060","unresolved":false,"context_lines":[{"line_number":612,"context_line":"                for keys in self._test_class.unique_keys:"},{"line_number":613,"context_line":"                    keytup \u003d tuple(keys)"},{"line_number":614,"context_line":"                    unique_values \u003d tuple(fields[k] for k in keytup)"},{"line_number":615,"context_line":"                    self._unique_tracker[keytup].add(unique_values)"},{"line_number":616,"context_line":"            if k in self.valid_field_filter:"},{"line_number":617,"context_line":"                self.valid_field_filter[k] \u003d val"},{"line_number":618,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ff346bd7_2d56315e","line":615,"range":{"start_line":615,"start_character":20,"end_line":615,"end_character":67},"in_reply_to":"ff346bd7_1ff3a50c","updated":"2017-07-25 01:42:27.000000000","message":"Don\u0027t worry! I would love to discuss with reviewers. It helps me think better. :)\n\nYes, I thought we need to check if \"unique_values\" already exists. But actually this adds extra burden to codes, also it makes almost no sense. Remember in step (1), the \"_unique_tracker\" gets updated by values generated by self.get_random_object_fields(), and these \"objs\" are NOT stored in DB. Which is to say, even though we generate same \"unique_values\" here, it will not actually violate DB layer unique key constraints. \n\n\nThe fix I posted here is one way to fix the bug. I am also working on another bug \u003chttps://review.openstack.org/#/c/485082/\u003e, and it seems it can also fix this one and in an easily understood way. \n\nIn that fix, I add a dict to hold an additional actual object in step (2). Then in step (3), the randomly generated value is replaced by the actual value. Since this router_id actually exists, we will never hit this bug!\n\nIn this case, we can also close this bug, even though that is not the main purpose of the fix. But it would be appreciated if you can review that one too!","commit_id":"7ab58eb742416d88a1463f59b382907e5143e62e"}]}