)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"5baae878_99fa8610","updated":"2024-02-27 00:11:13.000000000","message":"\u003e Sep 11, 2020 2:56 PM\n\u003e\n\u003e I think we should stack up the refactor on the surgical fix\n\nSee, this is why I hate putting off refactors.... we\u0027ve lost 3.5yrs (+/-) of context for why I did a thing...","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"51bc306c_ee65d3ec","updated":"2024-02-26 23:26:13.000000000","message":"I really like what this change is trying to fix; I would prefer some different tactics:\n\nhttps://review.opendev.org/c/openstack/swift/+/910285\n\nI have a few nits:\n\nI don\u0027t understand what\u0027s going on with the user_id property - it looks like a drive by.\n\nI\u0027m annoyed by the \"stuffing\" of the sw_req environ for paramater passing to the error handlers\n\nI\u0027d nit that sw_resp.request \u003d\u003d sw_req - there\u0027s no need to pass both\n\nMy only real complaint is that writing the error handlers over in s3response splits the error handling between two modules - and since the success handling is already in s3request I suggest we just keep it all there.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d571910b8722a806768dbf2fd3b349ebf6a4edb9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"0615a340_54afef8b","updated":"2024-02-27 14:43:46.000000000","message":"Tim suggested we expand scope and move all the response handling into s3resonse because that\u0027s normally where he\u0027s goes looking for S3Response.from_swift_resp()\n\nIt\u0027s maybe akward that nearly the entirety of the translation of the sw_resp into a S3Respones (or error) is already in s3request - but I\u0027d prefer to either leave it where it is [1] or move it all.  If we have a clear preference between those two options I\u0027d be happy to help out, I think this change as is sits the fence in an akward posistion.\n\nTim\u0027s right that I\u0027m not sold on using the s3req.environ for user_id, nor the sw_rep.environ to pass the container/obj params (i.e. if that\u0027s a good idea why not stick app in there while we\u0027re at it?).  Keys in the request environ have a tendency to become an external-to-the-mw interface we rely on - we should add new interfaces for intra-mw communication very thoughtfully.  Ideally we could consider that orthogonally to the error handling cleanup/refactor.\n\n1. 910285: sq: keep s3 req error handling in one place | https://review.opendev.org/c/openstack/swift/+/910285","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"f258cc7f_2a3f4c13","in_reply_to":"51bc306c_ee65d3ec","updated":"2024-02-27 00:11:13.000000000","message":"\u003e I\u0027m annoyed by the \"stuffing\" of the sw_req environ for paramater passing to the error handlers\n\nI mean, isn\u0027t that the *entire point* of a request environment? To be a place where we stuff all the random context for a request that other pieces of code need? Pretty sure that was also why I felt like pulling `user_id` off of the request object and into the env was a good call.\n\n\u003e I\u0027d nit that sw_resp.request \u003d\u003d sw_req - there\u0027s no need to pass both\n\nOK, *probably*... at the same time, I know it\u0027s dangerous to assume that all `Response`s will have a request/environ attached. It\u0027s almost certainly a sign of some subtle lurking bug, but see all the places where we `raise AccessDenied` with no `request` kwarg. I guess it comes down to the separation of responsibilities -- a request *necessarily* has the environment bag o\u0027 context: it can\u0027t exist without one -- while a response can be (and reasonably often is) just some disembodied _thing_ devoid of context, even when that\u0027s a clear fiction.\n\n\u003e My only real complaint is that writing the error handlers over in s3response splits the error handling between two modules\n\nYeah, that was something that leapt out at me when I looked at this again this morning (for the first time in a while) -- but I\u0027d recommend going the other way. Get as much as possible of the `swift response -\u003e s3 response` translation into s3response.py, which is where I usually find myself *wanting* to look.\n\n\u003e the success handling is already in s3request\n\nI mean, I can always grow the scope, I suppose...","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d571910b8722a806768dbf2fd3b349ebf6a4edb9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"d68dc276_c5347241","in_reply_to":"f258cc7f_2a3f4c13","updated":"2024-02-27 14:43:46.000000000","message":"\u003e but I\u0027d recommend going the other way. Get as much as possible of the swift response -\u003e s3 response translation into s3response.py\n\nif we can get it all in S3Response.from_swift_response() that would probably be pretty great!\n\n\u003e I mean, I can always grow the scope, I suppose...\n\nI\u0027d rather stick with the existing pattern and clean it up (i.e. leave it in s3requst) or do the whole thing - half measures will tend to loose momentum even with the best of intentions and leave us in a schizophrenic state.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":34892,"name":"ASHWIN A NAIR","display_name":"indianwhocodes","email":"nairashwin952013@gmail.com","username":"indianwhocodes","status":"Nvidia"},"change_message_id":"b00d4c7c4f0ecca2feac954e63bd7e1e3f22ba3c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":16,"id":"101d48f3_cf877cd6","updated":"2024-11-08 18:02:04.000000000","message":"Not ready yet","commit_id":"6e7873d46b68471ee30f054e0be3662ccd52251c"}],"swift/common/middleware/s3api/s3request.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"da534326329d89dc7765ff706f70e8d09cfda0e7","unresolved":false,"context_lines":[{"line_number":1255,"context_line":"                    owner \u003d acl.get(\u0027Owner\u0027)"},{"line_number":1256,"context_line":"                except (ValueError, TypeError, KeyError):"},{"line_number":1257,"context_line":"                    owner \u003d None"},{"line_number":1258,"context_line":"                if owner is None or owner \u003d\u003d self.user_id:"},{"line_number":1259,"context_line":"                    raise BucketAlreadyOwnedByYou(container)"},{"line_number":1260,"context_line":"                raise BucketAlreadyExists(container)"},{"line_number":1261,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_5163461a","line":1258,"range":{"start_line":1258,"start_character":45,"end_line":1258,"end_character":57},"updated":"2020-09-11 22:40:48.000000000","message":"These could almost be top-level functions, or even pulled out to s3response (which seems like a more-fitting module)... looks like this is the only remaining reference to `self` in them.","commit_id":"ae98c833bbcb7bf4d3c8034dd7f45d4de5889512"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"da534326329d89dc7765ff706f70e8d09cfda0e7","unresolved":false,"context_lines":[{"line_number":1325,"context_line":"                raise NoSuchKey(obj)"},{"line_number":1326,"context_line":""},{"line_number":1327,"context_line":"        else:"},{"line_number":1328,"context_line":"            raise KeyError"},{"line_number":1329,"context_line":""},{"line_number":1330,"context_line":"    def _get_response(self, app, method, container, obj,"},{"line_number":1331,"context_line":"                      headers\u003dNone, body\u003dNone, query\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_11a8ae1d","line":1328,"updated":"2020-09-11 22:40:48.000000000","message":"KeyError is gross and not obvious at all","commit_id":"ae98c833bbcb7bf4d3c8034dd7f45d4de5889512"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":56,"context_line":"    MissingContentLength, InvalidStorageClass, S3NotImplemented, InvalidURI, \\"},{"line_number":57,"context_line":"    MalformedXML, InvalidRequest, RequestTimeout, InvalidBucketName, \\"},{"line_number":58,"context_line":"    BadDigest, AuthorizationHeaderMalformed, SlowDown, \\"},{"line_number":59,"context_line":"    AuthorizationQueryParametersError, ServiceUnavailable, BrokenMPU"},{"line_number":60,"context_line":"from swift.common.middleware.s3api.exception import NotS3Request"},{"line_number":61,"context_line":"from swift.common.middleware.s3api.utils import utf8encode, \\"},{"line_number":62,"context_line":"    S3Timestamp, mktime, MULTIUPLOAD_SUFFIX"}],"source_content_type":"text/x-python","patch_set":10,"id":"42b7cd9d_8ef3352b","side":"PARENT","line":59,"updated":"2024-02-26 23:26:13.000000000","message":"another reason maybe to move the error handlers to s3response is that\u0027s where we defined all these exceptions\n\nalthough the code that picks which s3 error to send for a given swift error isn\u0027t obviously the same as an s3-response; maybe it makes sense to import them here - we certainly didn\u0027t come close to eliminating even a majority.","commit_id":"50336c509877e7c325823498cd67bf6cf4f4bef3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":551,"context_line":"            \u0027check_signature\u0027: self.check_signature,"},{"line_number":552,"context_line":"        }"},{"line_number":553,"context_line":"        self.account \u003d None"},{"line_number":554,"context_line":"        self.user_id \u003d None"},{"line_number":555,"context_line":"        self.policy_index \u003d None"},{"line_number":556,"context_line":""},{"line_number":557,"context_line":"        # Avoids that swift.swob.Response replaces Location header value"}],"source_content_type":"text/x-python","patch_set":10,"id":"61db242e_91e06c29","side":"PARENT","line":554,"updated":"2024-02-26 23:26:13.000000000","message":"I see a lot of existing code interacting with this attribute and it seems to work fine.  The only difference I can spot with the property is the side effect of storing the value in the s3req environ - but that doesn\u0027t seem needed?","commit_id":"50336c509877e7c325823498cd67bf6cf4f4bef3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1406,"context_line":"            elif b\u0027quota\u0027 in err_msg:"},{"line_number":1407,"context_line":"                raise err_resp(err_msg)"},{"line_number":1408,"context_line":"            else:"},{"line_number":1409,"context_line":"                raise err_resp()"},{"line_number":1410,"context_line":""},{"line_number":1411,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"},{"line_number":1412,"context_line":"            err_str \u003d err_msg.decode(\u0027utf8\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"8e783251_1c100721","side":"PARENT","line":1409,"updated":"2024-02-26 23:26:13.000000000","message":"the isinstance stuff is gross for sure - this change seems like an improvement in the direction of standardization","commit_id":"50336c509877e7c325823498cd67bf6cf4f4bef3"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":false,"context_lines":[{"line_number":1406,"context_line":"            elif b\u0027quota\u0027 in err_msg:"},{"line_number":1407,"context_line":"                raise err_resp(err_msg)"},{"line_number":1408,"context_line":"            else:"},{"line_number":1409,"context_line":"                raise err_resp()"},{"line_number":1410,"context_line":""},{"line_number":1411,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"},{"line_number":1412,"context_line":"            err_str \u003d err_msg.decode(\u0027utf8\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"4a4190ed_272a1b76","side":"PARENT","line":1409,"in_reply_to":"8e783251_1c100721","updated":"2024-02-27 00:11:13.000000000","message":"Acknowledged","commit_id":"50336c509877e7c325823498cd67bf6cf4f4bef3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":559,"context_line":""},{"line_number":560,"context_line":"    @user_id.setter"},{"line_number":561,"context_line":"    def user_id(self, value):"},{"line_number":562,"context_line":"        self.environ[\u0027s3api.user_id\u0027] \u003d value"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def check_signature(self, secret):"},{"line_number":565,"context_line":"        secret \u003d utf8encode(secret)"}],"source_content_type":"text/x-python","patch_set":10,"id":"d0c6efd7_77d639af","line":562,"updated":"2024-02-26 23:26:13.000000000","message":"why are we stashing this here instead of as an attribute on the request?","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1260,"context_line":"        method \u003d method or self.environ[\u0027REQUEST_METHOD\u0027]"},{"line_number":1261,"context_line":""},{"line_number":1262,"context_line":"        if container is None:"},{"line_number":1263,"context_line":"            container \u003d self.container_name"},{"line_number":1264,"context_line":"        if obj is None:"},{"line_number":1265,"context_line":"            obj \u003d self.object_name"},{"line_number":1266,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"bf48da1b_9e92f7f0","line":1263,"updated":"2024-02-26 23:26:13.000000000","message":"these are kind of mis-leading; e.g. on an list-buckets request self.container_name is still None too\n\nmaybe slightly more obvoius as \n\n    -        if container is None:\n    -            container \u003d self.container_name\n    -        if obj is None:\n    -            obj \u003d self.object_name\n    +        container \u003d container or self.container_name\n    +        obj \u003d obj or self.object_name\n    \n... oh wow, excpet that\u0027s apparently not equivilent when obj kwarg gets set to explicitly empty string 😞\n\n    -        if container is None:\n    -            container \u003d self.container_name\n    -        if obj is None:\n    -            obj \u003d self.object_name\n    +        container \u003d container or self.container_name\n    +        obj \u003d self.object_name if obj is None else obj\n\n\n^ seems to pass tests 😕","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1268,"context_line":"                                   body\u003dbody, query\u003dquery)"},{"line_number":1269,"context_line":""},{"line_number":1270,"context_line":"        try:"},{"line_number":1271,"context_line":"            sw_resp \u003d sw_req.get_response(app)"},{"line_number":1272,"context_line":"        except swob.HTTPException as err:"},{"line_number":1273,"context_line":"            # Maybe a 422 from HashingInput? Put something in"},{"line_number":1274,"context_line":"            # s3api.backend_path - hopefully by now any modifications to the"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff1e311e_608161e1","line":1271,"updated":"2024-02-26 23:26:13.000000000","message":"IME the resp object created by get_response is going to have it\u0027s .request attribute set to sw_req after returning from this call\n\nwe shouldn\u0027t need to explicitly pass sw_req into our error handlers if they already get sw_resp","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1281,"context_line":"            _, self.account, _ \u003d split_path(sw_resp.environ[\u0027PATH_INFO\u0027],"},{"line_number":1282,"context_line":"                                            2, 3, True)"},{"line_number":1283,"context_line":"            # Update s3.backend_path from the response environ"},{"line_number":1284,"context_line":"            self.environ[\u0027s3api.backend_path\u0027] \u003d sw_resp.environ[\u0027PATH_INFO\u0027]"},{"line_number":1285,"context_line":""},{"line_number":1286,"context_line":"        # keep a record of the backend policy index so that the s3api can add"},{"line_number":1287,"context_line":"        # it to the headers of whatever response it returns, which may not"}],"source_content_type":"text/x-python","patch_set":10,"id":"362d4239_ee425484","line":1284,"updated":"2024-02-26 23:26:13.000000000","message":"the backend_path handled in the error and non-error branches look like a use-case for try/except/else/finally - but we\u0027re actaully pulling from different .environ in each branch (sw_req vs sw_resp)","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":true,"context_lines":[{"line_number":1281,"context_line":"            _, self.account, _ \u003d split_path(sw_resp.environ[\u0027PATH_INFO\u0027],"},{"line_number":1282,"context_line":"                                            2, 3, True)"},{"line_number":1283,"context_line":"            # Update s3.backend_path from the response environ"},{"line_number":1284,"context_line":"            self.environ[\u0027s3api.backend_path\u0027] \u003d sw_resp.environ[\u0027PATH_INFO\u0027]"},{"line_number":1285,"context_line":""},{"line_number":1286,"context_line":"        # keep a record of the backend policy index so that the s3api can add"},{"line_number":1287,"context_line":"        # it to the headers of whatever response it returns, which may not"}],"source_content_type":"text/x-python","patch_set":10,"id":"4004c99e_00bd1e84","line":1284,"in_reply_to":"362d4239_ee425484","updated":"2024-02-27 00:11:13.000000000","message":"I had the same thought at some point, and was frustrated by that difference. Probably ought to leverage that `sw_resp.request is sw_req` invariant you noticed -- it\u0027ll be interesting to see if tests notice.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1286,"context_line":"        # keep a record of the backend policy index so that the s3api can add"},{"line_number":1287,"context_line":"        # it to the headers of whatever response it returns, which may not"},{"line_number":1288,"context_line":"        # necessarily be this resp."},{"line_number":1289,"context_line":"        self.policy_index \u003d get_policy_index(sw_req.headers, sw_resp.headers)"},{"line_number":1290,"context_line":"        resp \u003d S3Response.from_swift_resp(sw_resp)"},{"line_number":1291,"context_line":"        status \u003d resp.status_int  # pylint: disable-msg\u003dE1101"},{"line_number":1292,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"cb8e3519_73c0cfe2","line":1289,"updated":"2024-02-26 23:26:13.000000000","message":"it seems like there\u0027s a lot of attributes we backfill on the s3request after we make the swift_req\n\nthere\u0027s doesn\u0027t seem to be a clear pattern on when they belong in the s3req environ","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d571910b8722a806768dbf2fd3b349ebf6a4edb9","unresolved":true,"context_lines":[{"line_number":1286,"context_line":"        # keep a record of the backend policy index so that the s3api can add"},{"line_number":1287,"context_line":"        # it to the headers of whatever response it returns, which may not"},{"line_number":1288,"context_line":"        # necessarily be this resp."},{"line_number":1289,"context_line":"        self.policy_index \u003d get_policy_index(sw_req.headers, sw_resp.headers)"},{"line_number":1290,"context_line":"        resp \u003d S3Response.from_swift_resp(sw_resp)"},{"line_number":1291,"context_line":"        status \u003d resp.status_int  # pylint: disable-msg\u003dE1101"},{"line_number":1292,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"90e61823_7c923082","line":1289,"in_reply_to":"49cd22c5_50aa9e09","updated":"2024-02-27 14:43:46.000000000","message":"yeah I think that patch did toy with the idea of using the req environ but ultimately realized it just wanted to put the value in the resp headers and using an attribute was sufficient and prefered to the request environ (which typically suggests the state needs to outlive the mw\u0027s request object)","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":true,"context_lines":[{"line_number":1286,"context_line":"        # keep a record of the backend policy index so that the s3api can add"},{"line_number":1287,"context_line":"        # it to the headers of whatever response it returns, which may not"},{"line_number":1288,"context_line":"        # necessarily be this resp."},{"line_number":1289,"context_line":"        self.policy_index \u003d get_policy_index(sw_req.headers, sw_resp.headers)"},{"line_number":1290,"context_line":"        resp \u003d S3Response.from_swift_resp(sw_resp)"},{"line_number":1291,"context_line":"        status \u003d resp.status_int  # pylint: disable-msg\u003dE1101"},{"line_number":1292,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"49cd22c5_50aa9e09","line":1289,"in_reply_to":"cb8e3519_73c0cfe2","updated":"2024-02-27 00:11:13.000000000","message":"The `policy_index` attr [was introduced quite recently](https://github.com/openstack/swift/commit/60c04f116b41f97a5dcf9c07ef6e77ae9bdfe61e) -- if this patch had merged first, the obvious thing would have been to stuff it in the environ. I can change that to look more like `user_id`, but I don\u0027t think you\u0027re sold on that yet...","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1300,"context_line":"                    self.user_id \u003d self.user_id.encode(\u0027utf8\u0027)"},{"line_number":1301,"context_line":"            else:"},{"line_number":1302,"context_line":"                # tempauth"},{"line_number":1303,"context_line":"                self.user_id \u003d self.access_key"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"        success_codes \u003d self._swift_success_codes(method, container, obj)"},{"line_number":1306,"context_line":"        if status in success_codes:"}],"source_content_type":"text/x-python","patch_set":10,"id":"e2c2c127_07a12fca","line":1303,"updated":"2024-02-26 23:26:13.000000000","message":"so this is probably mainly where we set the user_id - not clear on the value of putting it in the req environ as a side-effect.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1313,"context_line":"        else:"},{"line_number":1314,"context_line":"            req_type \u003d \u0027object\u0027"},{"line_number":1315,"context_line":"        sw_req.environ[\u0027s3api.container\u0027] \u003d container"},{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"}],"source_content_type":"text/x-python","patch_set":10,"id":"dfd4fe1c_c0ae811e","line":1316,"updated":"2024-02-26 23:26:13.000000000","message":"just making sure I understand what everything is - this is annotating these values in the sw_req environ - mostly so we don\u0027t have to re-parse them out of `PATH_INFO` in the error handlers?\n\nI can\u0027t say for sure; but it might make sense for this to happen in `to_swift_req`\n\nor maybe just don\u0027t use the sub-req environ to pass around paramaters.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1314,"context_line":"            req_type \u003d \u0027object\u0027"},{"line_number":1315,"context_line":"        sw_req.environ[\u0027s3api.container\u0027] \u003d container"},{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"},{"line_number":1320,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"127854cc_093e0257","line":1317,"updated":"2024-02-26 23:26:13.000000000","message":"I guess since our handler\u0027s won\u0027t have the s3req we have to pass them some of the original request environ by hand","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1315,"context_line":"        sw_req.environ[\u0027s3api.container\u0027] \u003d container"},{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"}],"source_content_type":"text/x-python","patch_set":10,"id":"c810f5df_19b84201","line":1318,"updated":"2024-02-26 23:26:13.000000000","message":"this kind of dynamism is typically a turn off for me; the most convinencing description of the issues this kind of code causes I\u0027ve found is \"the grep test\" [1]\n\n1. https://jamie-wong.com/2013/07/12/grep-test/","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"},{"line_number":1322,"context_line":"            err_str \u003d resp.body.decode(\u0027utf8\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"786509ee_fc311a3e","line":1319,"updated":"2024-02-26 23:26:13.000000000","message":"why don\u0027t we pass in the s3req or s3resp?  instead of dirying up the sw_req environ with a bunch of attributes from the s3req?","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":true,"context_lines":[{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"},{"line_number":1322,"context_line":"            err_str \u003d resp.body.decode(\u0027utf8\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"b4051595_c055f45d","line":1319,"in_reply_to":"786509ee_fc311a3e","updated":"2024-02-27 00:11:13.000000000","message":"We already have (and in at least one case *need*) the `s3api.*` namespace in the environ; it\u0027s not clear to me why we should view that as \"dirtying\"","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d571910b8722a806768dbf2fd3b349ebf6a4edb9","unresolved":true,"context_lines":[{"line_number":1316,"context_line":"        sw_req.environ[\u0027s3api.object\u0027] \u003d obj"},{"line_number":1317,"context_line":"        sw_req.environ[\u0027s3api.user_id\u0027] \u003d self.user_id"},{"line_number":1318,"context_line":"        getattr(s3response, \u0027handle_%s_%s_error\u0027 % (req_type, method))("},{"line_number":1319,"context_line":"            sw_req, sw_resp, app)"},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"        if status \u003d\u003d HTTP_BAD_REQUEST:"},{"line_number":1322,"context_line":"            err_str \u003d resp.body.decode(\u0027utf8\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"43b490cb_95856b75","line":1319,"in_reply_to":"b4051595_c055f45d","updated":"2024-02-27 14:43:46.000000000","message":"I think you\u0027re conflating the s3req.environ (the one the user/pipeline gave us) and the sw_req.environ - the one we make up and doesn\u0027t live after we return from this method.\n\nIf we need to pass state between mw\u0027s - yes req.environ is the way to go.\n\nIf we need to pass info between function calls - I think a function paramater is more obvious than the request environ.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1340,"context_line":"        if status in (HTTP_RATE_LIMITED, HTTP_TOO_MANY_REQUESTS):"},{"line_number":1341,"context_line":"            if self.conf.ratelimit_as_client_error:"},{"line_number":1342,"context_line":"                raise SlowDown(status\u003d\u0027429 Slow Down\u0027)"},{"line_number":1343,"context_line":"            raise SlowDown()"},{"line_number":1344,"context_line":""},{"line_number":1345,"context_line":"        raise InternalError(\u0027unexpected status code %d\u0027 % status)"},{"line_number":1346,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"6c1e52ab_4827917c","line":1343,"updated":"2024-02-26 23:26:13.000000000","message":"I guess the order of operations is designed to allow special handling of these \"generic\" errors in the `handle_\u003creq_tye\u003e_\u003cmetod\u003e_error` handlers. \n\nIt still feels like we\u0027re spreading the response error handling around two modules.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1341,"context_line":"            if self.conf.ratelimit_as_client_error:"},{"line_number":1342,"context_line":"                raise SlowDown(status\u003d\u0027429 Slow Down\u0027)"},{"line_number":1343,"context_line":"            raise SlowDown()"},{"line_number":1344,"context_line":""},{"line_number":1345,"context_line":"        raise InternalError(\u0027unexpected status code %d\u0027 % status)"},{"line_number":1346,"context_line":""},{"line_number":1347,"context_line":"    def get_response(self, app, method\u003dNone, container\u003dNone, obj\u003dNone,"}],"source_content_type":"text/x-python","patch_set":10,"id":"749d4fd0_d44fb9cb","line":1344,"updated":"2024-02-26 23:26:13.000000000","message":"FWIW s3api unittests seem happy if I move the `handle_\u003creq_tye\u003e_\u003cmetod\u003e_error` down here; so I guess there\u0027s not currently any overlap between the per resource/method handlers and the generic handlers.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":true,"context_lines":[{"line_number":1341,"context_line":"            if self.conf.ratelimit_as_client_error:"},{"line_number":1342,"context_line":"                raise SlowDown(status\u003d\u0027429 Slow Down\u0027)"},{"line_number":1343,"context_line":"            raise SlowDown()"},{"line_number":1344,"context_line":""},{"line_number":1345,"context_line":"        raise InternalError(\u0027unexpected status code %d\u0027 % status)"},{"line_number":1346,"context_line":""},{"line_number":1347,"context_line":"    def get_response(self, app, method\u003dNone, container\u003dNone, obj\u003dNone,"}],"source_content_type":"text/x-python","patch_set":10,"id":"5c6defdd_2716f06b","line":1344,"in_reply_to":"749d4fd0_d44fb9cb","updated":"2024-02-27 00:11:13.000000000","message":"I feel like in general, there *shouldn\u0027t* be, though I could maybe see some cases where it could be useful to override.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":1487,"context_line":"                self.user_id \u003d self.user_id.encode(\u0027utf8\u0027)"},{"line_number":1488,"context_line":"        else:"},{"line_number":1489,"context_line":"            # tempauth"},{"line_number":1490,"context_line":"            self.user_id \u003d self.access_key"},{"line_number":1491,"context_line":""},{"line_number":1492,"context_line":"        sw_req.environ.get(\u0027swift.authorize\u0027, lambda req: None)(sw_req)"},{"line_number":1493,"context_line":"        self.environ[\u0027swift_owner\u0027] \u003d sw_req.environ.get(\u0027swift_owner\u0027, False)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3659f80a_a89dc28f","line":1490,"updated":"2024-02-26 23:26:13.000000000","message":"this reads really similar to the user_id handling in _get_response\n\ni\u0027d really like to understand the motivation/justification for moving it to a property and storing it in the environ","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"}],"swift/common/middleware/s3api/s3response.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":772,"context_line":""},{"line_number":773,"context_line":""},{"line_number":774,"context_line":"def handle_account_GET_error(req, resp, app):"},{"line_number":775,"context_line":"    pass"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"def handle_container_HEAD_error(req, resp, app):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3b33a2fd_e3332b7c","line":775,"updated":"2024-02-26 23:26:13.000000000","message":"is a GET the only possilbe method for the account resource?\n\nWhy don\u0027t we need a generic/default noop handler for when the getattr fails because of some unexpected method like PATCH or something...","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"5e3808cd8a496ac19abec6e43aa2af3e24848dbc","unresolved":true,"context_lines":[{"line_number":874,"context_line":"    if resp.status_int \u003d\u003d http.HTTP_NOT_FOUND:"},{"line_number":875,"context_line":"        raise NoSuchKey(req.environ[\u0027s3api.object\u0027])"},{"line_number":876,"context_line":"    if resp.status_int \u003d\u003d http.HTTP_CONFLICT:"},{"line_number":877,"context_line":"        raise ServiceUnavailable()"}],"source_content_type":"text/x-python","patch_set":10,"id":"e713928e_374290e1","line":877,"updated":"2024-02-26 23:26:13.000000000","message":"the main reason I can think of for having these be module level functions is for unittesting - but I don\u0027t see this change adding any new targeted tests.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"8566924d3a099ee9c1e6941005002cb1ce97d39e","unresolved":true,"context_lines":[{"line_number":874,"context_line":"    if resp.status_int \u003d\u003d http.HTTP_NOT_FOUND:"},{"line_number":875,"context_line":"        raise NoSuchKey(req.environ[\u0027s3api.object\u0027])"},{"line_number":876,"context_line":"    if resp.status_int \u003d\u003d http.HTTP_CONFLICT:"},{"line_number":877,"context_line":"        raise ServiceUnavailable()"}],"source_content_type":"text/x-python","patch_set":10,"id":"16338f9b_6d3a09fb","line":877,"in_reply_to":"e713928e_374290e1","updated":"2024-02-27 00:11:13.000000000","message":"It was a refactor -- I was sad I had to touch tests *at all*.","commit_id":"727ef3bc01934e49105df42143add64a610a3ca1"}]}