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

Modify all models + migrations to have `stripe_id` be VARCHAR(191) #350

Merged
merged 4 commits into from Aug 30, 2017

Conversation

Projects
None yet
5 participants
@ahubers
Contributor

ahubers commented Aug 21, 2017

Motivation

The max length for a key field in MySQL is 767 Bytes. A VARCHAR(255) in utf8 does not exceed that limit, however it does with bulkier encodings like utf8mb4. These encodings are becoming more common as they support emojis.

Reducing the stripe_id field to VARCHAR(191) keeps us from hitting that limit.

For some context on the issue I'm having, trying to run pinax-stripe migrations on our django instance is resulting in the following OperationalError:

self = <_mysql.connection open to '192.168.4.23' at 2dbf798>
query = b'CREATE TABLE `pinax_stripe_bitcoinreceiver` (`id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY, `stripe_id` varchar(2...text NOT NULL, `refund_address` longtext NOT NULL, `uncaptured_funds` bool NOT NULL, `used_for_payment` bool NOT NULL)'

    def query(self, query):
        # Since _mysql releases GIL while querying, we need immutable buffer.
        if isinstance(query, bytearray):
            query = bytes(query)
        if self.waiter is not None:
            self.send_query(query)
            self.waiter(self.fileno())
            self.read_query_result()
        else:
>           _mysql.connection.query(self, query)
E           django.db.utils.OperationalError: (1071, 'Specified key was too long; max key length is 767 bytes')

/home/vagrant/getmyboat/lib/python3.4/site-packages/MySQLdb/connections.py:270: OperationalError

Additional Info

The Motivation section above is informed entirely by https://stackoverflow.com/questions/1814532/1071-specified-key-was-too-long-max-key-length-is-767-bytes.

But Alex, you modified a migration? Isn't that bad?

Patching the migration with a new one would still result in the same failure I show above because it will still try to create the tables with the VARCHAR(255) keys. I don't see a problem with modifying these because I don't see stripe IDs ever having a length > 191. So this shouldn't affect any clients' tables.

For those already using pinax-stripe in production environments, they won't notice any difference -- for those trying to get it started in their own environment, they shouldn't run into this problem now...

Modify all models + migrations to have `stripe_id` be VARCHAR(191)
For databases with character encodings with bytes-per-char larger than
utf8, 255 is too large a VARCHAR length for the 1000 byte limit MySQL
imposes on keys. Reducing this to 191 allows the `stripe_id` field to be
a unique key in utf8mb4, a common upgrade to utf8 that has more
bytes-per-char.
@ahubers

This comment has been minimized.

Show comment
Hide comment
@ahubers

ahubers Aug 21, 2017

Contributor

This also Closes #273.

Contributor

ahubers commented Aug 21, 2017

This also Closes #273.

@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Aug 25, 2017

Member

@brosner I'd love to hear your thoughts on this.

Member

paltman commented Aug 25, 2017

@brosner I'd love to hear your thoughts on this.

@tstirrat15

This comment has been minimized.

Show comment
Hide comment
@tstirrat15

tstirrat15 Aug 28, 2017

What's the status on this?

tstirrat15 commented Aug 28, 2017

What's the status on this?

@brosner

This comment has been minimized.

Show comment
Hide comment
@brosner

brosner Aug 29, 2017

Member

@paltman it'd be best for me not to discuss my thoughts on this issue :-)

Database preferences aside, I accept this change. Thank you @ahubers.

Member

brosner commented Aug 29, 2017

@paltman it'd be best for me not to discuss my thoughts on this issue :-)

Database preferences aside, I accept this change. Thank you @ahubers.

@tstirrat15

This comment has been minimized.

Show comment
Hide comment
@tstirrat15

tstirrat15 Aug 29, 2017

@paltman can this now be merged?

tstirrat15 commented Aug 29, 2017

@paltman can this now be merged?

@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Aug 30, 2017

Member

@brosner cool. I'll I'll merge it. Should this be considered BIC? What will happen when people upgrade and there are changes to migrations that are already applied?

Member

paltman commented Aug 30, 2017

@brosner cool. I'll I'll merge it. Should this be considered BIC? What will happen when people upgrade and there are changes to migrations that are already applied?

@paltman paltman merged commit f49cceb into pinax:master Aug 30, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Aug 30, 2017

Member

Thank you for your patience @ahubers as well as your contribution.

Member

paltman commented Aug 30, 2017

Thank you for your patience @ahubers as well as your contribution.

@tstirrat15

This comment has been minimized.

Show comment
Hide comment
@tstirrat15

tstirrat15 Sep 1, 2017

@paltman when can we expect this to be released?

tstirrat15 commented Sep 1, 2017

@paltman when can we expect this to be released?

@gregnewman

This comment has been minimized.

Show comment
Hide comment
@gregnewman

gregnewman Sep 1, 2017

Contributor

@tstirrat15 we are close. I'm finishing up some documentation and I think there's an issue that needs tests before he pushes a new release. Most of the work on the current milestone is done.

Contributor

gregnewman commented Sep 1, 2017

@tstirrat15 we are close. I'm finishing up some documentation and I think there's an issue that needs tests before he pushes a new release. Most of the work on the current milestone is done.

@ahubers

This comment has been minimized.

Show comment
Hide comment
@ahubers

ahubers Sep 1, 2017

Contributor

What will happen when people upgrade and there are changes to migrations that are already applied?

The migration will already exist in their django_migrations table and nothing will happen for those users. If they have build pipelines that rerun migrations from scratch for their test suite (something like... Django), then their test suite will test against a DB created with this new schema. However I don't see that causing issues unless they are inserting unnecessarily long strings into stripe_id fields in their test fixtures.

Contributor

ahubers commented Sep 1, 2017

What will happen when people upgrade and there are changes to migrations that are already applied?

The migration will already exist in their django_migrations table and nothing will happen for those users. If they have build pipelines that rerun migrations from scratch for their test suite (something like... Django), then their test suite will test against a DB created with this new schema. However I don't see that causing issues unless they are inserting unnecessarily long strings into stripe_id fields in their test fixtures.

@paltman paltman added this to the Samwise milestone Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment