Change "object_id" to be a "CharField" #138

jsoa · 2020-05-14T18:09:20Z

The code in this change set migrates the current CrudEvent.object_id from an IntegerField to a CharField as a means to allow various objects with different PK types.

This includes a change to the model_signals that query's for the object instead of looking for the PK, this is because the common UUIDField as a PK usually has a default, thereofre PK would never be empty, even on creation.

This also includes a bug fix for post_delete with the PK not being available in the crud_flow closure. See #138 (comment)

Fixes: #131
Fixes: #90

Tested with Django 3.0
Tested with UUIDField
Tested with BigAutoField
Added Unit tests for UUIDField and BigAutoField
Added Unit tests for deleting objects
Added Test models for UUIDField and BigAutoField
Added test app endpoints for UUID and BigInt
Includes updates to the sqlite DB with data related UUID and BigInt

Deep dive

The main purpose of these tests is to ensure current users of this app, will not need worry about the migration that will change the type of object_id from integer to char. All the output below should be the same per test group, i.e. django version and db. If anything should be different, it will be noted below.

All the test below will do the following

Checkout current master and initialize the test project
Hit test_app/create_obj/ 5 times
Hit test_app/update_obj/?id=N for the 5 items above
print the following to verify the current type of object is int and basic query works

>>> for x in CRUDEvent.objects.all():
...     print(x.content_type, x.object_id, type(x.object_id))
...
test_app | test model 5 <class 'int'>
test_app | test model 4 <class 'int'>
test_app | test model 3 <class 'int'>
test_app | test model 2 <class 'int'>
test_app | test model 1 <class 'int'>
test_app | test model 5 <class 'int'>
test_app | test model 4 <class 'int'>
test_app | test model 3 <class 'int'>
test_app | test model 2 <class 'int'>
test_app | test model 1 <class 'int'>
 >>> CRUDEvent.objects.filter(object_id=int(1))
<QuerySet [<CRUDEvent: CRUDEvent object (6)>, <CRUDEvent: CRUDEvent object (1)>]>

checkout the new branch and run migrations
print the same output as above, ensuring the new type is str and the query still works and returns the same results, by passing it str and int versions.

>>> for x in CRUDEvent.objects.all():
...     print(x.content_type, x.object_id, type(x.object_id))
...
test_app | test model 5 <class 'str'>
test_app | test model 4 <class 'str'>
test_app | test model 3 <class 'str'>
test_app | test model 2 <class 'str'>
test_app | test model 1 <class 'str'>
test_app | test model 5 <class 'str'>
test_app | test model 4 <class 'str'>
test_app | test model 3 <class 'str'>
test_app | test model 2 <class 'str'>
test_app | test model 1 <class 'str'>
>>> CRUDEvent.objects.filter(object_id='1')
<QuerySet [<CRUDEvent: CRUDEvent object (6)>, <CRUDEvent: CRUDEvent object (1)>]>
>>> CRUDEvent.objects.filter(object_id=int(1))
<QuerySet [<CRUDEvent: CRUDEvent object (6)>, <CRUDEvent: CRUDEvent object (1)>]>

Current progress

	Django Version	DB	DB Version
[x]	3.0	Postgres	12.0
[x]	3.0	Postgres	11.0
[x]	3.0	Postgres	10.0
[x]	3.0	Postgres	9
[x]	2.2	Postgres	12.0
[x]	2.2	Postgres	11.0
[x]	2.2	Postgres	10.0
[x]	2.2	Postgres	9.0
[x]*	1.11	Postgres	12.0
[x]*	1.11	Postgres	11.0
[x]*	1.11	Postgres	10.0
[x]*	1.11	Postgres	9.0
[x]	3.0	MySQL	8
[x]	3.0	MySQL	5
[x]	2.2	MySQL	8
[x]	2.2	MySQL	5
[x]*	1.11	MySQL	8
[x]*	1.11	MySQL	5

Notes

Django 1.11 created more CRUDEvent objects (on master branch)

