fix: support Pydantic MISSING sentinel in ops.Relation.save

[dimaqq]

Support Pydantic's experimental MISSING sentinel field types and values.

ops.Relation.load() does the right thing already.
ops.Relation.save() needs a small fix.

Check list:

• core code change
• unit tests pass
• ruff format
• docs update
• test on real Juju --> chore: test Pydantic MISSING dimaqq/hexanator#7
• separate new test case
• make the new test conditional on being able to import MISSING
• (whatever reviews bring)

Fixes #2299

[james-garner-canonical]

The core code change looks good to me, as does the to-do list.

[dimaqq]

This is where we use Pydantic's slightly magic method to get the values from the Pydantic object.

The model_dump() drops the fields with the MISSING value.

[james-garner-canonical]

The implementation looks good based on our team discussion.

The addition of a class using this sentinel to the tests is great, ensuring that we don't crash on MISSING anymore. I think it would be a good idea to add a test that explicitly asserts that MISSING doesn't get serialized as well.

Are you still planning to add some real Juju tests, per the checklist in the PR description? I'm not sure if they're necessary, but I'm definitely not opposed to us adding more integration tests.

[tonyandrewmeyer]

I remain unconvinced that there's a need to support this use-case, and also that "MISSING" should mean "REMOVE", but the PR does implement the team decision.

I do think we should explicitly document the behaviour in a comment - maybe also in the reference doc as well (it feels like that should be expected if we want charms to rely on this behaviour).

[dimaqq]

@james-garner-canonical this assertion ensures that the field with a MISSING value does not end up in the databag.

[james-garner-canonical]

Ah, good point!

[james-garner-canonical]

Saying that fields without a value will be erased from the relation data is clear, but not quite true:

• If a field from dataclasses.fields isn't present on the object, dataclasses.asdict will raise an AttributeError.
• If a field from get_type_hints is not present on a vanilla class, getattr(obj, field) will raise an AttributeError.

We could 'fix' this in the vanilla class case (getattr(obj, field) for field in fields if hasattr(obj, field)), but the dataclasses.asdict case would be trickier to change, and I wouldn't suggest it.

Whether we change the behaviour for vanilla classes or not, I think we should document the expected behaviour in all three cases: Pydantic=erase, dataclasses=error, vanilla=?

[dimaqq]

I think that the only other "thing" where a field can be legally missing is a TypedDict with a NotRequired annotation.

Today, these are not supported:

• .load(SomeTypeDict, rel) will return a plain dict with everything in the databag, ignoring the typed keys
• .save(SomeTypeDict(foo="bar"), rel) will write nothing, whether values match declared keys or not

neither raises any errors...

So, I'd punt any discussion on other data-ish classes.
Let's solve Pydantic specifically in this PR.

[james-garner-canonical]

Yes, let's leave TypedDict out of scope for this PR. I'm on board with not making any further behaviour changes.

However, I think we should either:

• Be clear that for dataclasses and vanilla classes, a 'field' not having a corresponding 'value' will lead to an error instead.
• Or be clear that omitting a value is only really possible with Pydantic classes via model_dump.

I think the new docstring gets pretty close on the latter. But maybe it could be:

        If a Pydantic model's ``model_dump`` method omits any field
        (e.g. if its value is Pydantic's ``MISSING`` sentinel)
        the field will be erased from the relation data.

[james-garner-canonical]

I'm happy with the new behaviour and with the test coverage.