)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"43ff7be5f4fe083ae3150612f51af3d3f05fc96a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Trivial: fix marker checking when value is None"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There are cases where the marker_value is None during pagination. When"},{"line_number":10,"context_line":"marker_value is None, we cannot do comparison using \u0027\u003c\u0027 or \u0027\u003e\u0027"},{"line_number":11,"context_line":"operators. This patch adds a check if that is the case and skip the"},{"line_number":12,"context_line":"comparison if so."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1ac06dbe_0902766f","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":46},"updated":"2016-08-22 08:29:57.000000000","message":"Could you please elaborate on this? Are referring to the case when you pass multiple sort keys and the value of one of those can be None?","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"ac77d65ffbf2e9bfe8a874a56588a34b82fad9e5","unresolved":false,"context_lines":[{"line_number":10,"context_line":"marker_value is None, we cannot do comparison using \u0027\u003c\u0027 or \u0027\u003e\u0027"},{"line_number":11,"context_line":"operators. This patch adds a check if that is the case and skip the"},{"line_number":12,"context_line":"comparison if so."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: Iea2cd0bb2556b0b15a0baaa76ef522a3097f9928"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1ac06dbe_eae36c31","line":13,"updated":"2016-08-22 15:58:05.000000000","message":"nit: do you mind filing a bug for this?","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"}],"oslo_db/sqlalchemy/utils.py":[{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"43ff7be5f4fe083ae3150612f51af3d3f05fc96a","unresolved":false,"context_lines":[{"line_number":164,"context_line":"                crit_attrs.append((model_attr \u003d\u003d marker_values[j]))"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"            model_attr \u003d getattr(model, sort_keys[i])"},{"line_number":167,"context_line":"            if marker_values[i] is not None:"},{"line_number":168,"context_line":"                if sort_dirs[i].startswith(\u0027desc\u0027):"},{"line_number":169,"context_line":"                    crit_attrs.append((model_attr \u003c marker_values[i]))"},{"line_number":170,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"1ac06dbe_8945c6b4","line":167,"updated":"2016-08-22 08:29:57.000000000","message":"So, if there is only one sort key and its value in the marker is None, than we completely skip the filtering. Doesn\u0027t it break pagination?","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"ac77d65ffbf2e9bfe8a874a56588a34b82fad9e5","unresolved":false,"context_lines":[{"line_number":164,"context_line":"                crit_attrs.append((model_attr \u003d\u003d marker_values[j]))"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"            model_attr \u003d getattr(model, sort_keys[i])"},{"line_number":167,"context_line":"            if marker_values[i] is not None:"},{"line_number":168,"context_line":"                if sort_dirs[i].startswith(\u0027desc\u0027):"},{"line_number":169,"context_line":"                    crit_attrs.append((model_attr \u003c marker_values[i]))"},{"line_number":170,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"1ac06dbe_2a862465","line":167,"in_reply_to":"1ac06dbe_1f5bc488","updated":"2016-08-22 15:58:05.000000000","message":"Ok, thanks for clarifications! \n\nI guess I see your point here. But I\u0027m not sure that silently skipping a sorting key is a good idea either: if there are *no* non-null values corresponding to sorting keys we should probably fail early instead of producing a weird result (limit works, but without filtering you\u0027ll see the very first page again at some point)","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"},{"author":{"_account_id":18653,"name":"Milan Kováčik","email":"mkovacik@redhat.com","username":"dparalen"},"change_message_id":"07f45633bea17bbbb692474ac5858e4c5b918dc0","unresolved":false,"context_lines":[{"line_number":164,"context_line":"                crit_attrs.append((model_attr \u003d\u003d marker_values[j]))"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"            model_attr \u003d getattr(model, sort_keys[i])"},{"line_number":167,"context_line":"            if marker_values[i] is not None:"},{"line_number":168,"context_line":"                if sort_dirs[i].startswith(\u0027desc\u0027):"},{"line_number":169,"context_line":"                    crit_attrs.append((model_attr \u003c marker_values[i]))"},{"line_number":170,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a8ec9b2_37068a1d","line":167,"in_reply_to":"1ac06dbe_2a862465","updated":"2016-09-15 16:53:04.000000000","message":"@Roman\nbased on the above case, if the sort was based only on the created_at column and the last 500 items in a 1000 rows table had null/None as a value in that column, how should the pagination fail soon? I guess serving first 499 paged by 50 and then failing isn\u0027t cool either. \n\nI\u0027d say if the nulls don\u0027t affect the sort it should be safe to ignore them. If there\u0027s no other key provided by the user of the library, it\u0027s the users responsibility; I\u0027d leave that for the docstring.\n\nPlease note also in my case (Ironic Inspector node table) I need to keep track of the started_at and finished_at records, the later will always be null for substantial amount of the nodes I keep in the db.\n\nCheers,\nmilan","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"},{"author":{"_account_id":8246,"name":"Qiming Teng","email":"tengqm@outlook.com","username":"tengqm"},"change_message_id":"a14bb440ac19acb499ff1cbc9ffda65b24e88bb1","unresolved":false,"context_lines":[{"line_number":164,"context_line":"                crit_attrs.append((model_attr \u003d\u003d marker_values[j]))"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"            model_attr \u003d getattr(model, sort_keys[i])"},{"line_number":167,"context_line":"            if marker_values[i] is not None:"},{"line_number":168,"context_line":"                if sort_dirs[i].startswith(\u0027desc\u0027):"},{"line_number":169,"context_line":"                    crit_attrs.append((model_attr \u003c marker_values[i]))"},{"line_number":170,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"1ac06dbe_1f5bc488","line":167,"in_reply_to":"1ac06dbe_8945c6b4","updated":"2016-08-22 09:00:38.000000000","message":"Right. It looks like it is breaking pagination. Because the comparison on line 169 and 171 don\u0027t make any sense anyway.\n\nHowever, in real-life usage, we always use object ID as one of the last sorting key so we can ensure the result is always predictable.\n\nHere is how this problem was found.\n\nWe have entities in DB with columns \u0027created_at\u0027 and \u0027id\u0027. The \u0027created_at\u0027 column will remain None until the resource is actually created. Now we want to list the entities (paginated). We pass in sort keys and sort dirs as this:\n\n   sort_keys \u003d [\u0027created_at\u0027, \u0027id\u0027]\n   sort_dirs \u003d [\u0027desc-nullslast\u0027, \u0027asc\u0027]\n\nSomewhere near the end of the listing, there are entities with \u0027created_at\u0027 column set to None. ESPECIALLY the VERY LAST item, where the marker_values[i] gets a value of None. The current logic is trying to check if \u0027created_at\u0027 \u003e None or \u0027created_at\u0027 \u003c None. That is obviously wrong.\n\nIn the revised logic, we are not breaking any existing use cases, just check if a marker_value[i] is None or not and skip the comparison if so.\n\nThe pagination STILL WORKS because we have \u0027limit\u0027 that is still effective on line 180.","commit_id":"5de75db79fd1af0c0a44eb2c4cbb4cdbbc0159ce"},{"author":{"_account_id":18653,"name":"Milan Kováčik","email":"mkovacik@redhat.com","username":"dparalen"},"change_message_id":"5d85ca71d85755517ad8c2e00ca8e9f7f7f2d830","unresolved":false,"context_lines":[{"line_number":90,"context_line":"    :param model: the ORM model class"},{"line_number":91,"context_line":"    :param limit: maximum number of items to return"},{"line_number":92,"context_line":"    :param sort_keys: array of attributes by which results should be sorted"},{"line_number":93,"context_line":"    :param marker: the last item of the previous page; we returns the next"},{"line_number":94,"context_line":"                    results after this value."},{"line_number":95,"context_line":"    :param sort_dir: direction in which results should be sorted (asc, desc)"},{"line_number":96,"context_line":"                     suffix -nullsfirst, -nullslast can be added to defined"},{"line_number":97,"context_line":"                     the ordering of null values"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a8ec9b2_974d9e40","line":94,"range":{"start_line":93,"start_character":4,"end_line":94,"end_character":45},"updated":"2016-09-15 17:00:05.000000000","message":"Please, document the key skipping in case a null/None value of a marker column/attribute.","commit_id":"889c060a238c9fbd2166cf7a373bba6465b8c849"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"731a5e1100c9412baa190339fc09e321307e8666","unresolved":false,"context_lines":[{"line_number":228,"context_line":"            # NOTE: We skip the marker value comparison if marker_values[i] is"},{"line_number":229,"context_line":"            #       None, for two reasons: 1) the comparison operators below"},{"line_number":230,"context_line":"            #       (\u0027\u003c\u0027, \u0027\u003e\u0027) are not applicable on None value; 2) this is"},{"line_number":231,"context_line":"            #       safe because we can assume the primary key is included in"},{"line_number":232,"context_line":"            #       sort_key, thus checked as (one of) marker values."},{"line_number":233,"context_line":"            if marker_values[i] is not None:"},{"line_number":234,"context_line":"                for j in range(i):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a71b18c_0094ae40","line":231,"range":{"start_line":231,"start_character":33,"end_line":231,"end_character":77},"updated":"2016-12-06 09:48:55.000000000","message":"We also allow unique keys, not only the primary keys, so NULL value is possible, but highly unlikely","commit_id":"b3869d04cff7071c1226758eb8b58fde9eba5b8d"}]}