)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d6abfd8a5c1a3f555a168f8dbb019b564e569ca6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"0332c2cf_221ffc40","updated":"2025-03-03 04:53:46.000000000","message":"Awesome work both of you, I\u0027m really impressed!! This test is a real tough one, and you guys got one that worked great!\n\nSome clean up of commented code is required, use of self.assrtEqual over assert. And a cleaner/simpler way of doing it by mocking the actual call to the function rather then using an object in hand and sideeffecting it.\n\nGreat work!","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"814ca3c7_f9fa44c3","updated":"2025-03-24 20:20:19.000000000","message":"I\u0027m kinda worried about the maintainability of having `commit()`/`rollback()` calls in the code that don\u0027t actually commit/rollback...","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"dd4d4d5a_4091d510","updated":"2025-03-04 10:06:19.000000000","message":"recheck","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@redhat.com","username":"cschwede"},"change_message_id":"0f360de8b50415deceb0bf279e7d35b0aa6bdce9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"c2bb869d_3d7cfcd5","updated":"2025-04-10 13:24:39.000000000","message":"For reference, this is about https://bugs.launchpad.net/swift/+bug/1384451","commit_id":"9b04aa134bd0da7e7901314e51164543a1640a94"}],"swift/common/db.py":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"16be267bc4b5408f97a6a37968f56714e0f4b9ae","unresolved":true,"context_lines":[{"line_number":549,"context_line":"        self.conn \u003d None"},{"line_number":550,"context_line":"        try:"},{"line_number":551,"context_line":"            yield conn"},{"line_number":552,"context_line":"            conn.rollback()"},{"line_number":553,"context_line":"            self.conn \u003d conn"},{"line_number":554,"context_line":"        except sqlite3.DatabaseError as e:"},{"line_number":555,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ea3532c0_9b08f19f","line":552,"updated":"2025-02-17 07:04:34.000000000","message":"And I do wonder if this rollback will go rollback the transaction to anything we try and do previously 😢","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":549,"context_line":"        self.conn \u003d None"},{"line_number":550,"context_line":"        try:"},{"line_number":551,"context_line":"            yield conn"},{"line_number":552,"context_line":"            conn.rollback()"},{"line_number":553,"context_line":"            self.conn \u003d conn"},{"line_number":554,"context_line":"        except sqlite3.DatabaseError as e:"},{"line_number":555,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5db1afd1_92c540a8","line":552,"in_reply_to":"ea3532c0_9b08f19f","updated":"2025-03-04 10:06:19.000000000","message":"Acknowledged","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"16be267bc4b5408f97a6a37968f56714e0f4b9ae","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        try:"},{"line_number":551,"context_line":"            yield conn"},{"line_number":552,"context_line":"            conn.rollback()"},{"line_number":553,"context_line":"            self.conn \u003d conn"},{"line_number":554,"context_line":"        except sqlite3.DatabaseError as e:"},{"line_number":555,"context_line":"            try:"},{"line_number":556,"context_line":"                conn.close()"}],"source_content_type":"text/x-python","patch_set":2,"id":"b96b231f_b46fb231","line":553,"updated":"2025-02-17 07:04:34.000000000","message":"Ahh the newly created conn object isn\u0027t being written back until after the yield, so after we\u0027ve finished using it. Which is why you have to pass the conn all around the place. I assumed once we created it the new one would be available to everyone else (ie written back earlier).\n\nI wonder if we could get the transaction function to write this back earlier, so other calls will use the same conn that is within the transaction.\n\nI guess it\u0027s trying to act as a singleton/sentinal and if a conn object is in use then something will need to go make another one.","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":550,"context_line":"        try:"},{"line_number":551,"context_line":"            yield conn"},{"line_number":552,"context_line":"            conn.rollback()"},{"line_number":553,"context_line":"            self.conn \u003d conn"},{"line_number":554,"context_line":"        except sqlite3.DatabaseError as e:"},{"line_number":555,"context_line":"            try:"},{"line_number":556,"context_line":"                conn.close()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a4be039_afea547c","line":553,"in_reply_to":"b96b231f_b46fb231","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":false,"context_lines":[{"line_number":590,"context_line":"    @contextmanager"},{"line_number":591,"context_line":"    def transaction(self):"},{"line_number":592,"context_line":"        \"\"\"Use with the \"with\" statement; ensures multiple database updates execute atomically.\"\"\""},{"line_number":593,"context_line":"        with self.get() as conn:"},{"line_number":594,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":595,"context_line":"            try:"},{"line_number":596,"context_line":"                conn.isolation_level \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"4705298c_21b858af","line":593,"updated":"2025-03-24 20:20:19.000000000","message":"Right; this will take care of ensuring we issue the `ROLLBACK`.","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"16be267bc4b5408f97a6a37968f56714e0f4b9ae","unresolved":true,"context_lines":[{"line_number":594,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":595,"context_line":"            try:"},{"line_number":596,"context_line":"                conn.isolation_level \u003d None"},{"line_number":597,"context_line":"                conn.execute(\u0027BEGIN TRANSACTION\u0027)"},{"line_number":598,"context_line":"                yield conn"},{"line_number":599,"context_line":"                conn.execute(\u0027COMMIT\u0027)"},{"line_number":600,"context_line":"            finally:"}],"source_content_type":"text/x-python","patch_set":2,"id":"03470b57_ad378e0f","line":597,"updated":"2025-02-17 07:04:34.000000000","message":"I wonder if we could write this back here, with something like:\n\n    self.conn \u003d conn\n\nDoes your testing work then without having to pass around the conn object. Not that I\u0027m against that if that\u0027s the direction we need to take.\n\nHmm, I wonder if we can take it one step further and mark that we\u0027re in a transaction so these can\u0027t be nested.. or maybe thats ok :hmmm:\n\nUPDATE: Maybe omething like https://paste.openstack.org/show/826853/","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":594,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":595,"context_line":"            try:"},{"line_number":596,"context_line":"                conn.isolation_level \u003d None"},{"line_number":597,"context_line":"                conn.execute(\u0027BEGIN TRANSACTION\u0027)"},{"line_number":598,"context_line":"                yield conn"},{"line_number":599,"context_line":"                conn.execute(\u0027COMMIT\u0027)"},{"line_number":600,"context_line":"            finally:"}],"source_content_type":"text/x-python","patch_set":2,"id":"75f3e02e_2a0ca75a","line":597,"in_reply_to":"03470b57_ad378e0f","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":true,"context_lines":[{"line_number":610,"context_line":"        with self.get() as conn:"},{"line_number":611,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":612,"context_line":"            try:"},{"line_number":613,"context_line":"                conn.isolation_level \u003d None"},{"line_number":614,"context_line":"                conn.execute(\u0027BEGIN\u0027)"},{"line_number":615,"context_line":"                # write back the conn, so we can call other methods in"},{"line_number":616,"context_line":"                # this context."}],"source_content_type":"text/x-python","patch_set":8,"id":"34a85f15_0890d0a2","line":613,"updated":"2025-03-24 20:20:19.000000000","message":"What are we hoping to get out of the `isolation_level` changes? Or are we just cribbing from `lock`?\n\nI\u0027m not actually sure what we\u0027re hoping to get out of them _there_, either -- AFAICT it\u0027s the only place we touch it!","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d8f44292988e7aa123f9f7b77af5f2710a36b4e3","unresolved":true,"context_lines":[{"line_number":610,"context_line":"        with self.get() as conn:"},{"line_number":611,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":612,"context_line":"            try:"},{"line_number":613,"context_line":"                conn.isolation_level \u003d None"},{"line_number":614,"context_line":"                conn.execute(\u0027BEGIN\u0027)"},{"line_number":615,"context_line":"                # write back the conn, so we can call other methods in"},{"line_number":616,"context_line":"                # this context."}],"source_content_type":"text/x-python","patch_set":8,"id":"df259db3_045d85e8","line":613,"in_reply_to":"34a85f15_0890d0a2","updated":"2025-04-10 04:58:24.000000000","message":"lol actually I came up with it comletely independantly. I never even remember seeing lock. TBH I\u0027m not even sure what you\u0027d use lock for. It doesn\u0027t return the connection.\n\nIt makes a connection object, makes it isolation level none (no auto transactions) but then take keeps the connection local and removed the self.local. But this means anything else with the object can just run:\n\n```\nwith broker.get() as conn:\n  \u003cdo things\u003e\n```\n\nBecause the\u0027ll go and create a brand new connection... so what is it even stopping!! Rolling back a tranaction on a connection object it hasn\u0027t given anyone :shrug:\n\nOh the begin immidaite must lock the file for writing so no onle else can use it. Maybe `get_db_connection` fails because of the write lock?","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":true,"context_lines":[{"line_number":611,"context_line":"            orig_isolation_level \u003d conn.isolation_level"},{"line_number":612,"context_line":"            try:"},{"line_number":613,"context_line":"                conn.isolation_level \u003d None"},{"line_number":614,"context_line":"                conn.execute(\u0027BEGIN\u0027)"},{"line_number":615,"context_line":"                # write back the conn, so we can call other methods in"},{"line_number":616,"context_line":"                # this context."},{"line_number":617,"context_line":"                conn.transaction_started \u003d True"}],"source_content_type":"text/x-python","patch_set":8,"id":"d30ec311_6f571ff8","line":614,"range":{"start_line":614,"start_character":30,"end_line":614,"end_character":35},"updated":"2025-03-24 20:20:19.000000000","message":"OK, implicitly `BEGIN DEFERRED`, so\n\n\u003e the transaction does not actually start until the database is first accessed.\n\nThe [docs go on to say](https://sqlite.org/lang_transaction.html#deferred_immediate_and_exclusive_transactions)\n\n\u003e Internally, the BEGIN DEFERRED statement merely sets a flag on the database connection that turns off the automatic commit that would normally occur when the last statement finishes.\n\n... which (combined with the neutering of `conn.commit()`) explains the need to do the commit on L621","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":true,"context_lines":[{"line_number":612,"context_line":"            try:"},{"line_number":613,"context_line":"                conn.isolation_level \u003d None"},{"line_number":614,"context_line":"                conn.execute(\u0027BEGIN\u0027)"},{"line_number":615,"context_line":"                # write back the conn, so we can call other methods in"},{"line_number":616,"context_line":"                # this context."},{"line_number":617,"context_line":"                conn.transaction_started \u003d True"},{"line_number":618,"context_line":"                self.conn \u003d conn"},{"line_number":619,"context_line":"                yield conn"}],"source_content_type":"text/x-python","patch_set":8,"id":"e593fba3_0cddc090","line":616,"range":{"start_line":615,"start_character":39,"end_line":616,"end_character":30},"updated":"2025-03-24 20:20:19.000000000","message":"OK, so that seems like a reasonable convenience at first -- and it means that we don\u0027t have to touch that much code to do this.\n\nBut it also has some weird knock-on effects. The code that\u0027s doing the updates still calls `commit`, even though it won\u0027t end up committing anything. So now a developer will need to remember that the code\u0027s lying to them because we\u0027re in a transaction context two or three layers up.\n\nOr at least, it\u0027s lying when we get there via that particular call stack? I feel like it\u0027s become more difficult to reason about what the code is doing when, and whether some reasonably low-level function like `update_put_timestamp` should be calling `commit` or not.\n\nI wonder if it would be better to introduce a new `update_metadata_and_put_timestamp` function that runs both sets of queries then commits (and probably deprecate `update_metadata` and `update_put_timestamp`). It might even make sense to use `lock` for the transaction management.","commit_id":"5c74a17f69382fff2f42b2d912fd9b8434329d47"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d8f44292988e7aa123f9f7b77af5f2710a36b4e3","unresolved":true,"context_lines":[{"line_number":591,"context_line":"        self.conn \u003d None"},{"line_number":592,"context_line":"        orig_isolation_level \u003d conn.isolation_level"},{"line_number":593,"context_line":"        conn.isolation_level \u003d None"},{"line_number":594,"context_line":"        conn.execute(\u0027BEGIN IMMEDIATE\u0027)"},{"line_number":595,"context_line":"        try:"},{"line_number":596,"context_line":"            yield True"},{"line_number":597,"context_line":"        finally:"}],"source_content_type":"text/x-python","patch_set":9,"id":"2806bc40_dbff7b59","line":594,"range":{"start_line":594,"start_character":28,"end_line":594,"end_character":37},"updated":"2025-04-10 04:58:24.000000000","message":"I wonder if this should be EXCLUSIVE rather then IMMEDIATE to really lock down the db.","commit_id":"9b04aa134bd0da7e7901314e51164543a1640a94"}],"swift/container/server.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4580ed8aa21517a09b9f854213f55291519ef651","unresolved":true,"context_lines":[{"line_number":1008,"context_line":"                    req.headers.get(\u0027x-backend-no-timestamp-update\u0027, False)):"},{"line_number":1009,"context_line":"                broker.update_put_timestamp(req_timestamp.internal, conn)"},{"line_number":1010,"context_line":"            self._update_metadata(req, broker, req_timestamp, \u0027POST\u0027, conn)"},{"line_number":1011,"context_line":"        self.sync_store.update_sync_store(broker)"},{"line_number":1012,"context_line":"        return HTTPNoContent(request\u003dreq)"},{"line_number":1013,"context_line":""},{"line_number":1014,"context_line":"    def __call__(self, env, start_response):"}],"source_content_type":"text/x-python","patch_set":2,"id":"352bc433_f8ecc33b","line":1011,"updated":"2025-03-24 20:20:19.000000000","message":"Aren\u0027t we still calling `_update_sync_store` in `update_metadata`?\n\nI _think_ I get why it could make sense to pull that out of the transaction (it\u0027s read-only on the broker and doesn\u0027t need to be in the write transaction), but even if we do that,\n\n1. we\u0027ll still want to use `_update_sync_store` for the exception handling and logging, and\n2. I\u0027m not sure we need to do it as part of _this_ change; I think we could get everything currently covered by `update_put_timestamp` and `_update_metadata` into a transaction and only worry about getting the sync point updates out later (if at all)","commit_id":"daa299e193cc8c5809b25eb5fa36cd6c8488c544"}],"test/unit/container/test_server.py":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d6abfd8a5c1a3f555a168f8dbb019b564e569ca6","unresolved":true,"context_lines":[{"line_number":1486,"context_line":"        self.assertEqual(info[\u0027x_container_sync_point1\u0027], -1)"},{"line_number":1487,"context_line":"        self.assertEqual(info[\u0027x_container_sync_point2\u0027], -1)"},{"line_number":1488,"context_line":""},{"line_number":1489,"context_line":"    # from swift.container.backend import ContainerBroker"},{"line_number":1490,"context_line":"    # from swift.container.server import ContainerController"},{"line_number":1491,"context_line":"    # swift.container.server.ContainerBroker"},{"line_number":1492,"context_line":"    def test_POST_atomic(self):"},{"line_number":1493,"context_line":"        \"\"\"Ensure atomic POST using transaction\"\"\""},{"line_number":1494,"context_line":"        # Create container"}],"source_content_type":"text/x-python","patch_set":7,"id":"1211c71d_55be485c","line":1491,"range":{"start_line":1489,"start_character":4,"end_line":1491,"end_character":44},"updated":"2025-03-03 04:53:46.000000000","message":"Should remove these 😊","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":1486,"context_line":"        self.assertEqual(info[\u0027x_container_sync_point1\u0027], -1)"},{"line_number":1487,"context_line":"        self.assertEqual(info[\u0027x_container_sync_point2\u0027], -1)"},{"line_number":1488,"context_line":""},{"line_number":1489,"context_line":"    # from swift.container.backend import ContainerBroker"},{"line_number":1490,"context_line":"    # from swift.container.server import ContainerController"},{"line_number":1491,"context_line":"    # swift.container.server.ContainerBroker"},{"line_number":1492,"context_line":"    def test_POST_atomic(self):"},{"line_number":1493,"context_line":"        \"\"\"Ensure atomic POST using transaction\"\"\""},{"line_number":1494,"context_line":"        # Create container"}],"source_content_type":"text/x-python","patch_set":7,"id":"ca0556ca_c34192a8","line":1491,"range":{"start_line":1489,"start_character":4,"end_line":1491,"end_character":44},"in_reply_to":"1211c71d_55be485c","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d6abfd8a5c1a3f555a168f8dbb019b564e569ca6","unresolved":true,"context_lines":[{"line_number":1489,"context_line":"    # from swift.container.backend import ContainerBroker"},{"line_number":1490,"context_line":"    # from swift.container.server import ContainerController"},{"line_number":1491,"context_line":"    # swift.container.server.ContainerBroker"},{"line_number":1492,"context_line":"    def test_POST_atomic(self):"},{"line_number":1493,"context_line":"        \"\"\"Ensure atomic POST using transaction\"\"\""},{"line_number":1494,"context_line":"        # Create container"},{"line_number":1495,"context_line":"        put_ts \u003d Timestamp(1)"}],"source_content_type":"text/x-python","patch_set":7,"id":"a32727ba_d8e34447","line":1492,"updated":"2025-03-03 04:53:46.000000000","message":"If I turn back the db changes this test fails. Which is good, with the changes:\n\n```\n$ pytest test/unit/container/test_server.py::TestContainerController::test_POST_atomic\n...                                                                                                                                                                                                                        \ntest/unit/container/test_server.py::TestContainerController::test_POST_atomic PASSED                                                                                                                                                                                      [100%]\n...\n```\n\nRevert continer server changes, and run again:\n```\n(venv) [matt ~/.../5/swift (review/boosung_kim/metadata-timestamp-partial-updates-fix %)]$ git checkout HEAD~ -- swift/container/server.py\n(venv) [matt ~/.../5/swift (review/boosung_kim/metadata-timestamp-partial-updates-fix +%)]$ pytest test/unit/container/test_server.py::TestContainerController::test_POST_atomic\n...\ntest/unit/container/test_server.py::TestContainerController::test_POST_atomic FAILED\n...\n```","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":1489,"context_line":"    # from swift.container.backend import ContainerBroker"},{"line_number":1490,"context_line":"    # from swift.container.server import ContainerController"},{"line_number":1491,"context_line":"    # swift.container.server.ContainerBroker"},{"line_number":1492,"context_line":"    def test_POST_atomic(self):"},{"line_number":1493,"context_line":"        \"\"\"Ensure atomic POST using transaction\"\"\""},{"line_number":1494,"context_line":"        # Create container"},{"line_number":1495,"context_line":"        put_ts \u003d Timestamp(1)"}],"source_content_type":"text/x-python","patch_set":7,"id":"6def491c_d2247a3f","line":1492,"in_reply_to":"a32727ba_d8e34447","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d6abfd8a5c1a3f555a168f8dbb019b564e569ca6","unresolved":true,"context_lines":[{"line_number":1527,"context_line":"            resp \u003d req.get_response(self.controller)"},{"line_number":1528,"context_line":""},{"line_number":1529,"context_line":"            # If a race condition exists, this assertion might fail"},{"line_number":1530,"context_line":"            assert resp.headers.get(\u0027x-container-meta-color\u0027) \u003d\u003d \u0027blue\u0027, \\"},{"line_number":1531,"context_line":"                \"Metadata changed before transaction completed\""},{"line_number":1532,"context_line":"            assert resp.headers.get(\u0027x-put-timestamp\u0027) \u003d\u003d post_ts.internal, \\"},{"line_number":1533,"context_line":"                \"Put Timestamp changed before transaction completed\""}],"source_content_type":"text/x-python","patch_set":7,"id":"59cd1754_041a8312","line":1530,"range":{"start_line":1530,"start_character":12,"end_line":1530,"end_character":20},"updated":"2025-03-03 04:53:46.000000000","message":"Rather then a standard assert, this is a test and the function is defined inside test function we should use:\n\n    self.assertEqual(resp.headers.get(\u0027x-container-meta-color\u0027), \u0027blue\u0027)\n    aelf.assertEquak(resp.headers.get(\u0027x-put-timestamp\u0027), post_ts.internal)\n\nWhy? Because the variables of the outer function are in scope for the inner function your using to mock here.. Pretty cool huh!","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":1527,"context_line":"            resp \u003d req.get_response(self.controller)"},{"line_number":1528,"context_line":""},{"line_number":1529,"context_line":"            # If a race condition exists, this assertion might fail"},{"line_number":1530,"context_line":"            assert resp.headers.get(\u0027x-container-meta-color\u0027) \u003d\u003d \u0027blue\u0027, \\"},{"line_number":1531,"context_line":"                \"Metadata changed before transaction completed\""},{"line_number":1532,"context_line":"            assert resp.headers.get(\u0027x-put-timestamp\u0027) \u003d\u003d post_ts.internal, \\"},{"line_number":1533,"context_line":"                \"Put Timestamp changed before transaction completed\""}],"source_content_type":"text/x-python","patch_set":7,"id":"799d59ba_3ff803db","line":1530,"range":{"start_line":1530,"start_character":12,"end_line":1530,"end_character":20},"in_reply_to":"59cd1754_041a8312","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"d6abfd8a5c1a3f555a168f8dbb019b564e569ca6","unresolved":true,"context_lines":[{"line_number":1546,"context_line":"                \"Timestamp changed before transaction completed\""},{"line_number":1547,"context_line":"            return orig_update_metadata(*args, **kwargs)"},{"line_number":1548,"context_line":""},{"line_number":1549,"context_line":"        with mock.patch.object(db, \u0027update_put_timestamp\u0027,"},{"line_number":1550,"context_line":"                               side_effect\u003dmock_update_put_timestamp):"},{"line_number":1551,"context_line":"            with mock.patch.object(self.controller, \u0027_update_metadata\u0027,"},{"line_number":1552,"context_line":"                                   side_effect\u003dmock_update_metadata):"}],"source_content_type":"text/x-python","patch_set":7,"id":"a6816565_f6d58dca","line":1549,"range":{"start_line":1549,"start_character":13,"end_line":1549,"end_character":30},"updated":"2025-03-03 04:53:46.000000000","message":"You\u0027re mocking a functon on a definded object (patch.object). We can get away from needing to create an object by just using:\n```\nwith mock.path(\u0027swift.common.db.DatabaseBroker.update_put_timestamp\u0027, mock_update_put_timestamp):\n```\n\nThat way you don\u0027t have to use sideeffect for it to take effect on the db object you have.\nI like how you issue 2 HEADs, but we could probably simplify it by just issuing 1 HEAD right in the middle (after we call one db modification and before the other)... we could simplfy your test slightly to something like:\n\n```\n    def test_POST_atomic(self):\n        \"\"\"Ensure atomic POST using transaction\"\"\"\n        # Create container\n        put_ts \u003d Timestamp(1)\n        req \u003d Request.blank(\u0027/sda1/p/a/c\u0027, environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027PUT\u0027},\n                            headers\u003d{\u0027X-Timestamp\u0027: put_ts.internal})\n        resp \u003d req.get_response(self.controller)\n        self.assertEqual(resp.status_int, 201)\n\n        # Set initial metadata\n        post_ts \u003d Timestamp(2)\n        req \u003d Request.blank(\u0027/sda1/p/a/c\u0027, environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027POST\u0027},\n                            headers\u003d{\u0027X-Timestamp\u0027: post_ts.internal,\n                                     \u0027X-Container-Meta-Color\u0027: \u0027blue\u0027})\n        resp \u003d req.get_response(self.controller)\n        self.assertEqual(resp.status_int, 204)\n\n        # Verify initial pre-POST metadata\n        req \u003d Request.blank(\u0027/sda1/p/a/c\u0027, environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027HEAD\u0027})\n        resp \u003d req.get_response(self.controller)\n        self.assertEqual(resp.headers.get(\u0027x-container-meta-color\u0027), \u0027blue\u0027)\n        self.assertEqual(resp.headers.get(\u0027x-put-timestamp\u0027), post_ts.internal)\n\n        # New POST request with a higher timestamp\n        new_post_ts \u003d Timestamp(3)\n\n        # Store the original methods\n        orig_update_put_timestamp \u003d \\\n            swift.common.db.DatabaseBroker.update_put_timestamp\n\n        def mock_update_put_timestamp(*args, **kwargs):\n            \"\"\"Issue HEAD req during update_put_timestamp to check metadata\"\"\"\n\n            # The transaction should have started. But let\u0027s update PUT\n            # timestamp and then check to see if anything has changed\n            resp \u003d orig_update_put_timestamp(*args, **kwargs)\n\n            # Make a head request in the middle of the transaction to prove\n            # nothing has changed after changing the PUT timestamp (transaction\n            # hasn\u0027t applied any changes yet).\n            req \u003d Request.blank(\u0027/sda1/p/a/c\u0027,\n                                environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027HEAD\u0027})\n            head_resp \u003d req.get_response(self.controller)\n\n            # If a race condition exists, this assertion might fail\n            self.assertEqual(\n                head_resp.headers.get(\u0027x-container-meta-color\u0027), \u0027blue\u0027)\n            self.assertEqual(\n                head_resp.headers.get(\u0027x-put-timestamp\u0027), post_ts.internal)\n\n            # Return orig update response and continue the transaction\n            return resp\n\n        with mock.patch(\u0027swift.common.db.DatabaseBroker.update_put_timestamp\u0027,\n                        mock_update_put_timestamp):\n            # Perform the delayed POST request\n            req \u003d Request.blank(\n                \u0027/sda1/p/a/c\u0027, environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027POST\u0027},\n                headers\u003d{\u0027X-Timestamp\u0027: new_post_ts.internal,\n                            \u0027X-Container-Meta-Color\u0027: \u0027red\u0027})\n            resp \u003d req.get_response(self.controller)\n            self.assertEqual(resp.status_int, 204)\n\n        # Verify final post-POST metadata\n        req \u003d Request.blank(\u0027/sda1/p/a/c\u0027,\n                            environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027HEAD\u0027})\n        resp \u003d req.get_response(self.controller)\n        self.assertEqual(resp.headers.get(\u0027x-container-meta-color\u0027),\n                         \u0027red\u0027)\n        self.assertEqual(resp.headers.get(\u0027x-put-timestamp\u0027),\n                         new_post_ts.internal)\n```\n\nThat way we get monkey patch the call rather then an object in hand, simplifying things. And we know we\u0027re sending the HEAD in after something is written, yet not committed (thanks to the transaction).","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"},{"author":{"_account_id":37720,"name":"Boosung Kim","display_name":"Boosung Kim","email":"boosung@boosungkim.com","username":"boosungkim"},"change_message_id":"a6c5f83afcc5d10f7e26d67b30a2da1a2bafd536","unresolved":false,"context_lines":[{"line_number":1546,"context_line":"                \"Timestamp changed before transaction completed\""},{"line_number":1547,"context_line":"            return orig_update_metadata(*args, **kwargs)"},{"line_number":1548,"context_line":""},{"line_number":1549,"context_line":"        with mock.patch.object(db, \u0027update_put_timestamp\u0027,"},{"line_number":1550,"context_line":"                               side_effect\u003dmock_update_put_timestamp):"},{"line_number":1551,"context_line":"            with mock.patch.object(self.controller, \u0027_update_metadata\u0027,"},{"line_number":1552,"context_line":"                                   side_effect\u003dmock_update_metadata):"}],"source_content_type":"text/x-python","patch_set":7,"id":"59723d8a_fd13907a","line":1549,"range":{"start_line":1549,"start_character":13,"end_line":1549,"end_character":30},"in_reply_to":"a6816565_f6d58dca","updated":"2025-03-04 10:06:19.000000000","message":"Done","commit_id":"3f8096d6d7538d93915a98947db3bd5bc3c4f1d5"}]}