Added support of Oracle #619
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #619 +/- ##
==========================================
- Coverage 86.31% 86.18% -0.13%
==========================================
Files 52 53 +1
Lines 2090 2114 +24
==========================================
+ Hits 1804 1822 +18
- Misses 286 292 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| migrations.RemoveField( | ||
| model_name='response', | ||
| name='request' | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='sqlquery', | ||
| name='request' | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='profile', | ||
| name='request' | ||
| ), |
There was a problem hiding this comment.
Is this valid? I don't see this appearing in silk/models.py.
| @@ -0,0 +1,101 @@ | |||
| # Generated by Django 1.9.7 on 2018-01-10 14:14 | |||
There was a problem hiding this comment.
This is a very old version of Django. Should this file be regenerated using a supported version of django?
There was a problem hiding this comment.
Also, a migration here may require the parent django application to run the migration when upgrading/downgrading django-silk which we should avoid, or if we really need to, should be very clearly messaged in the changelog.
As this migration is currently written, I think upgrading django-silk would require wiping the silk database and downgrading django-silk would not be supported (unless several manual steps are taken to reset the django-silk database which would also involve wiping the database). This PR would need be a breaking change requiring a major version bump.
| @@ -58,7 +57,6 @@ def __init__(self, d): | |||
|
|
|||
|
|
|||
| class Request(models.Model): | |||
| id = CharField(max_length=36, default=uuid4, primary_key=True) | |||
There was a problem hiding this comment.
The default id field for a django model is going to be a BigAutoField which is an integer field. Removing this line would change a Request model from an integer field to a charfield, which would probably break a lot of dependencies or require wiping a lot of data.
| @@ -163,7 +164,15 @@ def __str__(self): | |||
| return '#queries >= %s' % self.value | |||
|
|
|||
| def contribute_to_query_set(self, query_set): | |||
| return query_set.annotate(num_queries=Count('queries')) | |||
| return query_set.annotate( | |||
| # This is overly complex due to Oracle not accepting group by on TextField | |||
There was a problem hiding this comment.
Is contribute_to_query_set used anywhere in the hot path (for silk to ingest data)? If it is, this change may incur a significant performance degradation to host django apps.
Now I would like to discuss dropping the uuid on Request mode, is everybody okay with that?