Skip to content

fix: make pebble.Notice hashable #2873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tonyandrewmeyer wants to merge 1 commit into from
Closed

fix: make pebble.Notice hashable #2873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ops/pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,7 +1693,7 @@ class Notice:
occurrences: int
"""The number of times one of these notices has occurred."""

last_data: dict[str, str] = dataclasses.field(default_factory=dict[str, str])
last_data: dict[str, str] = dataclasses.field(default_factory=dict[str, str], hash=False)

@benhoyt benhoyt Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is okay, but it seems to me that these values should form part of the hash. For example, we could include tuple(sorted(notice.last_data.items())) in the hash (though not sure how to do that with dataclasses, off the top of my head.

@james-garner-canonical james-garner-canonical Nov 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this is that the hash could change over the course of the object's lifetime, which breaks lookups.

class D:
    _hash: int = 1
    def __hash__(self):
        return self._hash
        
d1 = D()
d = {d1: 'd1'}
d[d1]  # 'd1'
d1._hash = 100
d[d1]  # KeyError!

@james-garner-canonical james-garner-canonical Nov 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the reason we're making it hashable is that we expect last_data to be treated as immutable, even though frozen dataclass guarantees don't extend to mutable containers today ...

@james-garner-canonical james-garner-canonical Nov 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we'd actually want a KeyError if a user accidentally mutated the object? So we'd be intentionally implementing a __hash__ that can break, so that it breaks loudly on user errors.

@tonyandrewmeyer tonyandrewmeyer Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, last_data is not intended to be changed (I assume) but since we don't have frozendict yet, it's a bit messy.

I think it shouldn't be in the hash if you see a notice as being a Pebble notice, where last_data is basically a shortcut to some extra information, but comparing two notices means comparing the key (but then the dates and occurrences probably shouldn't be in the hash either). It probably should be in the hash if you see a notice as being a snapshot of a Pebble notice at a specific time (more the Python object than the Pebble concept).

I don't think this is a real issue. It came up when I did one of my semi-regular sweeps for issues by an agent, and it seemed worth bundling into a set of small fixes. But it does seem extremely unlikely that anyone does modify last_data in practice. Maybe people want to be able to hash a notice (collecting them in a dict where the key is the dict key?) but no-one is doing that now, since it wouldn't work.

Maybe we should just reject this?

@benhoyt benhoyt Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favour of just rejecting this. I agree it seems very unlikely for it to be an issue.

"""Additional data captured from the last occurrence of one of these notices."""

repeat_after: datetime.timedelta | None = None
Expand Down
22 changes: 22 additions & 0 deletions test/test_pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,28 @@ def test_notice_from_dict(self):
occurrences=8,
)

def test_notice_hashable(self):
now = datetime.datetime.now(datetime.timezone.utc)

def make(data: dict[str, str]):
return pebble.Notice(
id='123',
user_id=1000,
type=pebble.NoticeType.CUSTOM,
key='example.com/a',
first_occurred=now,
last_occurred=now,
last_repeated=now,
occurrences=1,
last_data=data,
)

# Frozen, so usable as a dict key or set member, with the dict field
# left out of the hash but still part of equality.
assert len({make({'a': '1'}), make({'a': '1'})}) == 1
assert make({'a': '1'}) != make({'a': '2'})
assert hash(make({'a': '1'})) == hash(make({'a': '2'}))


class TestPlan:
def test_services(self):
Expand Down
Loading