Relation data contents seems to support non-string keys, except it doesn't

[PietroPasotti]

If I do relation.data[self.unit]['foo'] = 42, I get a nice ModelError telling me relation data values should be strings. Fair enough.
If I do relation.data[self.unit][42] = 'foo', I get no error, if I inspect the databag the value is there:
assert relation.data[self.unit][42] == 'foo' works allright.

however, nothing is committed (I think?) and when the charm executes again, the data is gone.

This looks like a bug?

[PietroPasotti]

@pengale @rwcarlsen I think the issue is that in Model.relation_set we create the arguments to relation-set by 'blindly' casting key and value to str.
Which means that if I pass relation_set(object(), '2') our relation data will contain {"<object object at 0x7f0785ba3b00>": "2"}.

Is this something that we want to address in a more generalized manner?
Like, validate all arguments passed to hook-tools and force them to be 'primitive' types? Likely: Union[str, float, int, bytes]?
Even bool feels a bit iffy, because nobody but Python might understand "True"/"False".

@jameinel thoughts on this?

[rwcarlsen]

From the ops standpoint, it seems like we should keep setting and getting symmetrically w.r.t. types. If we start auto-converting some types to str, then how will we know to auto-convert them from string when the user retrieves them? We can't. Because of this, I think it's best that we only explicitly take and provide strings.

[PietroPasotti]

My point is that right now we are implicitly and silently converting everything to string.
I believe there's a line that goes: check_call('relation-set {}={}'.format(arg1, arg2))
which is what leaves us exposed to bugs if arg1 or arg2 are NOT strings.

My proposal would be to be more strict and check beforehand that arg1 and arg2 are already strings (or a handful of primitive types we're convinced we can safely stringify without problems)

[rwcarlsen]

@PietroPasotti I'm mostly in agreement - just that I don't think we should do any types other than string unless we have a way to know when to convert to non-string types upon retrieval (we currently don't).

[PietroPasotti]

Ok, so I think we're totally in agreement: we should do

assert isinstance(key, str)
assert isinstance(value, str)

on each relation_set(key, value) call.