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

Uniform handling of deferred (descriptor) model data #789

Merged
merged 7 commits into from
Feb 6, 2021

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Jun 18, 2020

Issue(s) Resolved

TLDR; Lektor provides a mechanism whereby the computation of model field values can be deferred until such time as the values are accessed (e.g. via template expansion). (The built-in markdown type makes use of this feature, as do a number of plugin-provided extension types.) Currently, there are a couple of places where reference to such a deferred field value will not work — worse yet, in these cases, failure is silent and, thus, confusing. This pull request aims to fix things so that deferred field values will work as expected in those cases.

Background

Lektor provides a mechanism whereby the computation of certain values of page model data can be deferred until template-evaluation time. (For example, the compilation of markdown text within a field of type markdown does not happen until template-evaluation time.) (This late-evaluation happens in Record.getitem, where, if field value appears to be a descriptor, its .__get__ method is called to compute the final value.)

Normally, these deferred values are transparently computed when they are accessed, either programmatically (e.g. page['body']) or within a Jinja template expression (e.g. {{ this.body }}), so for the most part, these deferred types “just work”.

There are a couple of code paths in the current code base which access the raw model data directly, rather than accessing it through Record.__getitem__. In these cases, deferred values do not work. Worse yet, currently, no warning or error messages are generated when this happens, so the failures are hard for the user to diagnose.

Fortunately, both of these cases are fairly straightforward to fix. This pull request does that.

Problematic code-paths

There are, however, a couple of code-paths where access to model data does not currently go through Record.__getitem__. Attempts to access deferred values in those code-paths will fail (usually in a subtle way, without generating any sort of explicit error or warning message.)

Two code-paths where this happens are:

There are other places where the raw model data is accessed directly without going through Record.__getitem__, but in those cases, specific system fields (e.g. _model or _hidden) are being accessed. I do not believe these system fields ever contain deferred values, so directly accessing them does/should not cause issues.

Default slug computation

The computation of the default value for a page’s _slug happens in Database.get_default_slug.

For example, suppose we would like to create URLs for blog posts that contain the year of publication. Setting slug_format to

{{ "{0.year:04d}/{1}".format(this.pub_date, this._id) }}

in the datamodel for the blog parent page will do this.

This currently fails, however, in cases where pub_date is a deferred value. This might happen when using the lektor-git-timestamp plugin which can be used to auto-compute pub_date from the git commit timestamps. Currently, when the slug_format value is jinja-evaluated, this is set not to a Page instance, but rather to a dict containing the raw page data. This.pub_date in this context is the deferred descriptor, not the actual desired value. Currently this will result in silent failure of the expansion of slug_format.

This can be fixed by deferring the computation of the default _slug value until after the Page instance is fully constructed.

Record.get_sort_key

In Record.get_sort_key, the sort key value is fetched directly from the raw model data,
rather than through Record.__getitem__.

As a result, if attempts are made to sort blog posts by, e.g., pub_date, and pub_date is a deferred value as described above, this will cause problems.

This can be fixed, simply by using Record.__getitem__ to access the sort field’s value.

Related Issues / Links

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Modified Database.process_data to defer computation of the default _slug. This solves the issue with slug_formats which access deferred model data.
  • Modified Record.sort_key to access model field values using Record.__getitem__ rather than accessing the raw model data (this._data) directly. This fixes the issue with using deferred fields as sort keys.
  • Added a diagnostic message to indicate when expansion of slug_format fails (instead of silently using the fall-back slug when there is an error in the slug_format.)

@dairiki dairiki marked this pull request as ready for review June 18, 2020 19:00
@dairiki dairiki force-pushed the feature.defer-slug-format branch 2 times, most recently from 81da822 to 25ac55e Compare February 4, 2021 16:55
@dairiki
Copy link
Contributor Author

dairiki commented Feb 4, 2021

@xlotlu @runfalk Is there any chance of getting this merged?

Yesterday, I set up a new site-building environment on a new computer. I stumbled across the issues fixed by this PR and spent a couple of hours "fixing" them before discovering that I had already fixed them once already in this PR. 😂😐

(I've just now rebased this back onto the current master.)

Copy link
Contributor

@yagebu yagebu left a comment

Choose a reason for hiding this comment

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

Hi, thanks for this PR and the detailed description of the issues you fixed. This looks pretty good, I just left two small comments.

lektor/db.py Outdated Show resolved Hide resolved
lektor/db.py Show resolved Hide resolved
@dairiki
Copy link
Contributor Author

dairiki commented Feb 6, 2021

Okay. I've rebased again (due to merge conflicts introduced by #871) and deleted the spurious and unused type_ argument.

Anything else?

Copy link
Contributor

@yagebu yagebu left a comment

Choose a reason for hiding this comment

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

Anything else?

Nope - thanks again :)

@yagebu yagebu merged commit a7ad0ff into lektor:master Feb 6, 2021
@dairiki
Copy link
Contributor Author

dairiki commented Feb 6, 2021

Thank you!

@dairiki dairiki deleted the feature.defer-slug-format branch February 19, 2021 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants