Overwrite policy: unread 'by name' lookup, silent drop on model reassignment, undocumented snapshot semantics

[LukasGold]

Three smaller items in the selective overwrite feature, found during the assessment in #163 and listed there under "Found but not fixed - needs a design decision". All three are still present on main (v2.5.1, 9351f6b). None of them produces a wrong write. Item 2 hides a caller error, items 1 and 3 cost maintenance effort.

Split off from #190, which covers the two items from the same list that do produce wrong results.

1. _overwrite_per_class["by name"] is built but never read

• declared as a nested dict at

  osw-python/src/osw/core.py

  Line 1663 in 9351f6b

  _overwrite_per_class: Dict[str, Dict[str, OSW.OverwriteClassParam]] = (

  and initialised at

  osw-python/src/osw/core.py

  Line 1690 in 9351f6b

  self._overwrite_per_class = {"by name": {}, "by type": {}}

• written and duplicate-checked at

  osw-python/src/osw/core.py

  Lines 1696 to 1705 in 9351f6b

  	model_name in self._overwrite_per_class["by name"].keys()
  	or model_type in self._overwrite_per_class["by type"].keys()
  	):
  	raise ValueError(
  	f"More than one OverwriteClassParam for the class "
  	f"'{model_type}' ({model_name}) has been passed in the "
  	f"list to 'overwrite_per_class'!"
  	)
  	self._overwrite_per_class["by name"][model_name] = param
  	self._overwrite_per_class["by type"][model_type] = param

• the only lookup reads ["by type"], at

  osw-python/src/osw/core.py

  Line 1996 in 9351f6b

  class_param = param._overwrite_per_class["by type"].get(class_type, None)

No line reads ["by name"]. A flat dict keyed by the OSW type carries the same information.

2. Reassigning model drops unknown per_property keys silently

__setattr__ calls _sync_per_property() whenever model, overwrite or per_property is assigned. The rebuild iterates the fields of the new model and reads the old per_property with .get:

osw-python/src/osw/core.py

Lines 1427 to 1431 in 9351f6b

	per_property_ = self.per_property or {}
	self._per_property = {
	field_name: per_property_.get(field_name, self.overwrite)
	for field_name in self.model.__fields__.keys()
	}

A per_property key that is not a field of the newly assigned model is never looked up and never reaches _per_property. The key check runs only in the construction-time validator:

osw-python/src/osw/core.py

Lines 1351 to 1368 in 9351f6b

	@validator("per_property")
	def validate_per_property(cls, per_property, values):
	if per_property is None: # nothing to check, the fallback applies
	return per_property
	model_ = values.get("model")
	if model_ is None:
	# 'model' itself did not validate; without it the property names
	# below cannot be checked at all
	raise ValueError("'model' is required to validate 'per_property'")
	field_names = list(model_.__fields__.keys())
	keys = per_property.keys()
	if not all(key in field_names for key in keys):
	missing_keys = [key for key in keys if key not in field_names]
	raise ValueError(
	f"Property not found in model: {', '.join(missing_keys)}"
	)
	return per_property

Assigning a new model can therefore turn a declared per-property protection into no protection, without an error. Expected: the same ValueError that the validator raises at construction.

3. Snapshot semantics are not documented

pydantic v1 copies a model on validation, so StoreEntityParam stores a copy of every OverwriteClassParam passed to it. Mutating the original afterwards has no effect on the policy that is applied.

This is the intended behaviour, but no docstring states it. Build the policy before constructing the param.