>>> for x in CRUDEvent.objects.all():
...     print(x.content_type, x.object_id, type(x.object_id))
...
test model 5 <class 'int'>
test model 4 <class 'int'>
test model 3 <class 'int'>
test model 2 <class 'int'>
test model 1 <class 'int'>
test model 5 <class 'int'>
test model 4 <class 'int'>
test model 3 <class 'int'>
test model 2 <class 'int'>
test model 1 <class 'int'>
content type 12 <class 'int'>
content type 11 <class 'int'>
content type 10 <class 'int'>
content type 9 <class 'int'>
content type 8 <class 'int'>
content type 7 <class 'int'>
content type 6 <class 'int'>
content type 5 <class 'int'>
content type 4 <class 'int'>
content type 3 <class 'int'>
content type 1 <class 'int'>

After migrating

content type 18 <class 'str'>
content type 17 <class 'str'>
content type 16 <class 'str'>
content type 15 <class 'str'>
content type 14 <class 'str'>
content type 13 <class 'str'>
test model 5 <class 'str'>
test model 4 <class 'str'>
test model 3 <class 'str'>
test model 2 <class 'str'>
test model 1 <class 'str'>
test model 5 <class 'str'>
test model 4 <class 'str'>
test model 3 <class 'str'>
test model 2 <class 'str'>
test model 1 <class 'str'>
content type 12 <class 'str'>
content type 11 <class 'str'>
content type 10 <class 'str'>
content type 9 <class 'str'>
content type 8 <class 'str'>
content type 7 <class 'str'>
content type 6 <class 'str'>
content type 5 <class 'str'>
content type 4 <class 'str'>
content type 3 <class 'str'>
content type 1 <class 'str'>

steverecio

Looks great! Just needs one small fix (change id to pk)

steverecio · 2020-05-17T16:47:44Z

easyaudit/signals/model_signals.py

-            else:
+            # Query the object, see if it exists
+            try:
+                old_model = sender.objects.get(id=instance.pk)


Please change this to pk=instance.pk

I have primary keys not named id so this throws an exception

@steverecio updated

steverecio

LGTM - I have this code deployed on my project and it seems to work as intended.

steverecio · 2020-05-17T22:19:11Z

One note after testing this a bit more. The object_id should have null=True to handle Delete events. I'm seeing the following error after deleting objects:

easy audit had a post_delete exception on CRUDEvent creation
...
IntegrityError: null value in column "object_id" violates not-null constraint

jsoa · 2020-05-18T00:00:52Z

@steverecio I was seeing this as well with postgres, doesn't seem to be an issue with sqlite.

The instance id is stripped before the transaction completes with latest postgres. Simple fix i think.

jsoa · 2020-05-18T00:12:01Z

Last commit should address the issue without needing to make the field nullable, here is a screenshot of a project ive been testing with..

steverecio · 2020-05-18T01:13:33Z

@jsoa nice fix! I like your approach much better.

@jheld @soynatan can we get this merged in?

easyaudit/signals/model_signals.py

- Uses the internal `_state` object

jsoa · 2020-05-28T19:42:54Z

@Etenil @jheld Updated code in order to not do the explicit query in the pre_save signal. Instead it will use the internal _state object to determine if the object is being added. Took a look and _state has been around since django 1.2

jheld · 2020-05-28T21:52:29Z

Oh that's a nice idea.

