Skip to content
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 #26889 -- Fixed missing PostgreSQL index in SchemaEditor.add_field(). #6909

Merged
merged 1 commit into from Jul 14, 2016
Merged

Fixed #26889 -- Fixed missing PostgreSQL index in SchemaEditor.add_field(). #6909

merged 1 commit into from Jul 14, 2016

Conversation

jdufresne
Copy link
Member

@timgraham
Copy link
Member

If we apply the similar reasoning as ticket #25412, we might consider this a data loss issue worthy of backporting to 1.8/1.9. Any thoughts? In that case, I wonder if the schema editor changes might be a bit too invasive for a stable release (concerned about breaking a third-party database backend).

@jdufresne
Copy link
Member Author

jdufresne commented Jul 13, 2016

I have created an alternative version of the patch that is less invasive and only involves the PostgreSQL backend. You can view it by viewing the full diff, not just the last commit. If you prefer the 2nd version I will squash the commits.

I think I like the original better as it offers a single hook to backends to inject indexes on fields (instead of adding the same index in multiple places). But ultimately both work and I am happy with either. I do see your point wrt backporting the change, so whatever makes that work easier is no problem.

Thanks. All feedback welcome.

@timgraham
Copy link
Member

Looks good. We can merge this approach and then follow up with a refactoring on master for the other approach. I'm not sure the SlugField test is needed (or all other subclasses of CharField).

@@ -1908,6 +1908,45 @@ def test_add_textfield_unhashable_default(self):
editor.add_field(Author, new_field)

@unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific")
def test_add_field_with_index_with_charfield(self):
Copy link
Member

@charettes charettes Jul 13, 2016

Choose a reason for hiding this comment

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

test_add_indexed_charfield seems like a more appropriate name and follows the format used in previous test methods.

@jdufresne
Copy link
Member Author

Made requested changes and squashed commits. Thanks.

@timgraham timgraham merged commit 2e4cfcd into django:master Jul 14, 2016
@jdufresne jdufresne deleted the add-field-like-index branch July 15, 2016 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants