Add option to ignore objects that fail to build

[hughrawlinson]

Hi! I was having an issue where one of my object paths contained a string that was longer than the maximum filename length on ext filesystems. Because of this one object path, my entire bakery run was failing - and in my case, I would have preferred for that single missing file to be omitted from the generated static site, rather than to have had the entire build fail. I've opened this PR to add a setting to log an error and skip the file, rather than fail. The default behaviour is the pre-existing behaviour - the new skipping is opt-in.

I've tested this on my wagtail and it works as expected. However, I would greatly appreciate any feedback or advice you have. In particular, I have the following questions:

1. What's a good way to prepare a test case for this?
2. Is there a more specific exception I should catch? (see below)
3. Better to log a warning? Or use f strings there? I assume not, for backwards python compatibility?
4. Any thoughts on the name of the setting? It's BAKERY_IGNORE_OBJECTS_THAT_FAILED_TO_BUILD now.

The exception I was getting was an OSError number 36 - filename too long - in the get_build_path function. I suspect that it's better to catch a broader error in that function or in the subsequent calls to get_content and build_file - but maybe I've gone too broad in catching anything?

[emilytoppm]

Hi @hughrawlinson, thanks for contributing!

1. I would suggest using https://pytest-django.readthedocs.io/en/latest/helpers.html#settings to turn your setting on for the test in question. For the test itself - perhaps mocking/patching self.build_object to raise an exception when called instead of doing anything, and using the caplog fixture to check your error shows up in the logs but no exception is raised?
2. Given that in you log or re-raise the error, this is probably okay, but I would see if you can narrow it slightly - file related will likely end up in OSError, and the request related errors seem like they'll be triggering AttributeError. Any thoughts @zerolab or @loicteixeira?
3. Logging seems like a good idea here. f strings are fine in other contexts - looks like we require Wagtail 2.10 or later, which requires Python 3.6, so we have f strings available. However, there can be some problems using them in logging: https://blog.pilosus.org/posts/2020/01/24/python-f-strings-in-logging/.

I would suggest using logger.exception, however (see https://docs.python.org/3/library/logging.html#logging.Logger.exception) so we get exception info attached properly.
4. Maybe BAKERY_SKIP_FAILED_OBJECTS for brevity? But not a strong preference.

[hughrawlinson]

I think I've addressed most of the comments - I'm just stuck with testing, I'm not really sure how to write a test for this. I will use the pytest helper and mocking build_object seems like a good idea. Is there an existing test I should copy and adapt?

[zerolab]

Hey @hughrawlinson,

https://github.com/wagtail/wagtail-bakery/blob/master/tests/integration/test_views.py#L27 is probably the closest to emulate. You want to call https://github.com/wagtail/wagtail-bakery/blob/c63597ac405c3c35bc0b3206c58d6e5d7b9be6c8/src/wagtailbakery/views.py#L84 and probably mock WagtailBakeryView.build_file to raise an exception, then override_settings with different values for BAKERY_IGNORE_OBJECTS_THAT_FAILED_TO_BUILD

[hughrawlinson]

Looks like Prettier got me, would you like me to revert these style changes?