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

TagTreeModel not compatible with MySQL #1

Closed
PamelaM opened this Issue Oct 16, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@PamelaM
Contributor

PamelaM commented Oct 16, 2015

See the Django documentation change: django/django@4f83bfa

And the closed ticket: https://code.djangoproject.com/ticket/2495

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Oct 16, 2015

Owner

Thanks for finding that! There are a couple of options, but the main issue this raises for me is that of slug and path length limits.

To fix this issue we could change it to a CharField and make the maximum length of a path 255, or simply remove the unique constraint - nobody should be trying to modify their path directly,

Owner

radiac commented Oct 16, 2015

Thanks for finding that! There are a couple of options, but the main issue this raises for me is that of slug and path length limits.

To fix this issue we could change it to a CharField and make the maximum length of a path 255, or simply remove the unique constraint - nobody should be trying to modify their path directly,

@PamelaM

This comment has been minimized.

Show comment
Hide comment
@PamelaM

PamelaM Oct 16, 2015

Contributor

You're welcome!

For now, I've hand-edited my migration to change the field to a
VARCHAR(250).

I agree that the path field probably doesn't need to be unique, but if it
does, it looks like one could add a "manual constraint" in update_extras()

pam

On Fri, Oct 16, 2015 at 9:44 AM, Richard Terry notifications@github.com
wrote:

Thanks for finding that! There are a couple of options, but the main issue
this raises for me is that of slug and path length limits.

To fix this issue we could change it to a CharField and make the maximum
length of a path 255, or simply remove the unique constraint - nobody
should be trying to modify their path directly,


Reply to this email directly or view it on GitHub
#1 (comment)
.

Pamela McA'Nulty
Chief Architect
Addgene, Inc
75 Sidney Street, Suite 550A
Cambridge, MA 02139
direct: 617-245-2576
office: 617-225-9000
www.addgene.org

Love Addgene? Follow us: Facebook https://www.facebook.com/addgene |
Twitter https://twitter.com/Addgene | LinkedIn
https://www.linkedin.com/company/addgene | Blog http://blog.addgene.org/

Contributor

PamelaM commented Oct 16, 2015

You're welcome!

For now, I've hand-edited my migration to change the field to a
VARCHAR(250).

I agree that the path field probably doesn't need to be unique, but if it
does, it looks like one could add a "manual constraint" in update_extras()

pam

On Fri, Oct 16, 2015 at 9:44 AM, Richard Terry notifications@github.com
wrote:

Thanks for finding that! There are a couple of options, but the main issue
this raises for me is that of slug and path length limits.

To fix this issue we could change it to a CharField and make the maximum
length of a path 255, or simply remove the unique constraint - nobody
should be trying to modify their path directly,


Reply to this email directly or view it on GitHub
#1 (comment)
.

Pamela McA'Nulty
Chief Architect
Addgene, Inc
75 Sidney Street, Suite 550A
Cambridge, MA 02139
direct: 617-245-2576
office: 617-225-9000
www.addgene.org

Love Addgene? Follow us: Facebook https://www.facebook.com/addgene |
Twitter https://twitter.com/Addgene | LinkedIn
https://www.linkedin.com/company/addgene | Blog http://blog.addgene.org/

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Oct 16, 2015

Owner

Sorry, on mobile, hit comment instead of the text box! Orig comment continues:

Nobody should be trying to modify their path directly, and it will be overwitten by save anyway, so its uniqueness will be guaranteed be the unique_together meta constraint.

Which leads me to the issue of maximum slug/path length. Think that's a different issue, so have opened as #2.

Based on my thoughts for #2, I think the best solution would just be to remove uniqueness. You should be fine with your migration fix though, and the upgrade notes will say if you need to do anything special in the future.

This also shows the tests need to run against different database types.

Thanks again!

Owner

radiac commented Oct 16, 2015

Sorry, on mobile, hit comment instead of the text box! Orig comment continues:

Nobody should be trying to modify their path directly, and it will be overwitten by save anyway, so its uniqueness will be guaranteed be the unique_together meta constraint.

Which leads me to the issue of maximum slug/path length. Think that's a different issue, so have opened as #2.

Based on my thoughts for #2, I think the best solution would just be to remove uniqueness. You should be fine with your migration fix though, and the upgrade notes will say if you need to do anything special in the future.

This also shows the tests need to run against different database types.

