lektor.db.Record equality

[relikd]

Should two pages be equal even though the alt is different? I just stumbled upon this because I wanted to store source objects in a set but the hash and equality says it is the same object.

lektor/lektor/db.py

Lines 483 to 488 in 74bce90

	def __eq__(self, other):
	if self is other:
	return True
	if self.__class__ != other.__class__:
	return False
	return self["_path"] == other["_path"]

[dairiki]

Agree, that does not seem right. We should probably fix that.

I do get just a little queasy thinking about what subtle breakage might occur if we do.

For grins, I tried changing Record.__eq__ to check (path, alt) for the equality key, and it didn't seem to break any of our tests, for whatever that's worth.

A quick look, though, found set operations on pages being performed here:

lektor/lektor/build_programs.py

Lines 224 to 225 in 74bce90

	pq = p_config.get_pagination_query(self.source)
	child_sources.append(set(all_children) - set(pq))

Those set operations could be affected by a change to page equality semantics — though in this specific case I don't think it would make a difference. I don't think our tests currently have good coverage for this sort of thing, however.

__hash__ is broken, too

lektor/lektor/db.py

Lines 493 to 494 in 74bce90

	def __hash__(self):
	return hash(self.path)

Here, __hash__ depends on self.path, while __eq__ looks at self["_path"]. Those are potentially different, e.g. for Pages that have a page_num set.

lektor/lektor/db.py

Lines 557 to 562 in 74bce90

	@cached_property
	def path(self):
	rv = self["_path"]
	if self.page_num is not None:
	rv = "%s@%s" % (rv, self.page_num)
	return rv

This means that currently, two Pages can have different hashes, while still comparing equal. This violates Python's requirement that hashes for objects that compare equal must be equal.

In other words, for Pages, page_num should also be included in the equality key.

[dairiki]

I just found this is my year-old draft PR #1007. (So, as I said, I guess I agree with you.) 😆

I suppose I should put some more work into that PR.