Add pagination and bulk update support for User(discord user) serializer. by RohanJnr · Pull Request #378 · python-discord/site

RohanJnr · 2020-08-26T17:39:39Z

closes #375

Implementation

Pagination

Using built in PageNumberPagination with the default page size being 10,000 user objects(dicts) per page.
2 query parameteres to control page number and page size.

Bulk Update

New endpoint: /bot/users/bulk_patch
A UserListSerializer for UserSerializer has been implemented to support bulk updates.(ref to docs: drf-Customizing multiple update)

Additional

Replace restframework_bulk package by implementing bulk creation of User models.

Tests for new features.

complete

Pagination is done via PageNumberPagination, i.e, each page contains a specific number of `user` objects.

implemented a method to handle bulk updates on user model via a new endpoint: /bot/users/bulk_patch

ghost · 2020-08-26T17:39:43Z

Thank you for contributing to Python Discord!

Please check out the following documents:

Join the Discord server! - If you have not already, join the Discord server and introduce yourself in our #dev-contrib channel.
Python Discord Contributor Wiki - A set of resources about setting up our services
Python Discord Code of Conduct - Code of conduct for our community
CONTRIBUTING.md - Guidelines on contributing to this repository

…ser object from database.

lemonsaurus · 2020-08-26T23:23:31Z

Okay, before I really jump into this review, I need you to take a look at the BulkSerializerMixin and figure out whether we still need it. It looks like it was implemented specifically to solve bulk input, and we even have a dependency just for that. So I'd like you to justify your approach over that approach, and consider whether this PR should remove the dependency that includes the BulkSerializerMixin.

lemonsaurus · 2020-08-26T23:25:09Z

To be clear, I'm not saying you should necessarily be using BulkSerializerMixin instead, but you need to at least figure out what it does and how it does it so we can make an informed decision about whether to keep it or get rid of it.

RohanJnr · 2020-08-27T13:44:05Z

I managed to simplify the code in the bulk update PR, I no longer need to override the to_internal_value function, I just had to define an id field explicitly as mentioned in the docs(which I missed before).

Coming to the drf-bulk package, we are only using it for multiple creations, this can be easily achieved by overriding the create method in ModelViewSet as mentioned in the docs(I tested it and works fine, takes around 7-10 lines of code).

So, I would say that we can get rid of drf-bulk(by this, I mean BulkSerailizerMixin and BulkCreateModelMixin which we are using) package and implement bulk features ourselves which is fairly easy.

lemonsaurus · 2020-08-27T13:51:24Z

Okay @RohanJnr - Then I think you should get rid of the bulk plugin.

…mplify UserListSerializer `to_internal_value()` function is not longer overriden in UserListSerializer, this is due to explicitly stating the `id` field in UserSerializer as mentioned in the documentation. Override `create()` method in UserListSerializer and override `get_serializer()` method in `UserViewSet` to support bulk creation.

…creation of User Models.

RohanJnr · 2020-08-30T11:49:21Z

This PR is ready for review.

the Pipfile.lock conflict was resolved by re-locking the pipfile.

RohanJnr · 2020-09-21T17:16:47Z

The bulk update method needs optimization. It calls the database on every instance that needs to be updated.
Currently working on a fix.

…thod The Model.objects.bulk_update() method greatly reduces the number of SQL queries by updating all required instances in 1 query.

RohanJnr · 2020-09-22T12:40:37Z

here are some rough benchmarks of the API changes coming in this PR
Creating 1000 users(bulk).
0.39 seconds

Creating 10000 users(bulk).
3.14 seconds

Updating 1000 users all in 1 SQL query.
0.74 seconds

Updating 10000 users all in 1 SQL query.
14.67 seconds

Updating 10000 users by updating 1000 users in 1 SQL query.
7.12 seconds

Updating 10000 users by updating 2000 users in 1 SQL query.
6.78 seconds

Looks like 2000 users in 1 SQL query is the sweet spot.

MrHemlock

Everything seems to be in order.

MarkKoz

Apologies for so many incremental reviews. I've finally done a comprehensive review and test of the API. Along with my request changes, here are the issues I encountered:

Posting a single user (i.e. no bulk_create) that already exists results in a 500 response because of an IntegrityError. This should be re-raises as 400 error instead.
Posting multiple users which exist with bulk_create works but the response still includes objects for users which were duplicates. It's just a copy of the data given in the request rather than what's in the database.

For example, if you try to set in_guild from false to true for an existing user, the response will return the object with in_guild: true even though it never changed and is still false in the DB.