Thanks again!

@PamelaM

This comment has been minimized.

Show comment
Hide comment
@PamelaM

PamelaM Oct 28, 2015

Contributor

Thanks!
I've found more problems found when MySQL is using utf8mb4 charset :(

All unique fields have a max-length of 191 characters (because MySQL max key length in 767 bytes and utf8mb4 reserves 4 bytes per character, thus 767/4 = 191)

If slug and name could both have configurable max-lengths (which should default to 255 if not defined), that would be great.

oh, and label should probably max out at the same size as 'name'

In our case, our maximum name length right now is 60 chars (with 3 levels in the tree), so 130 more chars should be (famous last words) plenty of headroom.

pam

Contributor

PamelaM commented Oct 28, 2015

Thanks!
I've found more problems found when MySQL is using utf8mb4 charset :(

All unique fields have a max-length of 191 characters (because MySQL max key length in 767 bytes and utf8mb4 reserves 4 bytes per character, thus 767/4 = 191)

If slug and name could both have configurable max-lengths (which should default to 255 if not defined), that would be great.

oh, and label should probably max out at the same size as 'name'

In our case, our maximum name length right now is 60 chars (with 3 levels in the tree), so 130 more chars should be (famous last words) plenty of headroom.

pam

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Oct 29, 2015

Owner

Thanks for the additional info, and for persevering! I'm travelling at the moment, so I'm afraid I haven't made much progress yet. I'm aiming to have this and other issues fixed in the week starting 9th Nov - will update this issue when there's news.

Owner

radiac commented Oct 29, 2015

Thanks for the additional info, and for persevering! I'm travelling at the moment, so I'm afraid I haven't made much progress yet. I'm aiming to have this and other issues fixed in the week starting 9th Nov - will update this issue when there's news.

@PamelaM PamelaM closed this Oct 29, 2015

@PamelaM PamelaM reopened this Oct 29, 2015

@PamelaM

This comment has been minimized.

Show comment
Hide comment
@PamelaM

PamelaM Nov 6, 2015

Contributor

FYI: I'm working on a PR for this - and the other charfield length issues.

Contributor

PamelaM commented Nov 6, 2015

FYI: I'm working on a PR for this - and the other charfield length issues.

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Nov 6, 2015

Owner

Oh great, thanks! I did start adding tests in the 'mysql' branch, but it was too painful to get working using a mobile. Will be back at work next week.

Owner

radiac commented Nov 6, 2015

Oh great, thanks! I did start adding tests in the 'mysql' branch, but it was too painful to get working using a mobile. Will be back at work next week.

@PamelaM

This comment has been minimized.

Show comment
Hide comment
@PamelaM

PamelaM Nov 8, 2015

Contributor

Note that my PR adds the MAX_LENGTH options and changes path from Text to Char field (since path should usually be the same length as name, this seems to be reasonable)

Thus, the MySQL problem is addressed as you suggested by removing the TextField and then by documenting the need for MAX_LENGTH settings dependent on your MySQL character encoding.

Contributor

PamelaM commented Nov 8, 2015

Note that my PR adds the MAX_LENGTH options and changes path from Text to Char field (since path should usually be the same length as name, this seems to be reasonable)

Thus, the MySQL problem is addressed as you suggested by removing the TextField and then by documenting the need for MAX_LENGTH settings dependent on your MySQL character encoding.

@torstenfeld

This comment has been minimized.

Show comment
Hide comment
@torstenfeld

torstenfeld Dec 16, 2015

@radiac any plans yet to merge the PR of @PamelaM ?

torstenfeld commented Dec 16, 2015

@radiac any plans yet to merge the PR of @PamelaM ?

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Dec 17, 2015

Owner

Yes, sorry for the delay - I should have time to work on tagulous properly next week, am hoping to resolve all open issues for the v0.12 release around the end of the week.

Owner

radiac commented Dec 17, 2015

Yes, sorry for the delay - I should have time to work on tagulous properly next week, am hoping to resolve all open issues for the v0.12 release around the end of the week.

@torstenfeld

This comment has been minimized.

Show comment
Hide comment
@torstenfeld

torstenfeld Dec 17, 2015

@radiac perfect, thanks a lot! :) 👍

torstenfeld commented Dec 17, 2015

@radiac perfect, thanks a lot! :) 👍

@radiac radiac closed this in 221c886 Aug 31, 2016

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