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

Fixed #28282 --- Fixed class-based model indexes to always name indexes on non-abstract models. #8613

Merged
merged 1 commit into from Jun 8, 2017

Conversation

Projects
None yet
2 participants
@jdufresne
Member

jdufresne commented Jun 7, 2017

https://code.djangoproject.com/ticket/28282

I also considered:

if not abstract:

As well as removing the if entirely. Why not always name indexes, even on abstract models? Looking for feedback on the intended design and approach here.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jun 7, 2017

Member

I don't recall exactly (3d19d14), but there's a test failure in migrations if the condition is minimized to if not abstract. Did you notice it? In any case, adding a comment explaining this condition would be useful.

Member

timgraham commented Jun 7, 2017

I don't recall exactly (3d19d14), but there's a test failure in migrations if the condition is minimized to if not abstract. Did you notice it? In any case, adding a comment explaining this condition would be useful.

@jdufresne

This comment has been minimized.

Show comment
Hide comment
@jdufresne

jdufresne Jun 8, 2017

Member

I've updated the PR by removing the condition entirely. I don't believe it is necessary as the line new_class._meta.indexes = [copy.deepcopy(idx) for idx in new_class._meta.indexes] should be sufficient to fix bug #27915. As each Index is copied, it will avoid duplicate names on extended model classes. Setting the name will always apply to the copy.

there's a test failure in migrations if the condition is minimized to if not abstract

Jenkins is not reporting an error. Does triggering this error require a different test environment? If so please share how I can replicate.

Adding a comment explaining this condition would be useful

I added a comment to explain the deepcopy().

Member

jdufresne commented Jun 8, 2017

I've updated the PR by removing the condition entirely. I don't believe it is necessary as the line new_class._meta.indexes = [copy.deepcopy(idx) for idx in new_class._meta.indexes] should be sufficient to fix bug #27915. As each Index is copied, it will avoid duplicate names on extended model classes. Setting the name will always apply to the copy.

there's a test failure in migrations if the condition is minimized to if not abstract

Jenkins is not reporting an error. Does triggering this error require a different test environment? If so please share how I can replicate.

Adding a comment explaining this condition would be useful

I added a comment to explain the deepcopy().

@timgraham timgraham merged commit 0c3c37a into django:master Jun 8, 2017

17 checks passed

docs Build #17175 ended
Details
flake8 Build #17293 ended
Details
isort Build #17308 succeeded in 19 sec
Details
pull-requests-javascript Build #13673 ended
Details
pull-requests-trusty/database=mysql,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=mysql,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-trusty/database=mysql_gis,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=mysql_gis,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-trusty/database=postgis,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=postgis,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-trusty/database=postgres,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=postgres,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-trusty/database=spatialite,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=spatialite,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-trusty/database=sqlite3,label=trusty-pr,python=python3.4 Build #12958 ended
Details
pull-requests-trusty/database=sqlite3,label=trusty-pr,python=python3.6 Build #12958 ended
Details
pull-requests-windows/database=sqlite3,label=windows,python=Python35 Build #9268 ended
Details

@jdufresne jdufresne deleted the jdufresne:indexes-named branch Jun 26, 2017

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