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

Config.validate keeps 'mdx_configs' values across different instances #2289

Closed
oprypin opened this issue Jan 19, 2021 · 5 comments · Fixed by #2290 or #2353
Closed

Config.validate keeps 'mdx_configs' values across different instances #2289

oprypin opened this issue Jan 19, 2021 · 5 comments · Fixed by #2290 or #2353

Comments

@oprypin
Copy link
Member

oprypin commented Jan 19, 2021

import mkdocs.config

# 1. OK
conf = mkdocs.config.Config(mkdocs.config.DEFAULT_SCHEMA)
conf.load_dict({'site_name': 'foo'})
conf.validate()
assert conf['mdx_configs'].get('toc') == None

# 2. OK
conf = mkdocs.config.Config(mkdocs.config.DEFAULT_SCHEMA)
conf.load_dict({'site_name': 'foo', 'markdown_extensions': [{"toc": {"permalink": "aaa"}}]})
conf.validate()
assert conf['mdx_configs'].get('toc') == {'permalink': 'aaa'}

# 3. Identical to 1 but not OK.
conf = mkdocs.config.Config(mkdocs.config.DEFAULT_SCHEMA)
conf.load_dict({'site_name': 'foo'})
conf.validate()
assert conf['mdx_configs'].get('toc') == None
# fails, actually is {'permalink': 'aaa'}
@waylan
Copy link
Member

waylan commented Jan 20, 2021

It took me a couple minutes but I figured out the issue. The data is not shared across instances of the mkdocs.config.config_options.MarkdownExtensions class, however, because you are using mkdocs.config.DEFAULT_SCHEMA each time, then the same instance is being reused each time. The fix you provide in #2290 certainly would address that, but I wonder if we could fix it elsewhere. After all, that fix is non-intuitive and the sort of problem I would expect to get repeated in the future (either with this option or some other options).

Of course, this matters when using the live reload server. Each reload also reloads and validates the config. A change to the config should be reflected in the reload. I wonder if perhaps we need to change the way we are loading the DEFAULT_SCHEMA so that each reload gives us a new instance of each config option class. The tricky part is that the existing API is already in use by plugins, so we need to be careful not to break backward compatibility here.

@waylan
Copy link
Member

waylan commented Jan 20, 2021

Perhaps we should add some sort of reset to each config option class. Of course, some options won't need it, but for those that do, each reload of the config would call the reset, which would reset any variables which hold data. That way it would just be an add-on feature and wouldn't break existing plugins. A do-nothing reset method would be defined on the base class so all subclasses would have it. Then we simply ensure it is called on each reload.

@waylan waylan added the Bug label Jan 20, 2021
@waylan waylan added this to the 1.2 milestone Jan 20, 2021
@oprypin
Copy link
Member Author

oprypin commented Jan 20, 2021

I don't think I agree with your assessment that this is a general / likely recurring problem. Here it really is specific to the way mkdocs.config.config_options.MarkdownExtensions operates. It has a one-of-a-kind interaction between two config keys and the implementation just forgets that an instance of OptionallyRequired doesn't represent a value, but rather a slot for values, and so instance members can't be used for purposes specific to one particular value. While that's a little confusing, I think that this rule isn't too difficult to follow and probably has no other violations (and the reason that made it prone to violation here is specific to the 2-field interaction).

I also see that good care was taken not to accidentally mutate defaults from a schema, which could've been a whole another class of bugs and is what I suspected initially.

Anyway, so how could we make this less confusing? Make those instances actually act as classes that can themselves be instantiated? Are we delving into metaclasses?

Let me just cut myself short there,... from what I said above I just think that this doesn't require any significant rework.

And sure, you could introduce this reset, with the sole user being MarkdownExtensions. But that does nothing towards detecting preventing such bugs anyway.

@waylan
Copy link
Member

waylan commented Jan 20, 2021

And sure, you could introduce this reset, with the sole user being MarkdownExtensions. But that does nothing towards detecting preventing such bugs anyway.

The thinking was that potential future options or options for third party plugins could also run into this issue. True, the classes would need to make use of the rest method to get the benefits, but at least we would have an official API for it. In reality, the fix in #2290 does a "reset" as the first step of the run_validation method., but it is not some official API.

However, it is probably better to not even need to consider that when creating option classes. One simple solution that occurs to me is to regenerate the scheme each time. Therefore, in mkdocs.config.default, instead of having:

DEFAULT_SCHEME = (
    ...
)

we would have:

def get_scheme():
    return (
        ...
    )

Then where we currently make reference to mkdocs.config.DEFAULT_SCHEME, we would instead use mkdocs.config.default.get_scheme(). This would ensure that a new instance of each config option class would be used each time that the config is reloaded.

waylan added a commit to waylan/mkdocs that referenced this issue Apr 5, 2021
Replace the global variable `mkdocs.config.DEFAULT_SCHEMA` with
the function `mkdocs.config.defaults.get_schema()`. An instance is no
longer created on import (eliminating circular imports under certain
circumstances) and each call to `get_schema()` builds a new instance
of each object. Fixes mkdocs#2289.
waylan added a commit that referenced this issue Apr 5, 2021
Replace the global variable `mkdocs.config.DEFAULT_SCHEMA` with
the function `mkdocs.config.defaults.get_schema()`. An instance is no
longer created on import (eliminating circular imports under certain
circumstances) and each call to `get_schema()` builds a new instance
of each object. Fixes #2289.
@oprypin
Copy link
Member Author

oprypin commented Aug 7, 2022

I still think #2353 was not necessary. It happens to fix some cases but not others - see #2909 now.

The reason I think so is that the ConfigItem instances are just part of the definition of a config, you shouldn't need to make a new definition of the config every time. The objects just shouldn't store state of an individual item in them.

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