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

[IMP] Provide useful tracebacks on exceptions while validating #28612

Closed
wants to merge 1 commit into
base: 10.0
from

Conversation

Projects
None yet
5 participants
@Yajo
Copy link
Contributor

Yajo commented Nov 13, 2018

Before this patch, any exception on a constraint that were not a ValidationError lacked proper traceback information. All the logs said was that this line failed, but this line was never the source of the problem itself. Since Odoo v10 uses python 2, the implicit exception chaining is not available.

After this patch, the exception itself will still lack that information, but at least Odoo logs will print the good traceback.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
@Tecnativa

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Nov 16, 2018

@odony is this something acceptable for you?

Show resolved Hide resolved odoo/models.py Outdated
@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Feb 13, 2019

@Yajo please fix the CLA question.

@Yajo Yajo force-pushed the Tecnativa:10.0-helpful-logs branch Feb 13, 2019

@Yajo

This comment has been minimized.

Copy link
Contributor Author

Yajo commented Feb 20, 2019

@odony Merge please? 🙏

[FIX] core: log unexpected validation exceptions
Before this patch, any exception raised by a constraint method that
were not of type `ValidationError` were hard to debug, because the
origin line was never logged.

Explicitly logging the error (with traceback) when we catch it
ensures proper contextual info, even in the absence of exception
chaining.

@odony odony force-pushed the Tecnativa:10.0-helpful-logs branch to 34a1c67 Feb 20, 2019

@robodoo robodoo removed the CI 🤖 label Feb 20, 2019

@odony

This comment has been minimized.

Copy link
Contributor

odony commented Feb 20, 2019

@Yajo sure, just pushed a minor update of the commit message :)
@robodoo r+

@robodoo robodoo added the r+ 👌 label Feb 20, 2019

robodoo pushed a commit that referenced this pull request Feb 20, 2019

[FIX] core: log unexpected validation exceptions
Before this patch, any exception raised by a constraint method that
were not of type `ValidationError` were hard to debug, because the
origin line was never logged.

Explicitly logging the error (with traceback) when we catch it
ensures proper contextual info, even in the absence of exception
chaining.

closes #28612
@Yajo

This comment has been minimized.

Copy link
Contributor Author

Yajo commented Feb 20, 2019

Just let me remind you that, AFAIK, for v11+ this shouldn't be needed because reraised exceptions retain original traceback in python 3.

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 20, 2019

Staging failed: ci/runbot on 9434fde196a91bb09d09fa908fc288d54abb2145 (view more at http://runbot.odoo.com/runbot/build/452867)

@odony

This comment has been minimized.

Copy link
Contributor

odony commented Feb 20, 2019

sigh.. v10 phantomjs timeouts
@robodoo retry

@Yajo right it could be omitted in P3, though practically speaking it won't hurt to have an extra log for this unlikely event.

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 20, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 20, 2019

@Yajo Yajo deleted the Tecnativa:10.0-helpful-logs branch Mar 8, 2019

@yelizariev

This comment has been minimized.

Copy link
Contributor

yelizariev commented Mar 11, 2019

This update breaks our CI for some hacky code 😬

Travis logs: https://travis-ci.com/it-projects-llc/misc-addons/jobs/177981438
Code: https://github.com/it-projects-llc/misc-addons/blob/d400882/web_debranding/models/ir_ui_view.py#L74-L85

In short, the code is trying to create a view, that works only in enterprise but not in community or vice-versa.

Not sure what I can do, but I agree with the problem that this update fixes.

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Mar 11, 2019

This has been reverted in 11.0+, but for v10 it still remains as it can be useful. As Oliver said, it's a good reminder for not doing this kind of things in stable. For avoiding the problem in 10.0 branches, you have to use this technique: https://github.com/OCA/bank-payment/pull/562/files#diff-c9e509eca85943bc8205a780bb737a66R16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.