canonical / canonical/operator

Relation.save should avoid writing relation data that only differs in ordering

Open
#2,736 0 comments 1 reaction 0 assignees View on GitHub
27.04 needs design roadmap
Dominant language
Python
Stars
267
Forks
136
Avg merge
1d 22h
Merged PRs (30d)
28

Description

## Context

https://github.com/michaeldmitry/flaplint detects charms causing spurious `relation-changed` events by writing relation data that hasn't changed, but results in a different string value being sent to Juju due to the *ordering* of items in the data changing -- for example items that are passed through a Python `set` to ensure uniqueness will naively have a different order on every run.

We [recommend](https://canonical.com/juju/docs/charmlibs/how-to/design-relation-interfaces/#collections) that interfaces shouldn't rely on collection ordering (and that collections should always be emitted in a stable order -- the issue the `flaplint` detects), but existing interfaces may not follow that advice.

`flaplint` was developed in response to this issue being encountered by charmers in the wild, and the possibility of Ops doing something to avoid charms making this kind of mistake was raised [on matrix](https://matrix.to/#/!s3OSHIxrClWP1K5Jb11DYSWSfq4ZxvE6BJN5RiQEqFU/$DDHHk1gObWT2pNsNwPyCQ1g7Ob_xZgBxAUHxLMX6D0s?via=ubuntu.com&via=matrix.org).

## Scope

`RelationDataContent.update` accepts values in their already-serialized string form, so we can't sort or otherwise validate content provided via this API.

The more recently developed typed `Relation.save` takes a `dataclass` instance or Pydantic model instance and either reads the current values of `dataclasses.fields` or calls `model_dump(mode='json', ...)` (for Pydantic models only currently -- see #2480 , passing the resulting values through either `json.dumps` or a user-provided encoder function.
- This means that if no encoder function is provided, then the values must be serializable by `json.dumps`, so they must be standard built-in data types (`str`, `int`, `float`, `bool`, `None`; or recursively a `list`, `tuple` or `dict` of the same).
- Conversely, if an encoder function is provided, we can make no assumptions about the values (the custom encoder might accept arbitrary value types that only implement comparison by identity, for example), nor about whether a write is a no-op (the encoder might introduce additional data to the serialized form, for example resolving placeholder values depending on the current state or time).

This means that we can only guarantee behaviour for `Relation.save` with no custom encoder -- so this is the only existing API we can modify to address this issue.

## Possible actions

The suggestion in discussion was to deep-sort relation data before serializing it. However, our concern with sorting as the solution is that if Ops guarantees that relation data is sorted, then charms may rely on this behaviour. We should also consider that if a charm is refreshed to a revision with an Ops version implementing this behaviour, it may result in a spurious `relation-changed` due to the data now being sorted before writing.

An alternative would be to check if the relation data to be written is identical to the existing relation data, modulo collection ordering. But this is only possible if we have the deserialized values of the current relation data.
- We could cache the value of `Relation.load` somewhere internal, but there's no guarantee that `Relation.load` is always called before `Relation.save` in every event. Instead we'd have to call `Relation.load` (or its internals) ourselves internally.
- This could work: we should be able to derive the class needed for `load` from the object provided to `save`, and we shouldn't need a custom decoder since we'll only act if there's no user-specified encoder -- however, we can't really guarantee that the same type can be used for saving and loading cleanly (this would be a new requirement for `Relation.save`), so this needs some further thought.

## Backwards compatibility

Given the scope, we need to determine whether there are any existing charms that have adopted `Relation.save`, don't provide a custom encoder, and expect collection order in relation data to be meaningful. Because relation data is inherently cross-charm, this may be better suited to more open-ended agent-powered investigation than our usual approach of checking if static analysis or unit tests fail.

Contributor guide

Open the contributing guide

Research direction

Start by tracing Relation.save, Relation.load, and RelationDataContent.update, including the typed dataclass and Pydantic paths described in the issue. Investigate backwards compatibility for existing charms and determine a safe, testable behavior for order-only changes; done means the approach is agreed and its compatibility impact is documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.