)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"f70616d70d6a73e47163075bbf7d6ddb44498cc0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"91181bcf_2282b6be","updated":"2025-12-08 10:22:15.000000000","message":"The fix is on the right track but needs to properly handle query parameters and fragments. Once that\u0027s addressed and test coverage is added, this should be good to go.","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"},{"author":{"_account_id":38656,"name":"Vishnu C","display_name":"Vishnu Chandrasenan","email":"vischan2@cisco.com","username":"vischan2"},"change_message_id":"3106a13d2574c3eaef25126f836a935765d69e1d","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"7bf59fc4_f245cd09","updated":"2025-12-08 16:05:26.000000000","message":"Addressed review feedback: implemented hybrid parsing approach to include scenarios involving path, fragment and added comprehensive test coverage. Thank you for the feedback.","commit_id":"9a11e6c10cc1ddeb5951e583a02b56120f3da8bd"},{"author":{"_account_id":38656,"name":"Vishnu C","display_name":"Vishnu Chandrasenan","email":"vischan2@cisco.com","username":"vischan2"},"change_message_id":"705c877729f08dfba2fbbe3f7d34359a6b38ee04","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c64874e0_d6c0d7c2","in_reply_to":"7bf59fc4_f245cd09","updated":"2025-12-09 21:03:58.000000000","message":"Done","commit_id":"9a11e6c10cc1ddeb5951e583a02b56120f3da8bd"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"36d4c3c51ebd66b90c4e3f946d439563aab56a1b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"36add7ef_0f061fad","updated":"2025-12-12 00:05:44.000000000","message":"I had a surprisingly difficult time finding what exactly the format of one of these secrets is.  Assuming that this is correct [0], a secret fits this regex:\n\n```\n[A-Za-z0-9/\\+\u003d]{40}\n```\n\nSo at least we don\u0027t need to worry about a \u0027@\u0027 in a secret (though I think your rfind approach would be able to handle it correctly).\n\nIf you want to do a followup to this patch, I suggest adding tests for all the legal characters.  That way if someone refactors the code at some point and makes a mistake, we\u0027ll be able to catch it.  But nice job on this patch and tests!\n\n[0] https://github.com/awslabs/git-secrets/blob/7d6b970cbd3c216353cb22b383b70c150140662e/git-secrets#L242","commit_id":"cbc4d9f5b1942308be32544825d0fcae746b4059"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"a8adc3708fb79371f8ad969d63d2edbb3f3e4656","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"fcc0ce6e_a1f2ab98","updated":"2025-12-09 07:15:36.000000000","message":"Looks good to me, thank you!!","commit_id":"cbc4d9f5b1942308be32544825d0fcae746b4059"},{"author":{"_account_id":38656,"name":"Vishnu C","display_name":"Vishnu Chandrasenan","email":"vischan2@cisco.com","username":"vischan2"},"change_message_id":"705c877729f08dfba2fbbe3f7d34359a6b38ee04","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"762b50ff_18a52594","updated":"2025-12-09 21:03:58.000000000","message":"This change has \u0027+2, +1 Code-Review\u0027 and \u0027Verified +1\u0027. Ready for \u0027Workflow +1\u0027 when a glance-core team has a moment. Thanks.","commit_id":"cbc4d9f5b1942308be32544825d0fcae746b4059"}],"glance/common/store_utils.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"f70616d70d6a73e47163075bbf7d6ddb44498cc0","unresolved":true,"context_lines":[{"line_number":249,"context_line":"    scheme, credentials_and_location \u003d url.split(\u0027://\u0027, 1)"},{"line_number":250,"context_line":"    separator_position \u003d credentials_and_location.rfind(\u0027@\u0027)"},{"line_number":251,"context_line":"    host_and_path \u003d credentials_and_location[separator_position + 1:]"},{"line_number":252,"context_line":"    new_url \u003d \"%s://%s:%s@%s\" % (scheme, new_access_key, new_secret_key,"},{"line_number":253,"context_line":"                                 host_and_path)"},{"line_number":254,"context_line":"    return new_url"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"2a03990c_7a8340b5","line":253,"range":{"start_line":252,"start_character":4,"end_line":253,"end_character":47},"updated":"2025-12-08 10:22:15.000000000","message":"The current implementation loses query parameters and fragments from URLs. The old code used `urlparse.urlunparse()` which properly preserved these components, but the new code only does string formatting.\n\nIf the original URL is `s3://key:secret@host.com/bucket/obj?param\u003dvalue#fragment`, the `host_and_path` will include `host.com/bucket/obj?param\u003dvalue#fragment`, which is incorrect. The query and fragment should be handled separately.\n\n\n\n\n```python\ndef _update_s3_url(url, new_access_key, new_secret_key):\n    \"\"\"Update S3 URL with new credentials.\n    \n    Uses rfind(\u0027@\u0027) to locate the last \u0027@\u0027 which separates credentials\n    from the host. This approach correctly handles special characters like \u0027/\u0027\n    in the secret access key, which urlparse misinterprets as path separators.\n    \"\"\"\n    # Parse URL to get query and fragment (these are parsed correctly)\n    parsed \u003d urlparse.urlparse(url)\n    scheme \u003d parsed.scheme\n    \n    # Find the last \u0027@\u0027 in the original URL (before query/fragment)\n    # This handles the case where secret key contains \u0027/\u0027\n    url_without_query \u003d url.split(\u0027?\u0027)[0].split(\u0027#\u0027)[0]\n    separator_position \u003d url_without_query.rfind(\u0027@\u0027)\n    \n    if separator_position \u003d\u003d -1:\n        # No credentials - shouldn\u0027t happen for S3, but handle gracefully\n        new_netloc \u003d \"%s:%s@%s\" % (new_access_key, new_secret_key, parsed.netloc)\n        path_part \u003d parsed.path  # Use parsed.path when no \u0027@\u0027 found\n    else:\n        # Extract host+path from after \u0027@\u0027\n        host_and_path \u003d url_without_query[separator_position + 1:]\n        # Remove scheme part if it\u0027s still there\n        if \u0027://\u0027 in host_and_path:\n            host_and_path \u003d host_and_path.split(\u0027://\u0027, 1)[1]\n        \n        # Separate host from path\n        first_slash \u003d host_and_path.find(\u0027/\u0027)\n        if first_slash \u003d\u003d -1:\n            host_part \u003d host_and_path\n            path_part \u003d \u0027\u0027\n        else:\n            host_part \u003d host_and_path[:first_slash]\n            path_part \u003d host_and_path[first_slash:]\n        \n        new_netloc \u003d \"%s:%s@%s\" % (new_access_key, new_secret_key, host_part)\n    \n    # Rebuild URL preserving query and fragment\n    return urlparse.urlunparse((\n        scheme,\n        new_netloc,\n        path_part,\n        parsed.params,\n        parsed.query,\n        parsed.fragment\n    ))\n```","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"},{"author":{"_account_id":38656,"name":"Vishnu C","display_name":"Vishnu Chandrasenan","email":"vischan2@cisco.com","username":"vischan2"},"change_message_id":"3106a13d2574c3eaef25126f836a935765d69e1d","unresolved":true,"context_lines":[{"line_number":249,"context_line":"    scheme, credentials_and_location \u003d url.split(\u0027://\u0027, 1)"},{"line_number":250,"context_line":"    separator_position \u003d credentials_and_location.rfind(\u0027@\u0027)"},{"line_number":251,"context_line":"    host_and_path \u003d credentials_and_location[separator_position + 1:]"},{"line_number":252,"context_line":"    new_url \u003d \"%s://%s:%s@%s\" % (scheme, new_access_key, new_secret_key,"},{"line_number":253,"context_line":"                                 host_and_path)"},{"line_number":254,"context_line":"    return new_url"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5765e956_ffeeeb4e","line":253,"range":{"start_line":252,"start_character":4,"end_line":253,"end_character":47},"in_reply_to":"2a03990c_7a8340b5","updated":"2025-12-08 16:05:26.000000000","message":"Thanks for the detailed review and for catching the scenario involving query and fragment. I have updated the patch as per suggestion to include:\n\n- Implement _update_s3_url() using the suggested hybrid approach that correctly handles special characters in credentials while preserving query and fragment. \n- Added handling for URLs without existing credentials\n- Added detailed docstring explaining why this hybrid approach using both urlparse and manual string parsing is needed with a concrete example","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"a8adc3708fb79371f8ad969d63d2edbb3f3e4656","unresolved":false,"context_lines":[{"line_number":249,"context_line":"    scheme, credentials_and_location \u003d url.split(\u0027://\u0027, 1)"},{"line_number":250,"context_line":"    separator_position \u003d credentials_and_location.rfind(\u0027@\u0027)"},{"line_number":251,"context_line":"    host_and_path \u003d credentials_and_location[separator_position + 1:]"},{"line_number":252,"context_line":"    new_url \u003d \"%s://%s:%s@%s\" % (scheme, new_access_key, new_secret_key,"},{"line_number":253,"context_line":"                                 host_and_path)"},{"line_number":254,"context_line":"    return new_url"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"0b9815c9_58ce567b","line":253,"range":{"start_line":252,"start_character":4,"end_line":253,"end_character":47},"in_reply_to":"5765e956_ffeeeb4e","updated":"2025-12-09 07:15:36.000000000","message":"Done","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"}],"glance/tests/unit/common/test_utils.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"f70616d70d6a73e47163075bbf7d6ddb44498cc0","unresolved":true,"context_lines":[{"line_number":1324,"context_line":"        url \u003d \u0027s3+https://KEY:a/b/c@bucket/object\u0027"},{"line_number":1325,"context_line":"        result \u003d store_utils._update_s3_url(url, \u0027NEW\u0027, \u0027x/y/z\u0027)"},{"line_number":1326,"context_line":"        expected \u003d \u0027s3+https://NEW:x/y/z@bucket/object\u0027"},{"line_number":1327,"context_line":"        self.assertEqual(result, expected)"},{"line_number":1328,"context_line":""},{"line_number":1329,"context_line":"    def test_construct_s3_url(self):"},{"line_number":1330,"context_line":"        \"\"\"Test _construct_s3_url with all attributes present.\"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"07bb9a25_f5f61d1b","line":1327,"updated":"2025-12-08 10:22:15.000000000","message":"Please ensure the test cases cover:\n- URLs with query parameters: `s3://key:secret@host.com/bucket/obj?param\u003dvalue`\n- URLs with fragments: `s3://key:secret@host.com/bucket/obj#fragment`\n- URLs with both: `s3://key:secret@host.com/bucket/obj?param\u003dvalue#fragment`\n- Secret keys with multiple slashes: `s3://key:secret/with/many/slashes@host.com/bucket/obj`\n- Hosts with port numbers: `s3://key:secret@host.com:8080/bucket/obj`","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"},{"author":{"_account_id":38656,"name":"Vishnu C","display_name":"Vishnu Chandrasenan","email":"vischan2@cisco.com","username":"vischan2"},"change_message_id":"3106a13d2574c3eaef25126f836a935765d69e1d","unresolved":true,"context_lines":[{"line_number":1324,"context_line":"        url \u003d \u0027s3+https://KEY:a/b/c@bucket/object\u0027"},{"line_number":1325,"context_line":"        result \u003d store_utils._update_s3_url(url, \u0027NEW\u0027, \u0027x/y/z\u0027)"},{"line_number":1326,"context_line":"        expected \u003d \u0027s3+https://NEW:x/y/z@bucket/object\u0027"},{"line_number":1327,"context_line":"        self.assertEqual(result, expected)"},{"line_number":1328,"context_line":""},{"line_number":1329,"context_line":"    def test_construct_s3_url(self):"},{"line_number":1330,"context_line":"        \"\"\"Test _construct_s3_url with all attributes present.\"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"2141526c_da17d237","line":1327,"in_reply_to":"07bb9a25_f5f61d1b","updated":"2025-12-08 16:05:26.000000000","message":"Added comprehensive test coverage for all requested edge cases (query params, fragments, ports, multiple slashes). \n\nVerified that all 18 S3 credential tests pass locally.","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"a8adc3708fb79371f8ad969d63d2edbb3f3e4656","unresolved":false,"context_lines":[{"line_number":1324,"context_line":"        url \u003d \u0027s3+https://KEY:a/b/c@bucket/object\u0027"},{"line_number":1325,"context_line":"        result \u003d store_utils._update_s3_url(url, \u0027NEW\u0027, \u0027x/y/z\u0027)"},{"line_number":1326,"context_line":"        expected \u003d \u0027s3+https://NEW:x/y/z@bucket/object\u0027"},{"line_number":1327,"context_line":"        self.assertEqual(result, expected)"},{"line_number":1328,"context_line":""},{"line_number":1329,"context_line":"    def test_construct_s3_url(self):"},{"line_number":1330,"context_line":"        \"\"\"Test _construct_s3_url with all attributes present.\"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"1c753256_63cbc3a5","line":1327,"in_reply_to":"2141526c_da17d237","updated":"2025-12-09 07:15:36.000000000","message":"Done","commit_id":"fc1a3f1f754ffe3abf935c066250e9a7f739f034"}]}