)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"d94963ffd54d92fa8cb29ce38cf0462029d68f34","unresolved":true,"context_lines":[{"line_number":14,"context_line":"\u0027ABC\u0027, \u0027Abc\u0027 etc same and delete the previous key-value and add"},{"line_number":15,"context_line":"new key-value when case-insensitive keys are being updated."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Closes-Bug: #1538011"},{"line_number":18,"context_line":"Change-Id: Icc418c20e14efa08b356d938839e430223836f80"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"fa1b401d_6d08f61a","line":17,"updated":"2023-03-09 09:46:19.000000000","message":"This bug is assigned to someone else for now.\nthis is assigned one 1535224\n\nyou may close both though in this patch, as they assigned long back.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"c998f83e9f616a63f176a8845fd3413320bb0571","unresolved":false,"context_lines":[{"line_number":14,"context_line":"\u0027ABC\u0027, \u0027Abc\u0027 etc same and delete the previous key-value and add"},{"line_number":15,"context_line":"new key-value when case-insensitive keys are being updated."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Closes-Bug: #1538011"},{"line_number":18,"context_line":"Change-Id: Icc418c20e14efa08b356d938839e430223836f80"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"43a385c6_5a5b6a0f","line":17,"in_reply_to":"d4f2c39e_b53a55a3","updated":"2023-11-24 06:58:38.000000000","message":"Done","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":true,"context_lines":[{"line_number":14,"context_line":"\u0027ABC\u0027, \u0027Abc\u0027 etc same and delete the previous key-value and add"},{"line_number":15,"context_line":"new key-value when case-insensitive keys are being updated."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Closes-Bug: #1538011"},{"line_number":18,"context_line":"Change-Id: Icc418c20e14efa08b356d938839e430223836f80"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"d4f2c39e_b53a55a3","line":17,"in_reply_to":"fa1b401d_6d08f61a","updated":"2023-03-15 10:38:44.000000000","message":"yes, I will add bug in commit msg when respin the change.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"26fed906c79a314291332f7d617288b2c2ad8acc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4a9f7bab_0cd891fb","updated":"2023-02-15 21:29:34.000000000","message":"Change looks reasonable but this needs a functional test. For example, something that does:\n\n1. Create aggregate with metadata key\u003d\u0027Abc\u0027\n2. Verify GET aggregate with metadata key\u003d\u0027Abc\u0027 succeeds\n3. Set aggregate\u0027s metadata key\u003d\u0027abc\u0027 succeeds\n4. Verify GET aggregate with metadata key\u003d\u0027abc\u0027 succeeds\n5. Verify GET aggregate with metadata key\u003d\u0027ABC\u0027 succeeds\n\nto verify behavior is working as expected and protect against regression.","commit_id":"40ae6bd92fdbebbbe64665c780d8079b93a47a70"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ed55175e7dbc213d9e933e2ec86660d6fa5d87e6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"bfe72d2f_5d7f8cd6","updated":"2023-03-13 23:16:00.000000000","message":"Since a problem was spotted with this approach ... maybe there is a simpler way to do this?","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53c329f94851401a1958dd65b4a88118b3f54999","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"1d8bb78e_7049d751","updated":"2023-03-09 15:23:10.000000000","message":"The original discussion that we had on this in 2016 was heavily dependent on melwitt, so I definitely think we should get her sign-off on this. I remember the original conversation and it being a \"hard problem\". I worry that just trying to casefold all the existing keys is not specific enough to handle both the insensitive mysql case and the sensitive cases of postgres and sqlite, a hint provided by your test case being wrong. I know we don\u0027t really technically support postgres anymore, but some people do still run with it, and it would be bad to silently corrupt data in that case. I think to be convinced, I\u0027d expect a much more comprehensive test case, and also probably a unit test that specifically behaves like mysql does, which the functional test can\u0027t, based on sqlite.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"d94963ffd54d92fa8cb29ce38cf0462029d68f34","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5cfc9674_838fe771","updated":"2023-03-09 09:46:19.000000000","message":"a small update in commit msg and a question else lgtm.\n","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"e89a174f53f64c8de7bf6dbf5e8bb3b3c8e36d81","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0ed0a54f_9c6e270b","updated":"2023-02-21 10:43:57.000000000","message":"recheck nova-multi-cell tempest tests timeout","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"98af3f4b067098a4ca01869a4c35432c2f055baa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"2deca559_5fa0c6c8","updated":"2023-02-20 15:53:45.000000000","message":"recheck nova-multi-cell tempest.lib.exceptions.SSHExecCommandFailed","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"752c30dca6870e0c8a596dc1a635504227af745e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"125ea117_67229531","updated":"2023-02-21 06:02:33.000000000","message":"recheck tempest-integrated-compute-ubuntu-focal TIMEOUT","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"26488bc9_556ce352","in_reply_to":"1d8bb78e_7049d751","updated":"2023-03-15 10:38:44.000000000","message":"Done!","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"57d49be60b9859c8dcd737316a0e630bd08736b7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3419fa92_15eb9a82","updated":"2023-03-15 20:13:30.000000000","message":"It seems like we (Dan, Sean, and myself) all agree that raising 409 conflict if the KeyError is caught is a reasonable way to handle this issue.\n\nMy idea to update the key if KeyError was admittedly a bit convoluted and the 409 seems like a very clear way to respond in this hopefully rare situation.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b5d4fa3f_a17d0884","updated":"2023-03-15 10:38:44.000000000","message":"Thanks for your suggestions Dan and melanie. I have tried to address comments and updated the patch.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"587d2d45533ba7f93737cfcd7778952fc4fd478c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b8832e6c_9fe1ff41","updated":"2023-03-15 12:20:09.000000000","message":"This need a release note by the way.\n\nthis is a breaking api change for anyone that relied on case senstivity of keys.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"0f260e7f7da7348c846ddc42a03ef964179c1388","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c1a0ad15_9cd0bf46","updated":"2023-03-15 12:19:35.000000000","message":"mariadb can be case sensitive too it just depend on the coalation type that is used\n\ni remember disucssiong this in a past ptg where i was very much on the side that metadta keys shoudl be case sensitive and we shoudl fix the coalation type to utf8_bin\n\ni think we decided to go another way but i honestly would find it surpising and generally consieder it a bug if \u0027ABC\u0027 and \u0027abc\u0027 keys were consider to be the same.\n\nno matter what we do here it will break someone since both behavior were valid and both were present depending on how you deployed.\n\nas such whiel we might be able to fix this as a bug i dont think we shoudl back port this.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f5ea3cd8a492be78f13acdc4c39b3a318bf0b307","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"18f71d3a_58579f03","in_reply_to":"14d6e8f5_e91c0810","updated":"2023-03-15 21:39:39.000000000","message":"I didn\u0027t think anything will be deleted in the case sensitive database. A KeyError would not be raised, the KeyError is inside a code block that iterates only over matches.\n\nIf it finds a match in the case sensitive database then it will match (with case) in the python dict too right? But abc is not there yet and it won\u0027t find a match. So none would be deleted and then you would have all ABC\u003d1 Abc\u003d2 and abc\u003d3.\n\nIIUC everything works fine even presently if the database is case sensitive. The weird 500 thing only happens if it\u0027s case insensitive and matches keys that are different in casing.\n\nAgain, maybe I\u0027m majorly missing something.\n\nAnd I also agree 409 sounds like the best bet here.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1e7327a145bbf794ef547e6dc322468b483c3a18","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"502c29ad_b6ab2ab5","in_reply_to":"18f71d3a_58579f03","updated":"2023-03-16 12:13:21.000000000","message":"Yes, I agree with melwitt. nothing will be deleted in case sensitive database, as we won\u0027t get KeyError in this case.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1e7327a145bbf794ef547e6dc322468b483c3a18","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c68a9b9c_78b57694","in_reply_to":"3419fa92_15eb9a82","updated":"2023-03-16 12:13:21.000000000","message":"Thanks for suggestion, I will respin the change and raise 409 conflict if we get KeyError.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"57d49be60b9859c8dcd737316a0e630bd08736b7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e00f1b90_310fd4ba","in_reply_to":"b8832e6c_9fe1ff41","updated":"2023-03-15 20:13:30.000000000","message":"Maybe I\u0027m missing something but I don\u0027t see how this would be a breaking change. KeyError only happens if there is a collision in a case insensitive database, so instead of raising a 500 the request to set the metadata would just succeed in that specific scenario. Generally 500 -\u003e non-500 don\u0027t require a microversion because we\u0027re not causing something that used to work to now fail.\n\nThere would be no change in behavior for case sensitive databases.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1e7327a145bbf794ef547e6dc322468b483c3a18","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"8f5ef243_2f5b882a","in_reply_to":"c1a0ad15_9cd0bf46","updated":"2023-03-16 12:13:21.000000000","message":"In earlier fix, I have changed the collation type to utf8_bin to resolve this issue here.\n\nhttps://review.opendev.org/c/openstack/nova/+/504885\n\nBut now, since everyone is agreeing on raising 409, I will update the patch with that.","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"96c6f37098e7d65fe9af4ac61f87d967d5060b7b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"14d6e8f5_e91c0810","in_reply_to":"e00f1b90_310fd4ba","updated":"2023-03-15 21:03:46.000000000","message":"If you are running in a case-sensitive database and already have ABC\u003d1, Abc\u003d2 and try to update abc\u003d3, which of the two existing ones will you delete? Probably the first, maybe \"it depends\" but you definitely won\u0027t get a third, which you expect because of how this has always worked, right?\n\nEither way, I think 409 is massively simpler and better anyway. Much easier to describe what the user should expect to happen in the docs if nothing else :)","commit_id":"9794fa24427dcf2045d5c96eba283b4a748290fc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5f98da7d7ab05ec4c518dd76529bb8ece7508af4","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"579447b9_2089f053","updated":"2024-03-27 03:04:43.000000000","message":"The fix itself looks good but I think a bit more test coverage is needed.","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"89fd52b1db52026dea52dfa4a68cf24d78545cfe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"2fbfd2fb_08e1442a","in_reply_to":"579447b9_2089f053","updated":"2024-04-08 10:39:45.000000000","message":"Acknowledged","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"89fd52b1db52026dea52dfa4a68cf24d78545cfe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"6decdbb0_50705c58","updated":"2024-04-08 10:39:45.000000000","message":"Thanks Melanie for your help.","commit_id":"747615b9ae3ddda84bc515299063768a4c77123f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b34d232c9a0e8609413ddc41d568d1e1762bc3a2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"beb88cef_be0ebdae","updated":"2024-11-15 01:16:53.000000000","message":"I guess I\u0027ll set the official -1 to move this to a 400. Other than that, patch looks good.","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a3c4fe133c5db2a4152227e97487314a5bec28b6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"127b43c1_58947f0f","updated":"2024-11-14 15:54:24.000000000","message":"Question for the other reviewers: based on [1], because we\u0027re fixing an error 500, we don\u0027t need a new microversion. However, we\u0027re fixing it with an error code that _wasn\u0027t previous possible_ for this API - we currently only have 400, 401, 403, 404 [2]. Now we\u0027re adding 409 to that list, and it\u0027s conceivable that clients that are out there can\u0027t handle a 409 for this API. Do we need a new microversion?\n\n[1] https://docs.openstack.org/nova/latest/contributor/microversions.html\n[2] https://docs.openstack.org/api-ref/compute/#create-or-update-aggregate-metadata","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"088a1f4bafe62ef645af60a1462762ce199d8b9e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"5d6cf89a_822e343f","in_reply_to":"beb88cef_be0ebdae","updated":"2024-11-15 13:23:18.000000000","message":"Done.","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"380bd0b70d02fcb520305ab86742c179b8348aa5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"0647dd1a_b0315c14","updated":"2024-12-04 15:32:27.000000000","message":"+1 but i think we are missign test coverage. what do others think?","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e465c47aabca54402972b6ca7c1c275087bc4631","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"46097997_3396885e","updated":"2024-11-15 14:14:05.000000000","message":"Looks good now. Thanks","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"088a1f4bafe62ef645af60a1462762ce199d8b9e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"874b8ef6_8d20fb31","updated":"2024-11-15 13:23:18.000000000","message":"Thanks for review. I have updated the patch as per your suggestions.","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bee63d8a46cbb1787da76d54b00acfaf35437e0f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"867c2981_cf1c5eb3","updated":"2025-03-25 13:31:08.000000000","message":"This has been up for review long enough.\nnone of the comment i have inline are blocking so we can adress those later in a follow up\n\nelevetating to +2w","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"}],"nova/api/openstack/compute/aggregates.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5f98da7d7ab05ec4c518dd76529bb8ece7508af4","unresolved":true,"context_lines":[{"line_number":247,"context_line":"            raise exc.HTTPBadRequest(explanation\u003de.format_message())"},{"line_number":248,"context_line":"        except exception.AggregateMetadataKeyExists as e:"},{"line_number":249,"context_line":"            raise exc.HTTPConflict(explanation\u003de.format_message())"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        return self._marshall_aggregate(req, aggregate)"},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"    def _marshall_aggregate(self, req, aggregate):"}],"source_content_type":"text/x-python","patch_set":4,"id":"363103e7_55fe0d39","line":250,"updated":"2024-03-27 03:04:43.000000000","message":"This part of the change is missing test coverage.","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"89fd52b1db52026dea52dfa4a68cf24d78545cfe","unresolved":false,"context_lines":[{"line_number":247,"context_line":"            raise exc.HTTPBadRequest(explanation\u003de.format_message())"},{"line_number":248,"context_line":"        except exception.AggregateMetadataKeyExists as e:"},{"line_number":249,"context_line":"            raise exc.HTTPConflict(explanation\u003de.format_message())"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        return self._marshall_aggregate(req, aggregate)"},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"    def _marshall_aggregate(self, req, aggregate):"}],"source_content_type":"text/x-python","patch_set":4,"id":"2cd8ef09_015ec7fc","line":250,"in_reply_to":"363103e7_55fe0d39","updated":"2024-04-08 10:39:45.000000000","message":"Acknowledged","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7b88c1d74acab19e47419296e53dfb87a8faf4e8","unresolved":true,"context_lines":[{"line_number":224,"context_line":"            raise exc.HTTPConflict(explanation\u003de.format_message())"},{"line_number":225,"context_line":"        return self._marshall_aggregate(req, aggregate)"},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"    @wsgi.expected_errors((400, 404, 409))"},{"line_number":228,"context_line":"    @wsgi.action(\u0027set_metadata\u0027)"},{"line_number":229,"context_line":"    @validation.schema(aggregates.set_metadata)"},{"line_number":230,"context_line":"    def _set_metadata(self, req, id, body):"}],"source_content_type":"text/x-python","patch_set":6,"id":"3068c60d_aa27cd3d","line":227,"updated":"2024-11-14 17:04:58.000000000","message":"based on https://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion fixing a http 500 does not require a new microvresion. If we want to be super sure then I suggest to return 400 instead of 409 as 400 is already a possible return code.","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"1384ef2e4c51ace720998188f2c1459df532fb6f","unresolved":true,"context_lines":[{"line_number":224,"context_line":"            raise exc.HTTPConflict(explanation\u003de.format_message())"},{"line_number":225,"context_line":"        return self._marshall_aggregate(req, aggregate)"},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"    @wsgi.expected_errors((400, 404, 409))"},{"line_number":228,"context_line":"    @wsgi.action(\u0027set_metadata\u0027)"},{"line_number":229,"context_line":"    @validation.schema(aggregates.set_metadata)"},{"line_number":230,"context_line":"    def _set_metadata(self, req, id, body):"}],"source_content_type":"text/x-python","patch_set":6,"id":"654b5b2a_0307c651","line":227,"in_reply_to":"3068c60d_aa27cd3d","updated":"2024-11-15 00:52:16.000000000","message":"Agree with gibi. 409 is a new error code for this API and if we return that it will be behaviour change so need microversion bump. returning 400 is ok and does not need microversion.","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"088a1f4bafe62ef645af60a1462762ce199d8b9e","unresolved":false,"context_lines":[{"line_number":224,"context_line":"            raise exc.HTTPConflict(explanation\u003de.format_message())"},{"line_number":225,"context_line":"        return self._marshall_aggregate(req, aggregate)"},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"    @wsgi.expected_errors((400, 404, 409))"},{"line_number":228,"context_line":"    @wsgi.action(\u0027set_metadata\u0027)"},{"line_number":229,"context_line":"    @validation.schema(aggregates.set_metadata)"},{"line_number":230,"context_line":"    def _set_metadata(self, req, id, body):"}],"source_content_type":"text/x-python","patch_set":6,"id":"3d5426d7_0b984208","line":227,"in_reply_to":"654b5b2a_0307c651","updated":"2024-11-15 13:23:18.000000000","message":"Done","commit_id":"599b6734f0a7f7f61828471364450cf19ffe3d11"}],"nova/objects/aggregate.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53c329f94851401a1958dd65b4a88118b3f54999","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"c2c8174d_8c18dbf9","line":123,"updated":"2023-03-09 15:23:10.000000000","message":"If we *are* running on a backend that is case sensitive (i.e. postgres and apparently even sqlite), we will end up with duplicates in all_keys_lower() and I wonder if we could accidentally delete the wrong one when we do this? That would be a real problem...","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a63268f52d921d462b69d3df2e732ccd3561d223","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"f13f2a1b_c3070e04","line":123,"in_reply_to":"168704e1_f048c094","updated":"2023-03-15 00:31:23.000000000","message":"Yeah, I think this issue in general is weird and is why most of us have avoided it for so long. IMHO it\u0027s of Low importance overall and involves a disproportionately high amount of thinking to \"solve\". I say \"solve\" because I\u0027m not sure there is a way to handle this that is universally right. But for the sake of discussion ...\n\nIf you do what the repro steps from the LP bug say:\n\n1) \u003e nova aggregate-create agg1\n2) \u003e nova aggregate-set-metadata agg1 abc\u003d1\n3) \u003e nova aggregate-set-metadata agg1 ABC\u003d2\n\nIf the backend is MySQL, on step 3 you get the KeyError and HTTP 500 to the user.\n\nIf the backend is case sensitive (sqlite? postgres?), on step 3 you have no error and you successfully set a new key \"ABC\" with value 2.\n\nSo we already have the API behaving differently depending on the backend.\n\nWhat would be the right behavior here? Or the least wrong behavior?\n\nWhat if making the API behave the same for all backends would turn some existing scenario from a 200 to a fail code? Then we would need a new API microversion.\n\nMy concern with case-folding and just storing everything lowercase in the database is if someone did:\n\n {\n    \"set_metadata\": {\n        \"metadata\": {\n            \"ABC\": \"value\"\n        }\n    }\n }\n \nand then they did a GET and the API sent:\n \n {\n    \"aggregate\": {\n        \"metadata\": {\n            \"abc\": \"value\"\n        }\n    }\n }\n \nand they tried to access what they just set, i.e. resp[\u0027aggregate\u0027][\u0027metadata\u0027][\u0027ABC\u0027], it would be a KeyError. \"How can the key I just set not be there?\" That\u0027s really the only behavior that would bother me.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"63b7b85be70af6f024dedbafa74387cf703e6a48","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"47fbb433_0b0cd015","line":123,"in_reply_to":"29088129_a3b5f649","updated":"2023-03-15 19:54:35.000000000","message":"ya just raising a 409 would work for me honestly\n\nlets not try to lowercase or normalise.\n\nfor now if the db does not suppoort case sensitivty due to the coalation type used  lets just raise a conflict instead of a 500.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"57d49be60b9859c8dcd737316a0e630bd08736b7","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"fe823d01_f6acc504","line":123,"in_reply_to":"47fbb433_0b0cd015","updated":"2023-03-15 20:13:30.000000000","message":"I\u0027m in support of raising 409 conflict for the KeyError as well. None of the other options feel like a clear solution.\n\nI didn\u0027t like the idea of changing the coalation because I\u0027m pretty sure that would break some existing working API request that had been matching in the database due to the case insensitivity. I just didn\u0027t think that would be worth the risk.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8bc9ea3359c3dcd11ebb9b61e7bab9ddad2adce0","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"29088129_a3b5f649","line":123,"in_reply_to":"6b24d502_5875c04d","updated":"2023-03-15 13:42:48.000000000","message":"@melwitt, what I meant was that I think it would be bad to make our code effectively codify the case-somewhat-insensitive behavior on platforms that actually do keep them separate.\n\nAnd I agree with you, this is extremely low-priority latent behavior and I don\u0027t really think it\u0027s worth a high-effort fix.\n\nWe could maybe just catch the KeyError and raise a `409 Conflict` with some reasonable error message and call it good?","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ed55175e7dbc213d9e933e2ec86660d6fa5d87e6","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"efeffb87_2b4384e9","line":123,"in_reply_to":"c2c8174d_8c18dbf9","updated":"2023-03-13 23:16:00.000000000","message":"Hrm, good point. So this logic will not be enough to cover that situation.\n\nWhat I was thinking in general is to always use the key value (and case) that the user provided and that could involve having to update a key if the user is updating metadata in a case insensitive database. And if it\u0027s a case sensitive database, there would be an exact match or no match.\n\nMaybe we could simplify this and instead of using lower() and all that, maybe we could just put a try-except around the update() to catch KeyError. And if we catch KeyError, that means we have a case insensitive match and we do the update thing in that case only. If there was a case match in a case sensitive database, KeyError would not be raised. If there was a case match in a case insensitive database, KeyError would also not be raised.\n\nDo you think there would be any issue with that approach?","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eb2e9f43fa824c5616369cd2791f5abbd48c12a5","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"168704e1_f048c094","line":123,"in_reply_to":"efeffb87_2b4384e9","updated":"2023-03-14 13:31:12.000000000","message":"Well, I think the problem there is that we persist the behavioral difference that the API user sees, depending on the backend in use, right? I guess I\u0027ll have to do some thinking on it in general.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"6b24d502_5875c04d","line":123,"in_reply_to":"f13f2a1b_c3070e04","updated":"2023-03-15 10:38:44.000000000","message":"I have moved the new key-value update logic in except block when KeyError is raised.\nAlso we need to have all_keys_lower as currently, I am not sure how would we get the key from metadata which user trying to update using corresponding key (from db, which is same string with different case) and all_keys_lower list.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"c998f83e9f616a63f176a8845fd3413320bb0571","unresolved":false,"context_lines":[{"line_number":120,"context_line":"                        # If \u0027abc\u0027 key is present and user tries to update"},{"line_number":121,"context_line":"                        # metadata using \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027), then delete"},{"line_number":122,"context_line":"                        # \u0027abc\u0027 key-value and add \u0027ABC\u0027 or (\u0027AbC\u0027, \u0027Abc\u0027)"},{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"}],"source_content_type":"text/x-python","patch_set":2,"id":"0dc3b3e1_5cc749b0","line":123,"in_reply_to":"fe823d01_f6acc504","updated":"2023-11-24 06:58:38.000000000","message":"Done","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53c329f94851401a1958dd65b4a88118b3f54999","unresolved":true,"context_lines":[{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"},{"line_number":127,"context_line":"                        else:"},{"line_number":128,"context_line":"                            meta_ref.update({\"value\": metadata[key]})"},{"line_number":129,"context_line":"                            already_existing_keys.add(key)"}],"source_content_type":"text/x-python","patch_set":2,"id":"b305a580_bf062214","line":126,"updated":"2023-03-09 15:23:10.000000000","message":"Instead of deleting it inline and then adding below, can you not just update `key` *and* `value` here? Or just use your two lists to find the non-lower() key that the user provided to index into `metadata`?\n```\nvalue \u003d metadata[all_keys[all_keys_lower.index(key.lower())]]\n```","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"c998f83e9f616a63f176a8845fd3413320bb0571","unresolved":false,"context_lines":[{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"},{"line_number":127,"context_line":"                        else:"},{"line_number":128,"context_line":"                            meta_ref.update({\"value\": metadata[key]})"},{"line_number":129,"context_line":"                            already_existing_keys.add(key)"}],"source_content_type":"text/x-python","patch_set":2,"id":"61d3c05b_039eb386","line":126,"in_reply_to":"ac92078f_4af22fda","updated":"2023-11-24 06:58:38.000000000","message":"Acknowledged","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":true,"context_lines":[{"line_number":123,"context_line":"                        # key-value in new-entries section below."},{"line_number":124,"context_line":"                        if key.lower() in all_keys_lower and \\"},{"line_number":125,"context_line":"                                key not in all_keys:"},{"line_number":126,"context_line":"                            query.filter_by(key\u003dkey).delete()"},{"line_number":127,"context_line":"                        else:"},{"line_number":128,"context_line":"                            meta_ref.update({\"value\": metadata[key]})"},{"line_number":129,"context_line":"                            already_existing_keys.add(key)"}],"source_content_type":"text/x-python","patch_set":2,"id":"ac92078f_4af22fda","line":126,"in_reply_to":"b305a580_bf062214","updated":"2023-03-15 10:38:44.000000000","message":"Thanks for the hint. I just needed to convert all_keys to list as it is of DictKeys type, which throws TypeError(TypeError: \u0027DictKeys\u0027 object is not subscriptable Error).","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"380bd0b70d02fcb520305ab86742c179b8348aa5","unresolved":true,"context_lines":[{"line_number":114,"context_line":"                    for meta_ref in query.all():"},{"line_number":115,"context_line":"                        key \u003d meta_ref.key"},{"line_number":116,"context_line":"                        try:"},{"line_number":117,"context_line":"                            meta_ref.update({\"value\": metadata[key]})"},{"line_number":118,"context_line":"                            already_existing_keys.add(key)"},{"line_number":119,"context_line":"                        except KeyError:"},{"line_number":120,"context_line":"                            # NOTE(ratailor): When user tries updating"},{"line_number":121,"context_line":"                            # metadata using case-sensitive key, we get"}],"source_content_type":"text/x-python","patch_set":7,"id":"b9077c41_891baeb0","line":118,"range":{"start_line":117,"start_character":28,"end_line":118,"end_character":58},"updated":"2024-12-04 15:32:27.000000000","message":"this path is not tested when we have 2 keys that idffer only by the case without raising a key error.","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"}],"nova/tests/functional/db/test_aggregate.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53c329f94851401a1958dd65b4a88118b3f54999","unresolved":true,"context_lines":[{"line_number":244,"context_line":"                          aggregate_obj._aggregate_get_from_db,"},{"line_number":245,"context_line":"                          self.context, result[\u0027id\u0027])"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_aggregate_update_metadata_with_case_insensitive_key(self):"},{"line_number":248,"context_line":"        fake_create_aggregate \u003d {"},{"line_number":249,"context_line":"            \u0027name\u0027: \u0027aggregate1\u0027,"},{"line_number":250,"context_line":"        }"}],"source_content_type":"text/x-python","patch_set":2,"id":"ce4c3f80_cd13279d","line":247,"updated":"2023-03-09 15:23:10.000000000","message":"This test passes without your change to aggregate.py, so I don\u0027t think that it\u0027s actually confirming your change. I\u0027m wondering if perhaps it\u0027s because sqlite and mysql don\u0027t have the same behavior, which prevents you from hitting the the path you\u0027re changing.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"c998f83e9f616a63f176a8845fd3413320bb0571","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                          aggregate_obj._aggregate_get_from_db,"},{"line_number":245,"context_line":"                          self.context, result[\u0027id\u0027])"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_aggregate_update_metadata_with_case_insensitive_key(self):"},{"line_number":248,"context_line":"        fake_create_aggregate \u003d {"},{"line_number":249,"context_line":"            \u0027name\u0027: \u0027aggregate1\u0027,"},{"line_number":250,"context_line":"        }"}],"source_content_type":"text/x-python","patch_set":2,"id":"1c4fd5a2_fa60f4e5","line":247,"in_reply_to":"749a767f_be156f36","updated":"2023-11-24 06:58:38.000000000","message":"Acknowledged","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ed55175e7dbc213d9e933e2ec86660d6fa5d87e6","unresolved":true,"context_lines":[{"line_number":244,"context_line":"                          aggregate_obj._aggregate_get_from_db,"},{"line_number":245,"context_line":"                          self.context, result[\u0027id\u0027])"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_aggregate_update_metadata_with_case_insensitive_key(self):"},{"line_number":248,"context_line":"        fake_create_aggregate \u003d {"},{"line_number":249,"context_line":"            \u0027name\u0027: \u0027aggregate1\u0027,"},{"line_number":250,"context_line":"        }"}],"source_content_type":"text/x-python","patch_set":2,"id":"de728be0_3106f859","line":247,"in_reply_to":"ce4c3f80_cd13279d","updated":"2023-03-13 23:16:00.000000000","message":"Yeah you\u0027re probably right. I think we would need to do something similar to these:\n\nhttps://github.com/openstack/nova/blob/3886f078dea50baa062c732a0bd9f653e35e09cc/nova/tests/unit/db/api/test_migrations.py#L114\n\nand use the OpportunisticDBTestMixin and MySQLOpportunisticFixture and PostgresqlOpportunisticFixture from oslo.db. (So this test would go into its own test class and the MySQL and postgres versions would inherit from it).\n\nThat would make it be able to run with MySQL/postgres or be skipped if there is no MySQL/postgres database available in the test environment. In the gate there is a MySQL and postgres database set up for testing, for unit tests anyway. I would expect it\u0027s the same for functional tests.\n\n(later) They are set up in the test-setup role [1][2] which runs during functional tests as seen in job-output.txt [3].\n\n[1] https://opendev.org/zuul/zuul-jobs/src/commit/6054680/roles/test-setup/tasks/main.yaml\n[2] https://opendev.org/openstack/nova/src/commit/3886f07/tools/test-setup.sh\n[3] https://zuul.opendev.org/t/openstack/build/addc49a2633d458fb9f21820a25c66cc/log/job-output.txt#500","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a63268f52d921d462b69d3df2e732ccd3561d223","unresolved":true,"context_lines":[{"line_number":244,"context_line":"                          aggregate_obj._aggregate_get_from_db,"},{"line_number":245,"context_line":"                          self.context, result[\u0027id\u0027])"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_aggregate_update_metadata_with_case_insensitive_key(self):"},{"line_number":248,"context_line":"        fake_create_aggregate \u003d {"},{"line_number":249,"context_line":"            \u0027name\u0027: \u0027aggregate1\u0027,"},{"line_number":250,"context_line":"        }"}],"source_content_type":"text/x-python","patch_set":2,"id":"749a767f_be156f36","line":247,"in_reply_to":"d3de2ea0_3f337235","updated":"2023-03-15 00:31:23.000000000","message":"That\u0027s fair enough and it\u0027s true that a standard local devstack would not have set it up.\n\nI don\u0027t think I have seen precedent of doing unit tests like that and it didn\u0027t cross my mind.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eb2e9f43fa824c5616369cd2791f5abbd48c12a5","unresolved":true,"context_lines":[{"line_number":244,"context_line":"                          aggregate_obj._aggregate_get_from_db,"},{"line_number":245,"context_line":"                          self.context, result[\u0027id\u0027])"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_aggregate_update_metadata_with_case_insensitive_key(self):"},{"line_number":248,"context_line":"        fake_create_aggregate \u003d {"},{"line_number":249,"context_line":"            \u0027name\u0027: \u0027aggregate1\u0027,"},{"line_number":250,"context_line":"        }"}],"source_content_type":"text/x-python","patch_set":2,"id":"d3de2ea0_3f337235","line":247,"in_reply_to":"de728be0_3106f859","updated":"2023-03-14 13:31:12.000000000","message":"We can do that *also* but I think it\u0027s probably better to make sure we have a unit test that behaves like mysql (and probably one with sqlite) so that we\u0027re insulated from the availability of those tests. IIRC, they\u0027re somewhat fragile in setup, and they\u0027re designed to fail silently. Also most developers don\u0027t have that setup, which means they\u0027re not running those locally.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"d94963ffd54d92fa8cb29ce38cf0462029d68f34","unresolved":true,"context_lines":[{"line_number":256,"context_line":"        self.assertEqual(agg1[\u0027metadetails\u0027][\u0027Abc\u0027],"},{"line_number":257,"context_line":"                         result[\u0027metadetails\u0027][\u0027Abc\u0027])"},{"line_number":258,"context_line":"        values \u003d deepcopy(_get_fake_aggregate(1, result\u003dFalse))"},{"line_number":259,"context_line":"        values[\u0027metadata\u0027] \u003d deepcopy(_get_fake_metadata(1))"},{"line_number":260,"context_line":"        values[\u0027metadata\u0027][\u0027abc\u0027] \u003d \u0027barbar\u0027"},{"line_number":261,"context_line":"        expected_metadata \u003d deepcopy(values[\u0027metadata\u0027])"},{"line_number":262,"context_line":"        aggregate_obj._aggregate_update_to_db(self.context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"54c99793_ff787107","line":259,"updated":"2023-03-09 09:46:19.000000000","message":"is it because host should not be set for this test?\nwhy not while retrieving aggregate, set and get metadata as metadetails by passing result\u003dTrue ?","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":true,"context_lines":[{"line_number":256,"context_line":"        self.assertEqual(agg1[\u0027metadetails\u0027][\u0027Abc\u0027],"},{"line_number":257,"context_line":"                         result[\u0027metadetails\u0027][\u0027Abc\u0027])"},{"line_number":258,"context_line":"        values \u003d deepcopy(_get_fake_aggregate(1, result\u003dFalse))"},{"line_number":259,"context_line":"        values[\u0027metadata\u0027] \u003d deepcopy(_get_fake_metadata(1))"},{"line_number":260,"context_line":"        values[\u0027metadata\u0027][\u0027abc\u0027] \u003d \u0027barbar\u0027"},{"line_number":261,"context_line":"        expected_metadata \u003d deepcopy(values[\u0027metadata\u0027])"},{"line_number":262,"context_line":"        aggregate_obj._aggregate_update_to_db(self.context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"e62ca353_d3ef1bad","line":259,"in_reply_to":"54c99793_ff787107","updated":"2023-03-15 10:38:44.000000000","message":"I don\u0027t need host that\u0027s why I did like this.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"c998f83e9f616a63f176a8845fd3413320bb0571","unresolved":false,"context_lines":[{"line_number":256,"context_line":"        self.assertEqual(agg1[\u0027metadetails\u0027][\u0027Abc\u0027],"},{"line_number":257,"context_line":"                         result[\u0027metadetails\u0027][\u0027Abc\u0027])"},{"line_number":258,"context_line":"        values \u003d deepcopy(_get_fake_aggregate(1, result\u003dFalse))"},{"line_number":259,"context_line":"        values[\u0027metadata\u0027] \u003d deepcopy(_get_fake_metadata(1))"},{"line_number":260,"context_line":"        values[\u0027metadata\u0027][\u0027abc\u0027] \u003d \u0027barbar\u0027"},{"line_number":261,"context_line":"        expected_metadata \u003d deepcopy(values[\u0027metadata\u0027])"},{"line_number":262,"context_line":"        aggregate_obj._aggregate_update_to_db(self.context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"98baad28_bcd1908c","line":259,"in_reply_to":"e62ca353_d3ef1bad","updated":"2023-11-24 06:58:38.000000000","message":"Acknowledged","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53c329f94851401a1958dd65b4a88118b3f54999","unresolved":true,"context_lines":[{"line_number":264,"context_line":"        updated \u003d aggregate_obj._get_by_metadata_from_db(self.context,"},{"line_number":265,"context_line":"                                                         key\u003d\u0027abc\u0027)"},{"line_number":266,"context_line":"        self.assertEqual(expected_metadata[\u0027abc\u0027],"},{"line_number":267,"context_line":"                         updated[0][\u0027metadetails\u0027][\u0027abc\u0027])"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"    def test_aggregate_update(self):"},{"line_number":270,"context_line":"        created \u003d _create_aggregate(self.context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4d934639_9f8c8c9d","line":267,"updated":"2023-03-09 15:23:10.000000000","message":"You need to assert that `Abc` is not in the metadata dict as well.","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"1da724dbd45ba13d63c63dce06102b4931cf4f1b","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        updated \u003d aggregate_obj._get_by_metadata_from_db(self.context,"},{"line_number":265,"context_line":"                                                         key\u003d\u0027abc\u0027)"},{"line_number":266,"context_line":"        self.assertEqual(expected_metadata[\u0027abc\u0027],"},{"line_number":267,"context_line":"                         updated[0][\u0027metadetails\u0027][\u0027abc\u0027])"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"    def test_aggregate_update(self):"},{"line_number":270,"context_line":"        created \u003d _create_aggregate(self.context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"37523671_9a135f3d","line":267,"in_reply_to":"4d934639_9f8c8c9d","updated":"2023-03-15 10:38:44.000000000","message":"Done","commit_id":"24d6fbf4380536aa931bd5fc3ba7d9a385136324"}],"nova/tests/unit/api/openstack/compute/test_aggregates.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5f98da7d7ab05ec4c518dd76529bb8ece7508af4","unresolved":true,"context_lines":[{"line_number":668,"context_line":"        body \u003d {\"set_metadata\": {\"metadata\": \"test\"}}"},{"line_number":669,"context_line":"        self.assertRaises(self.bad_request, eval(self.set_metadata),"},{"line_number":670,"context_line":"                          self.req, \"1\", body\u003dbody)"},{"line_number":671,"context_line":""},{"line_number":672,"context_line":"    def test_delete_aggregate(self):"},{"line_number":673,"context_line":"        with mock.patch.object(self.controller.api,"},{"line_number":674,"context_line":"                               \u0027delete_aggregate\u0027) as mock_del:"}],"source_content_type":"text/x-python","patch_set":4,"id":"8e95cf5f_aaaf9f2f","line":671,"updated":"2024-03-27 03:04:43.000000000","message":"This is just an example of testing the API part:\n\n```\n    def test_set_metadata_case_sensitive(self):\n        body \u003d {\"set_metadata\": {\"metadata\": {\"foo\": \"bar\"}}}\n        side_effect \u003d exception.AggregateMetadataKeyExists(\n            aggregate_id\u003d\"2\", key\u003d\"foo\")\n\n        with mock.patch.object(self.controller.api,\n                               \u0027update_aggregate_metadata\u0027,\n                               side_effect\u003dside_effect) as mock_update:\n            self.assertRaises(exc.HTTPConflict, eval(self.set_metadata),\n                self.req, \"2\", body\u003dbody)\n            mock_update.assert_called_once_with(self.context, \"2\",\n                body[\"set_metadata\"][\u0027metadata\u0027])\n```","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"89fd52b1db52026dea52dfa4a68cf24d78545cfe","unresolved":false,"context_lines":[{"line_number":668,"context_line":"        body \u003d {\"set_metadata\": {\"metadata\": \"test\"}}"},{"line_number":669,"context_line":"        self.assertRaises(self.bad_request, eval(self.set_metadata),"},{"line_number":670,"context_line":"                          self.req, \"1\", body\u003dbody)"},{"line_number":671,"context_line":""},{"line_number":672,"context_line":"    def test_delete_aggregate(self):"},{"line_number":673,"context_line":"        with mock.patch.object(self.controller.api,"},{"line_number":674,"context_line":"                               \u0027delete_aggregate\u0027) as mock_del:"}],"source_content_type":"text/x-python","patch_set":4,"id":"0bd457e6_fa033719","line":671,"in_reply_to":"8e95cf5f_aaaf9f2f","updated":"2024-04-08 10:39:45.000000000","message":"Done","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"380bd0b70d02fcb520305ab86742c179b8348aa5","unresolved":true,"context_lines":[{"line_number":669,"context_line":"        self.assertRaises(self.bad_request, eval(self.set_metadata),"},{"line_number":670,"context_line":"                          self.req, \"1\", body\u003dbody)"},{"line_number":671,"context_line":""},{"line_number":672,"context_line":"    def test_set_metadata_case_sensitive(self):"},{"line_number":673,"context_line":"        body \u003d {\"set_metadata\": {\"metadata\": {\"foo\": \"bar\"}}}"},{"line_number":674,"context_line":"        side_effect \u003d exception.AggregateMetadataKeyExists("},{"line_number":675,"context_line":"            aggregate_id\u003d\"2\", key\u003d\"foo\")"}],"source_content_type":"text/x-python","patch_set":7,"id":"0d71444d_933925c8","line":672,"range":{"start_line":672,"start_character":8,"end_line":672,"end_character":40},"updated":"2024-12-04 15:32:27.000000000","message":"nit: test_set_metadata_case_sensitive_fails\n\nwe are missing hte happy path where your usign a db configured with a case sesitive coallation type that allows foo and FOO to both be inserted.\n\nbut this does thest the excption translation to a 400 bad request","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"}],"nova/tests/unit/objects/test_aggregate.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5f98da7d7ab05ec4c518dd76529bb8ece7508af4","unresolved":true,"context_lines":[{"line_number":197,"context_line":"        self.assertRaises(exception.AggregateMetadataKeyExists,"},{"line_number":198,"context_line":"                          agg.update_metadata,"},{"line_number":199,"context_line":"                          {\u0027abC\u0027: \u0027barbar\u0027})"},{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @mock.patch(\u0027nova.objects.aggregate._host_add_to_db\u0027)"},{"line_number":202,"context_line":"    def test_add_host_api(self, mock_host_add_api):"},{"line_number":203,"context_line":"        mock_host_add_api.return_value \u003d {\u0027host\u0027: \u0027bar\u0027}"}],"source_content_type":"text/x-python","patch_set":4,"id":"9c6bd228_9990932b","line":200,"updated":"2024-03-27 03:04:43.000000000","message":"I see why you wrote the test this way but it\u0027s not really testing your change because it\u0027s mocking `nova.objects.aggregate._metadata_add_to_db` where your change is.\n\nIt\u0027s a bit tricky but in some local testing I was able to cover your change with the following example. There might be a more elegant way to do it than this.\n\n```\n    @mock.patch(\u0027nova.compute.utils.notify_about_aggregate_action\u0027)\n    def test_update_metadata_api_case_sensitive(self, mock_notify):\n        # Mock context.session.query().filter_by().filter().all()\n        fake_context \u003d mock.Mock()\n        mock_query \u003d mock.Mock()\n        fake_context.session.query.return_value \u003d mock_query\n        mock_query \u003d mock_query.filter_by.return_value.filter.return_value\n        # Return a fake meta_ref that raises a KeyError\n        fake_meta_ref \u003d mock.Mock()\n        fake_meta_ref.update.side_effect \u003d KeyError\n        mock_query.all.return_value \u003d [fake_meta_ref]\n\n        # Create an aggregate in the database\n        agg \u003d aggregate.Aggregate(\n            context\u003dself.context, name\u003d\u0027agg\u0027, metadata\u003d{\u0027Abc\u0027: \u0027bar\u0027})\n        agg.create()\n        # Then replace its context with the mock context\n        agg._context \u003d fake_context\n\n        self.assertRaises(exception.AggregateMetadataKeyExists,\n                          agg.update_metadata,\n                          {\u0027abC\u0027: \u0027barbar\u0027})\n```","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"89fd52b1db52026dea52dfa4a68cf24d78545cfe","unresolved":false,"context_lines":[{"line_number":197,"context_line":"        self.assertRaises(exception.AggregateMetadataKeyExists,"},{"line_number":198,"context_line":"                          agg.update_metadata,"},{"line_number":199,"context_line":"                          {\u0027abC\u0027: \u0027barbar\u0027})"},{"line_number":200,"context_line":""},{"line_number":201,"context_line":"    @mock.patch(\u0027nova.objects.aggregate._host_add_to_db\u0027)"},{"line_number":202,"context_line":"    def test_add_host_api(self, mock_host_add_api):"},{"line_number":203,"context_line":"        mock_host_add_api.return_value \u003d {\u0027host\u0027: \u0027bar\u0027}"}],"source_content_type":"text/x-python","patch_set":4,"id":"7d2918e5_405b713b","line":200,"in_reply_to":"9c6bd228_9990932b","updated":"2024-04-08 10:39:45.000000000","message":"Done","commit_id":"980ddc8082fd02218a34d87c2f214a086dab3cdd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"380bd0b70d02fcb520305ab86742c179b8348aa5","unresolved":true,"context_lines":[{"line_number":176,"context_line":"                                                      {\u0027toadd\u0027: \u0027myval\u0027})"},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"    @mock.patch(\u0027nova.compute.utils.notify_about_aggregate_action\u0027)"},{"line_number":179,"context_line":"    def test_update_metadata_api_case_sensitive(self, mock_notify):"},{"line_number":180,"context_line":"        # Mock context.session.query().filter_by().filter().all()"},{"line_number":181,"context_line":"        fake_context \u003d mock.Mock()"},{"line_number":182,"context_line":"        mock_query \u003d mock.Mock()"}],"source_content_type":"text/x-python","patch_set":7,"id":"ebd4762f_2481572d","line":179,"range":{"start_line":179,"start_character":8,"end_line":179,"end_character":47},"updated":"2024-12-04 15:32:27.000000000","message":"again your only testing the error case.\n\nthis API is not case insensitive by definition\n\nthat depend on how your db is configured.\n\ni do still object to actually making it case insensitive\nso it woudl be better to test both the fail and success case.","commit_id":"ca85ee4ad6f84b77d95b7ba18c459165078bf2d9"}]}