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 #30463 -- Fixed crash of deprecation message when Meta.ordering contains expressions. #11377

Merged
merged 1 commit into from May 18, 2019

Conversation

ruchit2801
Copy link
Contributor

This solves the error raised when self._meta_ordering encountered OrderBy instances in .join() function. I've used regex function and python replace method to convert the string such that quotation marks are at the desired place and, the string remains in the form of real source code compatible with order_by().

I'd appreciate any cleaner solution than this one and would be happy to improve the code.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@ruchit2801 Thanks for this patch 👍 . Please add release notes (2.2.2) and a regression test (tests/ordering). Changing ordering for Article model e.g.

ordering = (                                                                                                                                  
    '-pub_date',                                                                                                                              
    'headline',                                                                                                                               
    models.F('author__name').asc(),                                                                                                           
    OrderBy(models.F('second_author__name')),                                                                                                 )

and fixing ordering.tests.OrderingTests.test_deprecated_values_annotate should be enough.

django/db/models/sql/compiler.py Outdated Show resolved Hide resolved
@felixxm felixxm self-assigned this May 17, 2019
@felixxm felixxm changed the title [WIP]Fixed #30463 -- Stringify OrderBy instances in Deprecation warning [WIP]Fixed #30463 -- Fixed crash of Meta.ordering deprecation message when it contains expressions. May 17, 2019
@ruchit2801
Copy link
Contributor Author

ruchit2801 commented May 17, 2019

@felixxm I've made relevant changes. Can you give me a little hint, on how can I reproduce this error?

File "django/django/db/models/sql/compiler.py", line 558, in as_sql
"', '".join(self._meta_ordering)
TypeError: sequence item 0: expected str instance, OrderBy found

Thanks for the help :)

@felixxm
Copy link
Member

felixxm commented May 17, 2019

@ruchit2801 I described this in a previous comment, it is also in ticket comments.

You can change ordering for the Article model in tests/ordering/models.py, e.g.

ordering = (                                                                                                                                  
    '-pub_date',                                                                                                                              
    'headline',                                                                                                                               
    models.F('author__name').asc(),                                                                                                           
    OrderBy(models.F('second_author__name')),
)

after that you will get an error when running ./runtests.py ordering.tests.

@ruchit2801 ruchit2801 force-pushed the t#30463 branch 3 times, most recently from 5504590 to a337f19 Compare May 17, 2019 16:29
@ruchit2801
Copy link
Contributor Author

ruchit2801 commented May 17, 2019

@felixxm Changes pushed :). The failed check seems due to some unknown module memcached, which is I think independent of changes in PR. How should I resolve this failed check? Ready for review

@felixxm felixxm changed the title [WIP]Fixed #30463 -- Fixed crash of Meta.ordering deprecation message when it contains expressions. Fixed #30463 -- Fixed crash of Meta.ordering deprecation message when it contains expressions. May 18, 2019
@felixxm felixxm changed the title Fixed #30463 -- Fixed crash of Meta.ordering deprecation message when it contains expressions. Fixed #30463 -- Fixed crash of deprecation message when Meta.ordering contains expressions. May 18, 2019
@felixxm felixxm merged commit 04042b2 into django:master May 18, 2019
@felixxm
Copy link
Member

felixxm commented May 18, 2019

@ruchit2801 Thanks again 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants