-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 RecursionError caused by giving config_from_object
nested mod…
#8619
Conversation
…ule that does not exist (celery#8517)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8619 +/- ##
==========================================
+ Coverage 87.32% 87.33% +0.01%
==========================================
Files 148 148
Lines 18512 18515 +3
Branches 3163 3163
==========================================
+ Hits 16165 16170 +5
+ Misses 2062 2060 -2
Partials 285 285
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Check out my comment
with pytest.raises(ModuleNotFoundError) as exc: | ||
self.app.config_from_object(f'{__name__}.bar') | ||
# the module must exist, but it should not have the config attr | ||
assert self.app.conf.broker_url is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this and the next assertion be outside the with
scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this resolved? @frolenkov-nikita
Thank you very much, I've incorporated your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems better now. waiting for another round of review from Nusnus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, good work!
Fixed RecursionError caused by giving
config_from_object
nested module that does not exist (#8517)Description
The problem is described by new unit test: t/unit/app/test_app.py::test_config_form_object__module_attr_does_not_exist
When you provide a string with valid module name but invalid submodule (attr) to
app.config_from_object
the 3rd party utilkombu.utils.imports.symbol_by_name
would raiseAttributeError
which would be "handled" byPendingConfiguration(UserDict)
and would cause infinite recursion. This happens during the "lazy initialization" of theapp.conf
whenapp.conf
attr is accessed.The fix is to catch
AttributeError
and raiseModuleNotFoundError
which would be propagated correctly and would provide the real error message to the end user.I don't feel very good re: this fix, I am new to this project, so your criticism is highly desired.