Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BigAutoField for User.id #826

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shunghsiyu
Copy link
Member

Types of changes

  • Bugfix (I'm not entirely sure)
  • New feature
  • Refactoring
  • Breaking change (any change that would cause existing functionality to not work as expected)
  • Documentation Update
  • Other (please describe)

Description

We have been using the bigint data type for User.id since ab2573d, but the id field that Django automatically create for us uses AutoField, which maps to the int data type.
This mismatch in data type is mostly benign, but when User.id grow large enough, it will blowup when we try to use setval on User.id (e.g. importing fixture with manage.py loaddata), with an error complaining value out of range.

Steps to Test This Pull Request

Steps to reproduce the behavior:

  1. Start the development environment
  2. Import this fixture with python manage.py loaddata fixtures.json

Expected behavior

Importing fixture data succeed without error

Related Issue

None that I know of.

More Information

We have been using the bigint data type for User.id since ab2573d, but the
id field that Django automatically create for us uses AutoField, which maps to
the int data type.
This mismatch in data type is mostly benign, but when User.id grow large
enough, it will blowup when we try to use setval on User.id (e.g. importing
fixture with `manage.py loaddata`), with an error complaining value out of
range.
@shunghsiyu shunghsiyu requested a review from uranusjr July 12, 2020 09:49
@shunghsiyu
Copy link
Member Author

Question for reviewers: can we omit src/users/migrations/0012_auto_20200712_1632.py?

@shunghsiyu shunghsiyu marked this pull request as ready for review July 12, 2020 09:52
@codecov-commenter
Copy link

Codecov Report

Merging #826 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #826      +/-   ##
==========================================
+ Coverage   76.14%   76.33%   +0.19%     
==========================================
  Files          70       70              
  Lines        2687     2688       +1     
==========================================
+ Hits         2046     2052       +6     
+ Misses        641      636       -5     
Impacted Files Coverage Δ
src/users/models.py 90.47% <100.00%> (+0.09%) ⬆️
src/core/models.py 96.42% <0.00%> (+1.78%) ⬆️
src/events/views.py 65.94% <0.00%> (+2.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3f2e02...8615a9e. Read the comment docs.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the migration work locally? Not sure if we need to do --dry-run here. It should be good to go if this succeeds locally.

@shunghsiyu
Copy link
Member Author

Does the migration work locally? Not sure if we need to do --dry-run here. It should be good to go if this succeeds locally.

I remember it worked locally for me, but I'll check again (probably tomorrow) just to be sure.

@shunghsiyu
Copy link
Member Author

shunghsiyu commented Jul 26, 2020

Migration works locally, and after the migration user creation still works.

However, there's one change in the schema that might need to be reviewed (the diff is below); user.id is changed from DEFAULT public.users_user_id_generator() NOT NULL to DEFAULT nextval('public.users_user_id_seq'::regclass) NOT NULL, which is probably not what we want.

@uranusjr what do you think we should do here?

Maybe we can add a default to the id field, which either:

  1. Is a function that call the PostgreSQL functionusers_user_id_generator() underneath
  2. Is a function that re-implements most of the PostgreSQL functionusers_user_id_generator() functionality in Python

Reference:
How to define a Django model field with a PostgreSQL function as default value

--- before.sql	2020-07-26 04:19:21.733974888 +0000
+++ after.sql	2020-07-26 04:20:03.486554286 +0000
@@ -1080,11 +1080,11 @@
 --
 -- Name: users_user; Type: TABLE; Schema: public; Owner: postgres
 --
 
 CREATE TABLE public.users_user (
-    id bigint DEFAULT public.users_user_id_generator() NOT NULL,
+    id bigint NOT NULL,
     password character varying(128) NOT NULL,
     last_login timestamp with time zone,
     is_superuser boolean NOT NULL,
     email character varying(255) NOT NULL,
     speaker_name character varying(100) NOT NULL,
@@ -1140,11 +1140,10 @@
 --
 -- Name: users_user_id_seq; Type: SEQUENCE; Schema: public; Owner: postgres
 --
 
 CREATE SEQUENCE public.users_user_id_seq
-    AS integer
     START WITH 1
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
     CACHE 1;
@@ -1326,10 +1325,17 @@
 
 ALTER TABLE ONLY public.users_cocrecord ALTER COLUMN id SET DEFAULT nextval('public.users_cocrecord_id_seq'::regclass);
 
 
 --
+-- Name: users_user id; Type: DEFAULT; Schema: public; Owner: postgres
+--
+
+ALTER TABLE ONLY public.users_user ALTER COLUMN id SET DEFAULT nextval('public.users_user_id_seq'::regclass);
+
+
+--
 -- Name: users_user_groups id; Type: DEFAULT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.users_user_groups ALTER COLUMN id SET DEFAULT nextval('public.users_user_groups_id_seq'::regclass);
 
@@ -2251,15 +2257,15 @@
 ALTER TABLE ONLY public.django_admin_log
     ADD CONSTRAINT django_admin_log_content_type_id_c4bce8eb_fk_django_co FOREIGN KEY (content_type_id) REFERENCES public.django_content_type(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: django_admin_log django_admin_log_user_id_c564eba6_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: django_admin_log django_admin_log_user_id_c564eba6_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.django_admin_log
-    ADD CONSTRAINT django_admin_log_user_id_c564eba6_fk_users_user_id FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT django_admin_log_user_id_c564eba6_fk FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
 -- Name: events_customevent events_customevent_begin_time_id_ec21a279_fk_events_time_value; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2355,15 +2361,15 @@
 ALTER TABLE ONLY public.events_sponsoredevent
     ADD CONSTRAINT events_sponsoredevent_end_time_id_3fa7d8a4_fk_events_time_value FOREIGN KEY (end_time_id) REFERENCES public.events_time(value) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: events_sponsoredevent events_sponsoredevent_host_id_ffa9bf06_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: events_sponsoredevent events_sponsoredevent_host_id_ffa9bf06_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.events_sponsoredevent
-    ADD CONSTRAINT events_sponsoredevent_host_id_ffa9bf06_fk_users_user_id FOREIGN KEY (host_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT events_sponsoredevent_host_id_ffa9bf06_fk FOREIGN KEY (host_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
 -- Name: proposals_additionalspeaker proposals_additional_proposal_type_id_cd3705a0_fk_django_co; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2371,31 +2377,31 @@
 ALTER TABLE ONLY public.proposals_additionalspeaker
     ADD CONSTRAINT proposals_additional_proposal_type_id_cd3705a0_fk_django_co FOREIGN KEY (proposal_type_id) REFERENCES public.django_content_type(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: proposals_additionalspeaker proposals_additionalspeaker_user_id_d1283d1e_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: proposals_additionalspeaker proposals_additionalspeaker_user_id_d1283d1e_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.proposals_additionalspeaker
-    ADD CONSTRAINT proposals_additionalspeaker_user_id_d1283d1e_fk_users_user_id FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT proposals_additionalspeaker_user_id_d1283d1e_fk FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: proposals_talkproposal proposals_talkproposal_submitter_id_3e6b8833_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: proposals_talkproposal proposals_talkproposal_submitter_id_3e6b8833_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.proposals_talkproposal
-    ADD CONSTRAINT proposals_talkproposal_submitter_id_3e6b8833_fk_users_user_id FOREIGN KEY (submitter_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT proposals_talkproposal_submitter_id_3e6b8833_fk FOREIGN KEY (submitter_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: proposals_tutorialproposal proposals_tutorialpr_submitter_id_4cb0d562_fk_users_use; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: proposals_tutorialproposal proposals_tutorialproposal_submitter_id_4cb0d562_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.proposals_tutorialproposal
-    ADD CONSTRAINT proposals_tutorialpr_submitter_id_4cb0d562_fk_users_use FOREIGN KEY (submitter_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT proposals_tutorialproposal_submitter_id_4cb0d562_fk FOREIGN KEY (submitter_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
 -- Name: reviews_review reviews_review_proposal_id_56d8777c_fk_proposals; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2403,15 +2409,15 @@
 ALTER TABLE ONLY public.reviews_review
     ADD CONSTRAINT reviews_review_proposal_id_56d8777c_fk_proposals FOREIGN KEY (proposal_id) REFERENCES public.proposals_talkproposal(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: reviews_review reviews_review_reviewer_id_29fc8a19_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: reviews_review reviews_review_reviewer_id_29fc8a19_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.reviews_review
-    ADD CONSTRAINT reviews_review_reviewer_id_29fc8a19_fk_users_user_id FOREIGN KEY (reviewer_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT reviews_review_reviewer_id_29fc8a19_fk FOREIGN KEY (reviewer_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
 -- Name: reviews_talkproposalsnapshot reviews_talkproposal_proposal_id_e6e0fa3a_fk_proposals; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2419,15 +2425,15 @@
 ALTER TABLE ONLY public.reviews_talkproposalsnapshot
     ADD CONSTRAINT reviews_talkproposal_proposal_id_e6e0fa3a_fk_proposals FOREIGN KEY (proposal_id) REFERENCES public.proposals_talkproposal(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: users_cocrecord users_cocrecord_user_id_9c56481d_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+-- Name: users_cocrecord users_cocrecord_user_id_9c56481d_fk; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.users_cocrecord
-    ADD CONSTRAINT users_cocrecord_user_id_9c56481d_fk_users_user_id FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
+    ADD CONSTRAINT users_cocrecord_user_id_9c56481d_fk FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
 -- Name: users_user_groups users_user_groups_group_id_9afc8d0e_fk_auth_group_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2435,32 +2441,16 @@
 ALTER TABLE ONLY public.users_user_groups
     ADD CONSTRAINT users_user_groups_group_id_9afc8d0e_fk_auth_group_id FOREIGN KEY (group_id) REFERENCES public.auth_group(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: users_user_groups users_user_groups_user_id_5f6f5a90_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
---
-
-ALTER TABLE ONLY public.users_user_groups
-    ADD CONSTRAINT users_user_groups_user_id_5f6f5a90_fk_users_user_id FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
-
-
---
 -- Name: users_user_user_permissions users_user_user_perm_permission_id_0b93982e_fk_auth_perm; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
 
 ALTER TABLE ONLY public.users_user_user_permissions
     ADD CONSTRAINT users_user_user_perm_permission_id_0b93982e_fk_auth_perm FOREIGN KEY (permission_id) REFERENCES public.auth_permission(id) DEFERRABLE INITIALLY DEFERRED;
 
 
 --
--- Name: users_user_user_permissions users_user_user_permissions_user_id_20aca447_fk_users_user_id; Type: FK CONSTRAINT; Schema: public; Owner: postgres
---
-
-ALTER TABLE ONLY public.users_user_user_permissions
-    ADD CONSTRAINT users_user_user_permissions_user_id_20aca447_fk_users_user_id FOREIGN KEY (user_id) REFERENCES public.users_user(id) DEFERRABLE INITIALLY DEFERRED;
-
-
---
 -- PostgreSQL database dump complete
 --

@uranusjr
Copy link
Member

Or we can subclass BigAutoField and implement the migration ourselves to fix the generated SQL declaration. This may be easier.

@shunghsiyu
Copy link
Member Author

I guess another option is to add a migrations.RunSQL operation in src/users/migrations/0012_auto_20200712_1632.py that change the default for user.id back.

But I'd like to avoid this option if possible, and rather have things declared in our Django model.

@uranusjr
Copy link
Member

This could work (not tested)

from psycopg2.extensions import AsIs 

id = models.BigAutoField(primary_field=True, default=AsIs('public.users_user_id_generator()'))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants