Support pydantic.experimental.missing_sentinel.MISSING in ops.Relation.save

[dimaqq]

I've extended the existing test like so:

     class MyPydanticDatabag(pydantic.BaseModel):
-        foo: str
+        foo: str | MISSING = MISSING
         bar: int = pydantic.Field(default=0, ge=0)
         baz: list[str] = pydantic.Field(default_factory=list)
         quux: Nested = pydantic.Field(default_factory=Nested)
+        hah: int | MISSING = MISSING

And that fails with KeyError('hah') at:

/code/operator/test/test_model_relation_data_class.py(407)_on_relation_changed()
-> event.relation.save(data, self.app, encoder=self.encoder)
> /code/operator/ops/model.py(1918)save()
-> data = {field: encoder(values[attr]) for attr, field in sorted(fields.items())}

[dimaqq]

Debug quip:

(Pdb) fields
{'foo': 'foo', 'bar': 'bar', 'baz': 'baz', 'quux': 'quux', 'hah': 'hah'}
(Pdb) values
{'foo': 'other-value', 'bar': 28, 'baz': ['x', 'y'], 'quux': {'sub': 8}}

Our top-level keys as Juju databag fields doesn't get the MISSING default value in this case.

For comparison, Pydnatic itself does just fine:

(Pdb) data.model_dump()
{'foo': 'other-value', 'bar': 28, 'baz': ['x', 'y'], 'quux': {'sub': 8}}

[dimaqq]

@tonyandrewmeyer I'd appreciate your take on this.
upstream docs: https://docs.pydantic.dev/latest/concepts/experimental/#missing-sentinel

from standup discussion on Monday, we may in general have an issue with erasing existing fields: suppose we've got two models Foo(name, surname) and Bar(name):

• relation.save(Foo("John", "Doe"))
• relation.save(Bar("Jane"))

We'd be left with a stale surname=Doe field.
We need to decide if that's intended, passable, or undesired.

In this specific issue, if we do:

• relation.save(Foo("John", "Wayne"))
• relation.save(Foo("John", MISSING))

Surely we'd want to erase the the surname key in this case.

[dimaqq]

Aside: I've confirmed that setting a relation field to an empty string causes the field to be removed:

https://documentation.ubuntu.com/juju/3.6/reference/hook-command/list-of-hook-commands/relation-set/#details

> juju exec --unit evil/0 'relation-set -r 1 --app foo=""'
> juju show-unit traefik-k8s/0  # relation counterpart

[tonyandrewmeyer]

I don't think we want to always replace - I'm pretty sure that was discussed during the planning for .save and .load and decided against then (and it'd be a breaking change now). A use case is that you might have a class that only has a subset of the fields (maybe you even have two classes, each having part of the databag). Perhaps that's not good charm design, but we did deliberately support that. It's also more similar to Juju, where you have to explicitly remove fields, relation-set is an update rather than a replace.

I'm not sure that Pydantic's MISSING translates to "remove this field". Reading the documentation linked above, it seems like it's more like an omitempty kind of thing. When loading, it seems like it would be ok for it to be successful, and it looks like it gets the value MISSING (rather than raising AttributeError), so I assume that is ok at the moment? When saving, I think the expected behaviour would be to skip that attribute rather than setting it to the empty string (ie. deleting from Juju).

I feel like the fix should be something along the lines of:

data = {field: encoder(values[attr]) for attr, field in sorted(fields.items()) if attr in values}

Basically: if the Pydantic model dump doesn't include the value then we don't send it to Juju. Or perhaps instead of getting the fields from model_fields we should get them from the model dump.

Back to the core issue of removing fields: isn't that something that we are recommending against doing anyway? Is this for the downgrade case? I would think in that situation you're meant to ignore extra data? If it's upgrading, I thought we were telling people to not remove fields. If it's not upgrading or downgrading then I'm not sure why we want to remove a field.

[tonyandrewmeyer]

Or another option instead of adding an argument to .save or a helper is using a custom encoder. For example:

import dataclasses
import json
from typing import Any

import ops
from ops import testing

REMOVED = 'removed'  # but actually you'd make a proper sentinel

@dataclasses.dataclass
class Data:
    a: str
    b: str | REMOVED = REMOVED

def e(value: Any):
    if value is REMOVED:
        return ''
    return json.dumps(value)

class FakeRel:
    def __init__(self):
        self.data = {'x': {}}

f = FakeRel()
f2 = FakeRel()
d = Data('hello')
d2 = Data('hello', 'world')

ops.Relation.save(f, d, 'x', encoder=e)
print(f.data['x'])

ops.Relation.save(f2, d2, 'x', encoder=e)
print(f2.data['x'])

# Output:
# {'a': '"hello"', 'b': ''}
# {'a': '"hello"', 'b': '"world"'}

(That could be pydantic, and it could use a real relation, it was just the simplest way to throw together an example).

You couldn't use the pydantic MISSING, since that's not in the fields so never gets encoded, but this also avoids laying on a meaning of "REMOVE" on to "MISSING", which I think are different things.