We use the _state a little in my project (I don't love it -- I wish django would make the contract exposed/official). I think so long as our test case is written such that if that goes away/changes, then it'll fail and then we'll be aware before release.

I/we need to first fix/address the breakage on the request event user signal before merging anything else. Then we can add a couple more things (nothing which breaks compatibility/super risky).

This seems like a good candidate!

jsoa · 2020-05-29T01:33:37Z

@jheld I also don't love checking _state, however I did add a compatibility test to ensure its available.

jheld · 2020-05-29T03:02:44Z

Excellent work with all of the test cases and test models. Comprehensive and pretty modular/DRY.

This looks like a great candidate for 1.3.0. I hope to finalize 1.2.3 this or next week.

jheld · 2020-06-04T03:42:16Z

Likely will release 1.2.3 this week/early next week. There was a bug introduced in the request event handler at the beginning of the alphas and the last/current alpha (4) effectively rolls back that change.

jheld · 2020-07-01T01:36:06Z

About ready to merge this!

@jsoa

So TestModel is still implicitly using AutoField (e.g. regular INT), right? And that is the "base" test case model so it is tested, and that's good.

Can you take a look over the comments/reviews once more to make sure all of those are answered/moot?

jsoa · 2020-07-02T17:19:38Z

@jheld That is correct TestModel is the basic django model with AutoField (integer PK)

I took a look again and everything seems good to go. The only thing that i'm not 100% certain of is if existing users of this app will have to do anything special after the schema migration is run, such as converting the integers to chars.

jheld

Can you remove the sqlite test db file?

jheld · 2020-07-11T16:58:49Z

@jsoa if I'm understanding your question, so long as they run migrations, they shouldn't have to do anything other than that. I'm happy to test on my local, too, next week. My project runs on the default configuration.

pcraig3 · 2020-07-17T23:28:06Z

Hello! Really excited to see this PR go in.

I'm working on a Django project where we're using UUIDs as primary keys for our users and this PR resolves the issues I'm seeing. It seems like it's nearly there, so hope this can be merged soon! 👍

jsoa · 2020-07-21T13:28:18Z

@jheld started testing the migration (manually). See PR description for details, still work in progress. we can add more DB versions and/or django versions if needed.

jheld · 2020-07-21T13:47:23Z

@jsoa this looks really great. Thank you for running through some simple (but time consuming!) test cases. My project uses MySQL but if you think you'll have time to run through that on your end I'll wait til then. Let me know -- happy to follow the instructions you've laid out.

jsoa · 2020-07-21T15:47:07Z

@jheld done with my testing, everything seems good to go, with the minor note about django 1.11. It would be nice to get someone to run through just some of the manual tests that was run.

Here is a docker-compose file i used for bootstrapping the DBs

version: "3"

services:

  # POSTGRES
  ea-psql12:
    image: postgres:12.0

    environment:
      - POSTGRES_PASSWORD=ea
      - POSTGRES_USER=ea
      - POSTGRES_DB=ea

    ports:
      - "54320:5432"

  ea-psql11:
    image: postgres:11.0

    environment:
      - POSTGRES_PASSWORD=ea
      - POSTGRES_USER=ea
      - POSTGRES_DB=ea

    ports:
      - "54321:5432"


  ea-psql10:
    image: postgres:10.0

    environment:
      - POSTGRES_PASSWORD=ea
      - POSTGRES_USER=ea
      - POSTGRES_DB=ea

    ports:
      - "54322:5432"

  ea-psql9:
    image: postgres:9

    environment:
      - POSTGRES_PASSWORD=ea
      - POSTGRES_USER=ea
      - POSTGRES_DB=ea

    ports:
      - "54323:5432"


  # MYSQL
  ea-mysql8:
    image: mysql:8

    environment:
      - MYSQL_ROOT_PASSWORD=ea
      - MYSQL_DATABASE=ea
      - MYSQL_USER=ea
      - MYSQL_PASSWORD=ea

    ports:
      - "33060:3306"

  ea-mysql5:
    image: mysql:5

    environment:
      - MYSQL_ROOT_PASSWORD=ea
      - MYSQL_DATABASE=ea
      - MYSQL_USER=ea
      - MYSQL_PASSWORD=ea

    ports:
      - "33061:3306"

Each DB has a different host port

PG
- 54320 - 12
- 54321 - 11
- 54322 - 10
- 54323 - 9
MYSQL
- 33060 - 8
- 33061 - 5

After each django version check, we need to reset the containers, by docker rm <container id>
Then docker volume rm $(docker volume ls --quiet --filter dangling=true) || true (removes any volume not attached to a container)
Then its just manually changed the DB connection in the test_project per DB version and Django version.

    # 'default': {
    #     'ENGINE': 'django.db.backends.postgresql',
    #     'NAME': 'ea',
    #     'USER': 'ea',
    #     'PASSWORD': 'ea',
    #     'HOST': 'localhost',
    #     'PORT': '54323'
    # }

    'default': {
        'ENGINE': 'django.db.backends.mysql',
        'NAME': 'ea',
        'USER': 'ea',
        'PASSWORD': 'ea',
        'HOST': '127.0.0.1',
        'PORT': '33061'
    }

jheld · 2020-07-21T16:58:58Z

@jsoa excellent use for docker-compose.

We don't support django 1.11 officially anymore.

2.0+ (and realistically other than travis tests passing, we really only need to be 2.2+).

So while I agree 1.11 is odd, I will argue we should ignore the difference. I have no idea what has changed/subtly broken in our usage.

[-] CrudEvent object_id as a CharField

66b6219

steverecio suggested changes May 17, 2020

View reviewed changes

[-] Use pk instead of id in pre_save object lookup

584a9b0

steverecio approved these changes May 17, 2020

View reviewed changes

[-] Fixes issue w/ post_delete / Adds delete test

cadc9dc

Etenil reviewed May 18, 2020

View reviewed changes

easyaudit/signals/model_signals.py Show resolved Hide resolved

jsoa added 3 commits May 18, 2020 09:02

[-] Adds updates from soynatan#141 PR

e97a2ad

Merge branch 'master' into crud_event_object_id_charfield

b6446a1

[-] Removes the need for an explicit query

8ddce19

- Uses the internal `_state` object

[-] Adds _state compat test

123e79a

jheld requested changes Jul 11, 2020

View reviewed changes

[-] Removes sqlite3 test db

523f0a1

jheld approved these changes Jul 22, 2020

View reviewed changes

jheld merged commit fd05524 into soynatan:master Jul 22, 2020

jheld mentioned this pull request Aug 7, 2020

Add: BigIntegerField and UUIDField -based CRUDEvents #103

Closed

jkarstens mentioned this pull request Aug 25, 2020

created or updated check in pre_save doesn't consider models with a pre-assigned pk #161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change "object_id" to be a "CharField" #138

Change "object_id" to be a "CharField" #138

jsoa commented May 14, 2020 •

edited

Loading

steverecio left a comment

steverecio May 17, 2020

jsoa May 17, 2020

steverecio left a comment

steverecio commented May 17, 2020

jsoa commented May 18, 2020

jsoa commented May 18, 2020

steverecio commented May 18, 2020

jsoa commented May 28, 2020

jheld commented May 28, 2020

jsoa commented May 29, 2020

jheld commented May 29, 2020

jheld commented Jun 4, 2020

jheld commented Jul 1, 2020

jsoa commented Jul 2, 2020

jheld left a comment

jheld commented Jul 11, 2020

pcraig3 commented Jul 17, 2020

jsoa commented Jul 21, 2020

jheld commented Jul 21, 2020

jsoa commented Jul 21, 2020

jheld commented Jul 21, 2020

Change "object_id" to be a "CharField" #138

Change "object_id" to be a "CharField" #138

Conversation

jsoa commented May 14, 2020 • edited Loading

Deep dive

Current progress

Notes

steverecio left a comment

Choose a reason for hiding this comment

steverecio May 17, 2020

Choose a reason for hiding this comment

jsoa May 17, 2020

Choose a reason for hiding this comment

steverecio left a comment

Choose a reason for hiding this comment

steverecio commented May 17, 2020

jsoa commented May 18, 2020

jsoa commented May 18, 2020

steverecio commented May 18, 2020

jsoa commented May 28, 2020

jheld commented May 28, 2020

jsoa commented May 29, 2020

jheld commented May 29, 2020

jheld commented Jun 4, 2020

jheld commented Jul 1, 2020

jsoa commented Jul 2, 2020

jheld left a comment

Choose a reason for hiding this comment

jheld commented Jul 11, 2020

pcraig3 commented Jul 17, 2020

jsoa commented Jul 21, 2020

jheld commented Jul 21, 2020

jsoa commented Jul 21, 2020

jheld commented Jul 21, 2020

jsoa commented May 14, 2020 •

edited

Loading