• OverwriteClassParam:

  osw-python/src/osw/core.py

  Lines 1340 to 1436 in 9351f6b

  	class OverwriteClassParam(OswBaseModel):
  	model: Type[OswBaseModel] # ModelMetaclass
  	"""The model class for which this is the overwrite params object."""
  	overwrite: Optional[OVERWRITE_CLASS_OPTIONS] = False
  	"""Defines the overall overwriting behavior. Used for any property if the
  	property specific setting is not set."""
  	per_property: Optional[Dict[str, OverwriteOptions]] = None
  	"""A key (property name) - value (overwrite setting) pair."""
  	_per_property: Dict[str, OVERWRITE_CLASS_OPTIONS] = PrivateAttr()
  	"""Private property, for internal use only. Use 'per_property' instead"""
  	@validator("per_property")
  	def validate_per_property(cls, per_property, values):
  	if per_property is None: # nothing to check, the fallback applies
  	return per_property
  	model_ = values.get("model")
  	if model_ is None:
  	# 'model' itself did not validate; without it the property names
  	# below cannot be checked at all
  	raise ValueError("'model' is required to validate 'per_property'")
  	field_names = list(model_.__fields__.keys())
  	keys = per_property.keys()
  	if not all(key in field_names for key in keys):
  	missing_keys = [key for key in keys if key not in field_names]
  	raise ValueError(
  	f"Property not found in model: {', '.join(missing_keys)}"
  	)
  	return per_property
  	@classmethod
  	def _normalize_overwrite(cls, value):
  	"""Replace the two non-policy values by the default setting.
  	Neither ``None`` nor the ``none`` sentinel is a policy:
  	``get_overwrite_setting()`` would hand them to the merge, where they
  	match no branch and silently behave like 'false'.
  	"""
  	if value is None or value is AddOverwriteClassOptions.none:
  	return cls.__fields__["overwrite"].get_default()
  	return value
  	def __setattr__(self, key, value):
  	"""Called when setting an attribute"""
  	if key == "overwrite":
  	value = self._normalize_overwrite(value)
  	# the effective settings are derived from these three, so any of them
  	# changing has to rebuild them
  	if key not in ("model", "overwrite", "per_property"):
  	super().__setattr__(key, value)
  	return
  	previous = getattr(self, key)
  	super().__setattr__(key, value)
  	try:
  	self._sync_per_property()
  	except ValueError:
  	# _sync_per_property() rejects before it touches _per_property,
  	# so restoring the field is enough to undo the assignment. Leaving
  	# a rejected value in place would let it take effect later, on the
  	# next assignment that happens to be accepted.
  	super().__setattr__(key, previous)
  	raise
  	def __init__(self, **data):
  	"""Called after validation. Sets the fallback for every property that
  	has not been specified in per_property."""
  	super().__init__(**data)
  	# routed through __setattr__, which normalizes and rebuilds
  	self.overwrite = self.overwrite
  	# todo: from class definition get properties with hidden /
  	# read_only option # those can be safely overwritten - set the to True
  	def _sync_per_property(self) -> None:
  	"""Rebuild the effective overwrite setting of every model field."""
  	if self.per_property and isinstance(
  	self.overwrite, AddOverwriteClassOptions
  	):
  	# _apply_overwrite_policy() short-circuits on 'replace remote'
  	# and 'keep existing' before it looks at a single property, so
  	# this combination would discard 'per_property' silently. Check
  	# it here rather than in a validator so that it also holds when
  	# either field is reassigned after construction.
  	raise ValueError(
  	f"'per_property' cannot be combined with overwrite="
  	f"'{self.overwrite.value}', which acts on the entity as a "
  	f"whole. Use an OverwriteOptions value for 'overwrite'."
  	)
  	per_property_ = self.per_property or {}
  	self._per_property = {
  	field_name: per_property_.get(field_name, self.overwrite)
  	for field_name in self.model.__fields__.keys()
  	}
  	def get_overwrite_setting(self, property_name: str) -> OverwriteOptions:
  	"""Returns the fallback overwrite option for the given field name"""
  	return self._per_property.get(property_name, self.overwrite)

• StoreEntityParam:

  osw-python/src/osw/core.py

  Lines 1625 to 1667 in 9351f6b

  	class StoreEntityParam(OswBaseModel):
  	entities: Union[OswBaseModel, List[OswBaseModel]] # actually model.Entity
  	"""The entities to store. Can be a single entity or a list of entities."""
  	namespace: Optional[str]
  	"""The namespace of the entities. If not set, the namespace is derived from the
  	entity."""
  	parallel: Optional[bool] = None
  	"""If set to True, the entities are stored in parallel."""
  	overwrite: Optional[OVERWRITE_CLASS_OPTIONS] = "keep existing"
  	"""If no class specific overwrite setting is set, this setting is used."""
  	overwrite_per_class: Optional[List[OSW.OverwriteClassParam]] = None
  	"""A list of OverwriteClassParam objects. If a class specific overwrite setting
  	is set, this setting is used.
  	"""
  	remove_empty: Optional[bool] = True
  	"""If true, remove key with an empty string value from the jsondata."""
  	change_id: Optional[str] = None
  	"""ID to document the change. Entities within the same store_entity() call will
  	share the same change_id. This parameter can also be used to link multiple
  	store_entity() calls."""
  	bot_edit: Optional[bool] = True
  	"""Mark the edit as bot edit,
  	which hides the edit from the recent changes in the default filer"""
  	edit_comment: Optional[str] = None
  	"""Additional comment to explain the edit."""
  	meta_category_title: Optional[Union[str, List[str]]] = "Category:Category"
  	debug: Optional[bool] = False
  	offline: Optional[bool] = False
  	"""If set to True, the processed entities are not upload but only returned as WtPages.
  	Can be used to create WtPage objects from entities without uploading them."""
  	verify_write: Optional[bool] = True
  	"""If set to True, the existence of every edited page is queried after the
  	upload. A page that does not exist afterwards is reported in
  	StoreEntityResult.failed instead of StoreEntityResult.pages. This costs one
  	additional API request per 50 edited pages, and one further request some
  	seconds later if a page is reported as missing. If the query itself fails,
  	the pages are reported as stored and an error is logged. Has no effect if
  	'offline' is True."""
  	_overwrite_per_class: Dict[str, Dict[str, OSW.OverwriteClassParam]] = (
  	PrivateAttr()
  	)
  	"""Private attribute, for internal use only. Use 'overwrite_per_class'
  	instead."""

Status

Fixes for all three are in #168, which is open and currently conflicting with main.

The sixth item from the same list, that a class and its subclass cannot get different policies, is deliberately not filed: dispatch stays keyed on the type default, and #168 only improves the error message.