This isn't too important since we never check the return value, but if you know of an easy fix then you should go for it. In fact, you can respond with nothing since we don't rely on the response. This was profiled not too long ago, and it turns out that serialisation takes a significant amount of time. Granted, it won't have as much of a performance benefit at this scale (IIRC we benchmarked serialisation of all users in a single response).
bulk_patch returns duplicate objects in the response. More specifically, the amount of duplicates it returns depends on the number of fields that were given in the request. Again, not too important, but fix it if you can.
Checking for duplicate IDs in the request doesn't work because filtered_instances = queryset.filter(id__in=object_ids) effectively filters out duplicates before they ever reach the serialiser. This is also indicative of a hole in your tests; there should be a test that catches this issue.

I suppose you could merge the duplicate check with the creation of [item["id"] for item in request.data] if you use a normal loop instead of a comprehension and use the same concept with a set as before.

MarkKoz · 2020-10-08T00:28:00Z

+        """
+        Update multiple User objects in a single request.
+
+        ## Route


This detailed endpoint info should be in the class docstring rather than in the method's docstring.

MarkKoz · 2020-10-08T00:29:48Z

+        ## Route
+        ### PATCH /bot/users/bulk_patch
+        Update all users with the IDs.
+        `id` field is mandatory, rest are optional.


Not quite accurate because it raises a validation error (400) if there are no fields besides the ID. Therefore, the ID and at least one other field are mandatory.

MarkKoz · 2020-10-08T00:31:20Z

    ### POST /bot/users
    Adds a single or multiple new users.
    The roles attached to the user(s) must be roles known by the site.
+    User creation process will be skipped if user is already present in the database.


The way you worded it is unclear in the sense that it may imply the entire operation will abort.

Suggested change

User creation process will be skipped if user is already present in the database.

Users that already exist in the database will be skipped.

MarkKoz · 2020-10-08T00:32:15Z

+    - page_size: Number of Users in one page.
+    - page: Page number


Suggested change

- page_size: Number of Users in one page.

- page: Page number

- page_size: number of Users in one page

- page: page number

It should also be noted these are optional parameters and that the default page size is 10k.

MarkKoz · 2020-10-08T00:32:25Z

    #### Status codes
    - 201: returned on success
    - 400: if one of the given roles does not exist, or one of the given fields is invalid
+    - 400: if multiple user objects with the same id are given.


Suggested change

- 400: if multiple user objects with the same id are given.

- 400: if multiple user objects with the same id are given

MarkKoz · 2020-10-08T00:32:40Z

+        - 200: returned on success.
+        - 400: if the request body was invalid, see response body for details.
+        - 404: if the user with the given id does not exist.


Suggested change

- 200: returned on success.

- 400: if the request body was invalid, see response body for details.

- 404: if the user with the given id does not exist.

- 200: returned on success

- 400: if the request body was invalid, see response body for details

- 404: if the user with the given id does not exist

MarkKoz · 2020-10-08T00:35:34Z

+        except KeyError:
+            # user ID not provided in request body.
+            resp = {
+                "Error": "User ID not provided."


For consistency with the rest of DRF, it should be named "detail".

Suggested change

"Error": "User ID not provided."

"detail": "The id field is missing from at least one object."

Alternatively, you can raise a validation error with

raise ValidationError({"id": ["This field is required."]})

Which may or may not be more idiomatic here. It's in a list cause that's how DRF normally formats such errors. I am leaning more towards the ValidationError approach.

MarkKoz · 2020-10-08T01:04:00Z

+
+        if not fields_to_update:
+            # Raise ValidationError when only id field is given.
+            raise ValidationError({"data": "Insufficient data provided."})


By convention, using "data" as a key name implies that there's something wrong with a field named data. If you need to raise ValidationErrors that aren't specific to a field, use the configured NON_FIELD_ERRORS_KEY as the key for consistency.

Furthermore, maybe the value should be in a list since this seems to be DRF convention. I think the idea is that there could be several errors for the same field/key, hence the list.

MarkKoz · 2020-10-08T01:16:56Z

+                try:
+                    user = instance_mapping[user_data["id"]]
+                except KeyError:
+                    raise NotFound({"id": f"User with id {user_data['id']} not found."})


Only ValidationErrors use the convention of the field name as the key. 404s raised by DRF elsewhere use detail as the key.

Suggested change

raise NotFound({"id": f"User with id {user_data['id']} not found."})

raise NotFound({"detail": f"User with id {user_data['id']} not found."})

I'm adding this comment for posterity: the reason it's okay to raise a 404 here and risk failing the entire request is because the syncer shouldn't ever run into a race condition where it's trying to update a user that has been deleted (or not created yet). The former is impossible because it never deletes users; it only sets in_guild as false. The latter is impossible because if it sees the user doesn't exist in the DB, it will try to create rather than update.

MarkKoz · 2020-10-08T01:48:25Z

+        ...     'id': int,
+        ...     'name': str,
+        ...     'discriminator': int,
+        ...     'roles': List[int],
+        ...     'in_guild': bool
+        ...     },
+        ...     {
+        ...     'id': int,
+        ...     'name': str,
+        ...     'discriminator': int,
+        ...     'roles': List[int],
+        ...     'in_guild': bool


The dict items should be indented.

Suggested change

... 'id': int,

... 'name': str,

... 'discriminator': int,

... 'roles': List[int],

... 'in_guild': bool

... },

... {

... 'id': int,

... 'name': str,

... 'discriminator': int,

... 'roles': List[int],

... 'in_guild': bool

... 'id': int,

... 'name': str,

... 'discriminator': int,

... 'roles': List[int],

... 'in_guild': bool

... },

... {

... 'id': int,

... 'name': str,

... 'discriminator': int,

... 'roles': List[int],

... 'in_guild': bool

…lk patch

…ere duplicates

MarkKoz · 2020-10-08T15:53:08Z

                    user = instance_mapping[user_data["id"]]
                except KeyError:
-                    raise NotFound({"id": f"User with id {user_data['id']} not found."})
+                    raise NotFound({"detail": [f"User with id {user_data['id']} not found."]})


Using a list is a ValidationError convention only

Suggested change

raise NotFound({"detail": [f"User with id {user_data['id']} not found."]})

raise NotFound({"detail": f"User with id {user_data['id']} not found."})

MarkKoz · 2020-10-08T15:57:36Z


-        return User.objects.bulk_create(new_users, ignore_conflicts=True)
+        users = User.objects.bulk_create(new_users, ignore_conflicts=True)
+        return User.objects.filter(id__in=[user.id for user in users])


You misunderstood what I meant by duplicates. I mean that it still returns objects which weren't created because the ID is already in the database. What you're trying to solve is removing duplicate objects in the response, which is impossible.

To fix this, you'd have to know which users are already in the DB before you do a bulk_create. This sounds relatively slow, so it may not be worth fixing (or return nothing) since we don't even read the response.

alright, I will return an empty list then.

Okay, don't forget to document that.

MarkKoz · 2020-10-08T16:10:32Z

+        for user_dict in validated_data:
+            if user_dict["id"] in seen:
+                raise ValidationError(
+                    {"id": f"User with ID {user_dict['id']} given multiple times."}


Suggested change

{"id": f"User with ID {user_dict['id']} given multiple times."}

{"id": [f"User with ID {user_dict['id']} given multiple times."]}

MarkKoz · 2020-10-08T16:15:26Z

+        object_ids = set()
+        for data in request.data:
+            try:
+                if data["id"] in object_ids:
+                    # If request data contains users with same ID.
+                    raise ValidationError(
+                        {"id": [f"User with ID {data['id']} given multiple times."]}
+                    )
+            except KeyError:
+                # If user ID not provided in request body.
+                raise ValidationError(
+                    {"id": ["This field is required."]}
+                )
+            object_ids.add(data["id"])
+
+        filtered_instances = queryset.filter(id__in=object_ids)


Since it's performing validation, I think it makes more sense for this code to be in the serialiser. In the hypothetical case that the serialiser needs to be used elsewhere, this code would have to be duplicated.

MarkKoz · 2020-10-08T16:23:14Z

I also got this warning:

web_1       | /usr/local/lib/python3.7/site-packages/rest_framework/pagination.py:200: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: <class 'pydis_site.apps.api.models.bot.user.User'> QuerySet.
web_1       |   paginator = self.django_paginator_class(queryset, page_size)

…onsistent results with an unordered object_list: <class 'pydis_site.apps.api.models.bot.user.User'> QuerySet.

MarkKoz

Excellent work. Thanks for putting up with my scrutiny.

RohanJnr added 3 commits August 26, 2020 21:04

add pagination for GET request on /bot/users endpoint

4644084

Pagination is done via PageNumberPagination, i.e, each page contains a specific number of `user` objects.

add support for bulk updates on user model

8e636a5

implemented a method to handle bulk updates on user model via a new endpoint: /bot/users/bulk_patch

Add reference links in UserListSerializer methods

792af71

RohanJnr requested a review from a team as a code owner August 26, 2020 17:39

RohanJnr requested review from fiskenslakt and scragly August 26, 2020 17:39

ghost added the needs 2 approvals label Aug 26, 2020

Except AttributeError when self.instance is None and while fetching U…

303cb19

…ser object from database.

RohanJnr added 5 commits August 28, 2020 12:58

Add tests for bulk patch for User Model and additional test for bulk …

2aa12e7

…creation of User Models.

Uninstall djangorestframework-bulk dependency

bfe32de

re-lcok pipfile

b3d4888

remove redundant if clause in update() method in UserListSeriazlier.

982ef75

RohanJnr force-pushed the user_endpoint branch from c1442bb to 982ef75 Compare September 9, 2020 15:46

Merge branch 'master' into user_endpoint

c2fd139

the Pipfile.lock conflict was resolved by re-locking the pipfile.

RohanJnr mentioned this pull request Sep 19, 2020

Smart syncing of users to the site python-discord/bot#1119

Closed

SebastiaanZ assigned lemonsaurus Sep 19, 2020

RohanJnr mentioned this pull request Sep 21, 2020

Smart syncing of Users python-discord/bot#1165

Merged

3 tasks

optimize bulk update endpoint by using Model.objects.bulk_update() me…

48fcd02

…thod The Model.objects.bulk_update() method greatly reduces the number of SQL queries by updating all required instances in 1 query.

MrHemlock self-requested a review September 22, 2020 17:40

MrHemlock approved these changes Sep 23, 2020

View reviewed changes

ghost added the needs 1 approval label Sep 23, 2020

RohanJnr requested a review from MarkKoz October 7, 2020 04:29

update documentation

5388034

MarkKoz requested changes Oct 8, 2020

View reviewed changes

ghost added the s: waiting for author Waiting for author to address a review or respond to a comment label Oct 8, 2020

RohanJnr added 2 commits October 8, 2020 12:24

catch IntegrityError and raise ValidationError during user creation.

995c62b

fix bug: bulk_patch returns duplicate objects in the response

6969333

ghost removed the s: waiting for author Waiting for author to address a review or respond to a comment label Oct 8, 2020

RohanJnr and others added 9 commits October 8, 2020 12:57

add test case: test_returns_400_for_existing_user

a907404

raise ValidationError if users have same ID in request data during bu…

cb32322

…lk patch

add testcase: test_returns_400_for_duplicate_request_users

446db59

update documentation

ffab239

use NON_FIELD_ERRORS_KEY for non-field-specific ValidationError response

fe851bd

normalize API error responses.

cd72f72

correct indentation

e741397

fix bug with bulk create: response includes objects for users which w…

bdc69a7

…ere duplicates

Merge branch 'master' into user_endpoint

ec8a5fc

RohanJnr requested a review from MarkKoz October 8, 2020 09:37

MarkKoz requested changes Oct 8, 2020

View reviewed changes

ghost added the s: waiting for author Waiting for author to address a review or respond to a comment label Oct 8, 2020

RohanJnr added 2 commits October 8, 2020 22:49

return empty list as response for bulk creation of users

570f0cf

supress warning: UnorderedObjectListWarning: Pagination may yield inc…

e405b60

…onsistent results with an unordered object_list: <class 'pydis_site.apps.api.models.bot.user.User'> QuerySet.

ghost removed the s: waiting for author Waiting for author to address a review or respond to a comment label Oct 8, 2020

RohanJnr added 3 commits October 8, 2020 23:22

Convert error message value from list to string

a57035b

Convert ValidationError response value to list from string

4e43549

Move Validation checks to serializer from viewset

1bef883

RohanJnr requested a review from MarkKoz October 8, 2020 18:36

MarkKoz approved these changes Oct 8, 2020

View reviewed changes

MarkKoz merged commit fcfb422 into python-discord:master Oct 8, 2020

RohanJnr deleted the user_endpoint branch October 9, 2020 18:40

	User creation process will be skipped if user is already present in the database.
	Users that already exist in the database will be skipped.

		- page_size: Number of Users in one page.
		- page: Page number

	- 400: if multiple user objects with the same id are given.
	- 400: if multiple user objects with the same id are given

	"Error": "User ID not provided."
	"detail": "The id field is missing from at least one object."

	raise NotFound({"id": f"User with id {user_data['id']} not found."})
	raise NotFound({"detail": f"User with id {user_data['id']} not found."})

	raise NotFound({"detail": [f"User with id {user_data['id']} not found."]})
	raise NotFound({"detail": f"User with id {user_data['id']} not found."})

	{"id": f"User with ID {user_dict['id']} given multiple times."}
	{"id": [f"User with ID {user_dict['id']} given multiple times."]}

Uh oh!

Conversation

RohanJnr commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Pagination

Bulk Update

Additional

Tests for new features.

Uh oh!

ghost commented Aug 26, 2020

Uh oh!

lemonsaurus commented Aug 26, 2020

Uh oh!

lemonsaurus commented Aug 26, 2020

Uh oh!

RohanJnr commented Aug 27, 2020

Uh oh!

lemonsaurus commented Aug 27, 2020

Uh oh!

RohanJnr commented Aug 30, 2020

Uh oh!

RohanJnr commented Sep 21, 2020

Uh oh!

RohanJnr commented Sep 22, 2020

Uh oh!

MrHemlock left a comment

Choose a reason for hiding this comment

Uh oh!

MarkKoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkKoz commented Oct 8, 2020

Uh oh!

MarkKoz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RohanJnr commented Aug 26, 2020 •

edited

Loading