Skip to content

Commit

Permalink
Changes length of Request.path and Request.view_name fields (#167)
Browse files Browse the repository at this point in the history 
Adds support for `utf8` and `utf8mb4` character sets in MySQL's default
configuration.

Closes #38.
  • Loading branch information
@smaccona @avelis
smaccona authored and avelis committed Mar 14, 2017
1 parent 2e1d5e0 commit d7ebda5
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
4 changes: 2 additions & 2 deletions silk/migrations/0001_initial.py
View file
Expand Up @@ -35,13 +35,13 @@ class Migration(migrations.Migration):
name='Request',
fields=[
('id', models.CharField(max_length=36, primary_key=True, default=uuid.uuid1, serialize=False)),
('path', models.CharField(db_index=True, max_length=300)),
('path', models.CharField(db_index=True, max_length=190)),
('query_params', models.TextField(blank=True, default='')),
('raw_body', models.TextField(blank=True, default='')),
('body', models.TextField(blank=True, default='')),
('method', models.CharField(max_length=10)),
('start_time', models.DateTimeField(db_index=True, default=django.utils.timezone.now)),
('view_name', models.CharField(db_index=True, blank=True, default='', max_length=300, null=True)),
('view_name', models.CharField(db_index=True, blank=True, default='', max_length=190, null=True)),
('end_time', models.DateTimeField(blank=True, null=True)),
('time_taken', models.FloatField(blank=True, null=True)),
('encoded_headers', models.TextField(blank=True, default='')),
Expand Down
4 changes: 2 additions & 2 deletions silk/models.py
View file
Expand Up @@ -61,14 +61,14 @@ def __init__(self):

class Request(models.Model):
id = CharField(max_length=36, default=uuid4, primary_key=True)
path = CharField(max_length=300, db_index=True)
path = CharField(max_length=190, db_index=True)
query_params = TextField(blank=True, default='')
raw_body = TextField(blank=True, default='')
body = TextField(blank=True, default='')
method = CharField(max_length=10)
start_time = DateTimeField(default=timezone.now, db_index=True)
view_name = CharField(
max_length=300, db_index=True, blank=True,
max_length=190, db_index=True, blank=True,
default='', null=True
)
end_time = DateTimeField(null=True, blank=True)
Expand Down

4 comments on commit d7ebda5

@auvipy
Copy link
Contributor

@auvipy auvipy commented on d7ebda5 Mar 14, 2017

Choose a reason for hiding this comment

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

I have a questions in mind, does changing existing migrations in this way good for existing projects? won't they break? what about removing all migrations and regenerate and make a new major release? or this way works fine?

@avelis
Copy link
Collaborator

@avelis avelis commented on d7ebda5 Mar 14, 2017

Choose a reason for hiding this comment

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

@auvipy You are correct. This change will likely break existing installs of this library. Some, but not all. This will go out as a major release. I will make sure release notes to alert people who upgrade to the new release. Usually major releases have an understanding that it contains breaking changes.

@smaccona
Copy link
Collaborator Author

@smaccona smaccona commented on d7ebda5 Mar 14, 2017 •
edited

Choose a reason for hiding this comment

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

@auvipy @avelis Actually I suspect this change will impact few existing users, if any.

Those with fully working existing Silk installations fall into one of the following categories:

  1. Are using PostgreSQL, or are using MySQL with a non-Unicode configuration (most likely). These users have not run into any issues while installing Silk.
  2. Are using MySQL with a Unicode configuration, and have changed their MySQL configuration to allow long indexes (probably second most likely, though not too many users know how to do this). These users also have not run into any issues while installing Silk.
  3. Are using MySQL with a Unicode configuration, and have manually changed these fields in the silk_request table, manually created the required indexes, and finally faked the initial migration to get Silk installed.

In each of these cases, changing the initial migration should have no affect, because Django thinks that it has already applied it and so won't do anything during the next migration. If the user rolls back Silk to the zero state, then all data will be lost and the silk_request table will be created with the new column lengths above, but rolling back to zero will cause this data loss with or without this change anyway.

The other group of users are those using MySQL with partially working installations (i.e. Django created the tables and columns but failed to create the indexes on silk_request because MySQL wouldn't allow it). These users should also be unaffected: their Silk installation will perform poorly both before and after this change because of the missing indexes. Again, Django will make no changes because it thinks that the initial migration is already applied. If Django thinks the initial migration has NOT been applied then Silk already won't be working at all for these users.

I think overall this can only increase adoption by making Silk usable out-of-the-box for MySQL users who use Unicode - at the moment they can't install it at all without manually reconfiguring things at the database level.

@auvipy
Copy link
Contributor

@auvipy auvipy commented on d7ebda5 Mar 14, 2017

Choose a reason for hiding this comment

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

Thanks for your detail explanation. I am convinced. thanks again for following the sensible way

Please sign in to comment.