Fix #979 - validators must not parse with toml #980
Conversation
3 years ago I made the wrong decision to pass validators defaults to TOML parser. Validator object is created at Python level, or at YAML level where there is no need to use toml as parser. TOML parser must ideally be used only on environment variables where there is no type system available. That said, this PR ensures toml will keep parsing values coming from envvars, but setdefault method will stop passing `tomlfy=True` so the type informed on the validator will be the type set as default value. The only possible breaking change on this is if someone is setting defaults relying on TOML to parse the data, in that case, user will need to manually call `parse_with_toml` on default data. I think that is not a common case, if someone reports it as a broken change, we can provide a new argument `parse_default_with_toml` on the Validator object later.
| @@ -396,7 +396,7 @@ def setdefault( | |||
| item, | |||
| default, | |||
| loader_identifier=loader_identifier, | |||
| tomlfy=True, | |||
| tomlfy=False, | |||
There was a problem hiding this comment.
all other set methods such as set and update have tomlfy=False as a default, the reason for it is for loaders to be able to enable it on-demand, env_loader will always pass `True to that parameter, while other loaders that can rely on a type system will probably not need to parse data again. (but looks like that is not the current case)
If I was going to reimplement this today, I would accept just the bare value and do not parse it, require that the caller parses data if needed, but now it is too late for this change, so lets deal with it.
| @@ -262,7 +262,8 @@ def _validate_items( | |||
| and self.is_type_of is str | |||
| ): | |||
| # avoid TOML from parsing "+-1" as integer | |||
| default_value = f"'{default_value}'" | |||
| # by forcing casting to str | |||
| default_value = f"@str {default_value}" | |||
There was a problem hiding this comment.
toml parsing will not be executed by setdefault, however the converters like @str will keep being processed! (this must have been the solution 3 years ago)
| validators=[Validator("TEST", default="+172800")] | ||
| validators=[Validator("TEST", default=-172800)] |
There was a problem hiding this comment.
This is a possible breaking change, but I guess no one is relying on it, it was odd to pass a str to a default and expect an integer back.
Now the behavior is more consistent, if default needs to be integer, the user must pass integer or add @int as prefix.
| DYNACONF_GROUP__TEST_LIST = ["'1'", "'2'"] # double quotes to cast as string | ||
| DYNACONF_GROUP__TEST_LIST="['1', '2']" |
There was a problem hiding this comment.
Double quotes is required to force str internal values, but the recommended way is to pass it outside not internally.
$ DYNACONF_LIST="['1','2']" dynaconf list -k LIST
LIST<BoxList> <BoxList: ['1', '2']>
There was a problem hiding this comment.
I've found tomlfy confusing sometimes, but now its clearer what it purpose was.
I think it is ok to do this break changes as it is low risk and is a design fix. It may be worth to review the documentation on this to assure it is up-to-date (I can do it tomorrow). I'd also suggest adding this to the next minor release (3.3.0) and add a upcoming Breaking Change Warning on the changelog of the next patches (3.2.x).
|
@rochacbruno This section of the docs needs to be updated: Lines 533 to 550 in 297eefc |
|
And it would be good to add the motivation for |
|
This implementation will be obsolete and replaced by #981 |
#979 #905
3 years ago I made the wrong decision to pass validators defaults to TOML parser.
Validator object is created at Python level, or at YAML level where there is no need to use toml as parser.
TOML parser must ideally be used only on environment variables where there is no type system available.
That said, this PR ensures toml will keep parsing values coming from envvars, but setdefault method will stop passing
tomlfy=Trueso the type informed on the validator will be the type set as default value.The only possible breaking change on this is if someone is setting defaults relying on TOML to parse the data, in that case, user will need to manually call
parse_with_tomlon default data.I think that is not a common case, if someone reports it as a broken change, we can provide a new argument
parse_default_with_tomlon the Validator object later.