fix!: correct the model config types

[tonyandrewmeyer]

Corrects types related to the model configuration:

• LazyMapping may have Boolean, integer, or float values, as well as strings
• _ConfigOption may have a secret value for `type - also move this to testing.py since that's the only place it's used and it's private
• _ModelBackend.config_get returns a dict with Boolean, integer, float, or string values

The PR also sets a missing return type so that pyright would detect the config_get issue.

There are no tests or pyright changes for the _ConfigOption change: we don't generally rely on pyright rather than unit tests for ensuring the typing is correct, but that doesn't seem feasible here. Let me know if you think we do need something added here to prevent regression.

There are no tests or pyright changes for the LazyMapping change, other than making sure we do have test_model.py exercise each of the config types. I don't think it's possible to have pyright detect this issue - e.g. you can get it to show an error if you try to use an integer config value as an integer (self.config["int-opt"] + 1), but that's still the case after the fix, since it could be a string (float, Boolean, secret) option. The test could have typing.assert_type calls but (a) that's only in Python 3.11, and (b) I'm not convinced we really want to do that. Let me know if you think we do need something added here to prevent regression.

Fixes #1182

[tonyandrewmeyer]

There was a bit more (all minor) breakage than I had expected (it's interesting that so many charms are actually doing static checking on this and yet no-one noticed that the type was wrong - I suspect that says something about how typing is used in Python). However, for all the charms I'm aware of, there should be PRs (big list above) that fix things.

[tonyandrewmeyer]

I'm not thrilled about adding this here. However, I couldn't find any other way to satisfy the way one of the charms (Jenkins, I think) was using relation data.

A minimal example is:

class A(typing.TypedDict):
    foo: str

r = ops.RelationDataContent(None, None, None)  # type:ignore
a = A(foo="hello")
r.update(a)

mypy complains about this:

/tmp/reldata.py:9: error: Argument 1 to "update" of "MutableMapping" has incompatible type "A"; expected "SupportsKeysAndGetItem[str, str]"  [arg-type]
/tmp/reldata.py:9: note: Following member(s) of "A" have conflicts:
/tmp/reldata.py:9: note:     Expected:
/tmp/reldata.py:9: note:         def __getitem__(self, str, /) -> str
/tmp/reldata.py:9: note:     Got:
/tmp/reldata.py:9: note:         def __getitem__(self, str, /) -> object

The charm error is the same.

It seems like it shouldn't be required, because RelationDataContent has an explicit __getitem__ that does return str, and the MutableMapping is [str, str]. It might be that this is a limitation of mypy? pyright does complain too, though:

/tmp/reldata.py
  /tmp/reldata.py:9:1 - error: No overloads for "update" match the provided arguments (reportCallIssue)
  /tmp/reldata.py:9:10 - error: Argument of type "A" cannot be assigned to parameter "__m" of type "Iterable[tuple[str, str]]" in function "update"
    "A" is incompatible with "Iterable[tuple[str, str]]"
      Type parameter "_T_co@Iterable" is covariant, but "str" is not a subtype of "tuple[str, str]"
        "str" is incompatible with "tuple[str, str]" (reportArgumentType)
2 errors, 0 warnings, 0 informations 

[tonyandrewmeyer]

I added this in the main conversation, but repeating here in context: I would prefer to have this be LazyMapping and just have the generic type made concrete for RelationDataContent (like with ConfigData), but that would be a backwards-incompatible change and isn't actually required to fix the bug.

[tonyandrewmeyer]

See the note above: this was the only way I could find to satisfy mypy when using relation.data[x].update({...}) where the value passed was a TypedDict.

[addyess]

@tonyandrewmeyer i just discovered this change has broken an untested situation:

this module previously could execute

some_dict = {"foo": "bar"}
self.relation.data[self.charm.model.unit].update(**some_dict)

but now there's an exception because i don't have an other argument

  File "/var/lib/juju/agents/unit-kubernetes-worker-0/charm/venv/ops/interface_aws/requires.py", line 133, in _joined
    self._request({"instance-id": self.instance_id, "region": self.region})
  File "/var/lib/juju/agents/unit-kubernetes-worker-0/charm/venv/ops/interface_aws/requires.py", line 190, in _request
    self._to_publish.update(**kwds)
TypeError: RelationDataContent.update() missing 1 required positional argument: 'other'

[addyess]

here's an example where both styles are acceptable in an update

Python 3.12.3 (main, May 22 2024, 13:41:37) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> a = {}
>>> a.update(**{"a": 1})
>>> a
{'a': 1}
>>> b = {"b": 2}
>>> a.update(b)
>>> a
{'a': 1, 'b': 2}
>>> c, d = {"c": 3}, {"d": 4}
>>> a.update(c, **d)
>>> a
{'a': 1, 'b': 2, 'c': 3, 'd': 4}

[benhoyt]

Ugh, what a mess Python typing is. Thanks for your effort here. It's rather annoying for charmers to have type cast(str, ...) all their config values now, but it does seem correct.

If config was a Mapping[str, Any], would they still have to `cast(str, ...)?

[tonyandrewmeyer]

If config was a Mapping[str, Any], would they still have to `cast(str, ...)?

I don't think so, although I didn't try it with the more complicated situations (like the one where update believes an object is being returned). If I understand correctly, in general Any works as some sort of magic 'can match anything', so you can do a = b where a needs to be a str and b is Any, even though it really seems like you only ought be able to do b = a (ie. "b can be anything, so a str is fine" rather than "a has to be a str, so 'no idea what it is' is fine").

I think it does then kind of 'pollute' the type checking, although if it's only being used once quickly afterwards (which was the case in some places, like getting written directly out into application config envars) it doesn't really matter. I think it's nicer if we avoid the "turn off type checking for this thing" behaviour.

For what it's worth, a few charms have merged the appropriate change without (so far) any charmers saying "ugh, I'm going to hate having these casts".