diff --git a/doc/changelog.rst b/doc/changelog.rst index d45ecf3..891661c 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -1,6 +1,21 @@ Changelog ========= +[Unreleased] +------------ + +Added +^^^^^ +- The storage contract checks that an update or a deletion frees the unique values of a resource. + +Changed +^^^^^^^ +- :class:`~scim2_server.memory.InMemoryStorage` reads, writes and searches faster. A search copies + only the resources of the returned page. The unique values are indexed, and so are the values a + search compares with ``eq``, such as ``externalId eq "…"``. +- :class:`~scim2_server.memory.InMemoryStorage` indexes the values under the policy in effect when + they were written, so it serves the resources of a single policy. + [0.8.2] - 2026-10-23 -------------------- diff --git a/doc/how-to/check-a-storage.rst b/doc/how-to/check-a-storage.rst index a18eac1..29d2280 100644 --- a/doc/how-to/check-a-storage.rst +++ b/doc/how-to/check-a-storage.rst @@ -78,9 +78,9 @@ Run the suite .. code-block:: console $ pytest - ........................................................................ [ 75%] - ........................ [100%] - 96 passed in 6.09s + ........................................................................ [ 66%] + .................................... [100%] + 108 passed in 6.09s Skip what the storage does not support -------------------------------------- @@ -101,7 +101,7 @@ override the ``provider`` fixture. In its :class:`~scim2_models.ServiceProviderC SKIPPED [46] scim2_server/testing.py:216: The service does not support filtering SKIPPED [19] scim2_server/testing.py:216: The service does not support sorting SKIPPED [10] scim2_server/testing.py:216: The service does not support cursor pagination - ======================= 31 passed, 75 skipped in 6.57s ========================= + ======================= 33 passed, 75 skipped in 6.57s ========================= For a storage that cannot search several resource types at once, set :attr:`~scim2_server.testing.ScimStorageContract.supports_root_search` to :data:`False`: @@ -114,7 +114,7 @@ For a storage that cannot search several resource types at once, set $ pytest -rs SKIPPED [2] scim2_server/testing.py:216: The service does not support searching at the root SKIPPED [10] scim2_server/testing.py:216: The service does not support cursor pagination - ======================= 94 passed, 12 skipped in 6.79s ========================= + ======================= 96 passed, 12 skipped in 6.79s ========================= A storage refuses a filter it cannot translate with :class:`~scim2_models.InvalidFilterException`, and a sort with :class:`~scim2_models.InvalidPathException`. The suite skips the filters and the @@ -123,8 +123,8 @@ sorts the storage refuses, and checks the others: .. code-block:: console $ pytest -rs - SKIPPED [11] scim2_server/testing.py:669: The storage refuses the search: Cannot filter on 'title' - SKIPPED [2] scim2_server/testing.py:669: The storage refuses the search: Cannot sort on 'title' + SKIPPED [11] scim2_server/testing.py:700: The storage refuses the search: Cannot filter on 'title' + SKIPPED [2] scim2_server/testing.py:700: The storage refuses the search: Cannot sort on 'title' The filters and the sorts search users with the attributes of the ``User`` resource type and of its enterprise extension. For a storage that does not store some of them, override diff --git a/scim2_server/memory.py b/scim2_server/memory.py index cb85312..f6c212f 100644 --- a/scim2_server/memory.py +++ b/scim2_server/memory.py @@ -22,6 +22,8 @@ from scim2_models import SearchRequest from scim2_models import Uniqueness from scim2_models import UniquenessException +from scim2_models.path import CompareOperator +from scim2_models.path import Comparison from scim2_server.storage import AsyncScimStorage from scim2_server.storage import ScimStorage @@ -142,11 +144,59 @@ def _decode(value: Any) -> Any: raise InvalidCursorException from None +Key = tuple[str | None, str] + + +def _key(resource: Resource[Any]) -> Key: + """Return the resource type and the identifier a resource is stored under. + + Per RFC 7643 §3.1, meta.resourceType holds the name of the resource + type, which may differ from its id. + """ + assert resource.meta is not None + return resource.meta.resource_type, cast(str, resource.id) + + +def _equality(scim_filter: ScimFilter[Any] | None) -> AttributeBinding | None: + """Return the attribute a filter compares to a string with eq, if it holds a single string.""" + if scim_filter is None or scim_filter.model is None: + return None + node = scim_filter.ast + if ( + not isinstance(node, Comparison) + or node.op != CompareOperator.eq + or not isinstance(node.value, str) + ): + return None + binding = scim_filter.resolve_comparison(node.attr_path, strict=False) + if binding is None or binding.is_multivalued or binding.target_type is not str: + return None + return binding + + +def _hashable(value: Any) -> Any: + """Return a form of a value that a dict can hold as a key. + + Lists and models cannot be hashed, so they are held by their + representation. + """ + try: + hash(value) + except TypeError: + return repr(value) + return value + + class InMemoryStorage(ScimStorage): """A storage keeping the resources in memory, for tests, debugging and demos. - It is not optimized for performance: a search walks every resource. A - lock serializes every access, so it can serve a threaded server. + It indexes the resources by identifier and the unique values. A search + walks every resource of the searched types, unless it compares a single + string attribute to a string with ``eq``. A lock serializes every access, + so it can serve a threaded server. + + It indexes the values under the policy in effect when they were written, + so it serves the resources of a single policy. :param clock: Return the date of a write, for ``meta.created`` and ``meta.lastModified``. Pass a fixed clock to get predictable dates. @@ -155,11 +205,36 @@ class InMemoryStorage(ScimStorage): supports_cursors = True def __init__(self, clock: Callable[[], datetime.datetime] = utcnow) -> None: - self.resources: list[Resource[Any]] = [] + self._resources: dict[str | None, dict[str, Resource[Any]]] = {} + self._owners: dict[str, dict[Any, Key]] = {} + self._unique_values: dict[Key, dict[str, Any]] = {} + self._equalities: dict[ + str | None, dict[AttributeBinding, dict[Any, set[str]]] + ] = {} + self._equal_values: dict[Key, dict[AttributeBinding, Any]] = {} self.clock = clock self.lock = RLock() self.versions = itertools.count(1) + @property + def resources(self) -> list[Resource[Any]]: + """The stored resources.""" + return [ + resource + for resources in self._resources.values() + for resource in resources.values() + ] + + @resources.setter + def resources(self, resources: list[Resource[Any]]) -> None: + self._resources = {} + self._owners = {} + self._unique_values = {} + self._equalities = {} + self._equal_values = {} + for resource in resources: + self._store(resource, self._values_of(resource)) + def generate_id(self, resource_type: ResourceType, resource: Resource[Any]) -> str: """Return the identifier of a new resource. @@ -189,9 +264,7 @@ def get( response_parameters: ResponseParameters[Any] | None = None, ) -> Resource[Any]: with self.lock: - return self.resources[self._index(resource_type, resource_id)].model_copy( - deep=True - ) + return self._stored(resource_type, resource_id).model_copy(deep=True) def search( self, @@ -201,19 +274,20 @@ def search( position: Any = None, ) -> SearchPage: with self.lock: - candidates = [ - resource.model_copy(deep=True) - for resource in self.resources - if any(self._is_of_type(resource, rt) for rt in resource_types) - ] + names = list(dict.fromkeys(rt.name for rt in resource_types)) + scim_filter = search_request.filter + candidates = self._candidates(names, scim_filter) - scim_filter = search_request.filter - if scim_filter is not None and not scim_filter.models and candidates: - models = tuple(dict.fromkeys(type(r) for r in candidates)) - scim_filter = parametrize(ScimFilter, Union[models])(str(scim_filter)) # noqa: UP007 + if scim_filter is not None and not scim_filter.models and candidates: + models = tuple(dict.fromkeys(type(r) for r in candidates)) + scim_filter = parametrize(ScimFilter, Union[models])(str(scim_filter)) # noqa: UP007 - found = [r for r in candidates if scim_filter is None or scim_filter.match(r)] - return _page(search_request, position, found) + found = [ + r for r in candidates if scim_filter is None or scim_filter.match(r) + ] + page = _page(search_request, position, found) + page.resources = [r.model_copy(deep=True) for r in page.resources] + return page def create( self, resource_type: ResourceType, resource: Resource[Any] @@ -228,8 +302,9 @@ def create( last_modified=now, version=self.next_version(), ) - self._check_uniqueness(resource) - self.resources.append(resource) + values = self._values_of(resource) + self._check_uniqueness(resource, values) + self._store(resource, values) return resource.model_copy(deep=True) def update( @@ -240,8 +315,7 @@ def update( expected_version: str | None = None, ) -> Resource[Any]: with self.lock: - index = self._index(resource_type, resource.id) - stored = self.resources[index] + stored = self._stored(resource_type, resource.id) self._check_version(stored, expected_version) updated = type(resource).model_validate(resource.model_dump()) @@ -253,8 +327,9 @@ def update( "version": self.next_version(), } ) - self._check_uniqueness(updated) - self.resources[index] = updated + values = self._values_of(updated) + self._check_uniqueness(updated, values) + self._store(updated, values) return updated.model_copy(deep=True) def delete( @@ -265,26 +340,83 @@ def delete( expected_version: str | None = None, ) -> None: with self.lock: - index = self._index(resource_type, resource_id) - self._check_version(self.resources[index], expected_version) - del self.resources[index] + stored = self._stored(resource_type, resource_id) + self._check_version(stored, expected_version) + self._remove(_key(stored)) - def _is_of_type(self, resource: Resource[Any], resource_type: ResourceType) -> bool: - """Tell whether a resource belongs to a resource type. + def _candidates( + self, names: list[str | None], scim_filter: ScimFilter[Any] | None + ) -> list[Resource[Any]]: + """Return the resources a search walks. - Per RFC 7643 §3.1, meta.resourceType holds the name of the resource - type, which may differ from its id. + A search on a single resource type, comparing a single string + attribute to a string with eq, walks only the resources holding an + equal value. The values are indexed on the first such search. """ - assert resource.meta is not None - return resource.meta.resource_type == resource_type.name - - def _index(self, resource_type: ResourceType, resource_id: str | None) -> int: - for index, resource in enumerate(self.resources): - if self._is_of_type(resource, resource_type) and resource.id == resource_id: - return index - raise NotFoundException( - detail=f"{resource_type.name} {resource_id!r} not found" - ) + binding = _equality(scim_filter) if len(names) == 1 else None + if binding is None: + return [ + resource + for name in names + for resource in self._resources.get(name, {}).values() + ] + + node = cast(Comparison, cast(ScimFilter[Any], scim_filter).ast) + try: + value = binding.comparable(node.value) + except ValueError: + return [] + resources = self._resources.get(names[0], {}) + ids = self._equality_index(names[0], binding).get(value, set()) + return [resources[resource_id] for resource_id in ids] + + def _equality_index( + self, name: str | None, binding: AttributeBinding + ) -> dict[Any, set[str]]: + """Return the identifiers of the resources of a type, by the value of an attribute.""" + indexes = self._equalities.setdefault(name, {}) + if binding not in indexes: + indexes[binding] = {} + for resource in self._resources.get(name, {}).values(): + self._index_equality(resource, binding) + return indexes[binding] + + def _index_equality( + self, resource: Resource[Any], binding: AttributeBinding + ) -> None: + key = _key(resource) + value = self._comparable_value(resource, binding) + self._equal_values.setdefault(key, {})[binding] = value + self._equalities[key[0]][binding].setdefault(value, set()).add(key[1]) + + def _store(self, resource: Resource[Any], values: dict[str, Any]) -> None: + """Keep a resource under its resource type and its identifier, and index its values.""" + key = _key(resource) + self._remove(key) + self._resources.setdefault(key[0], {})[key[1]] = resource + self._unique_values[key] = values + for urn, value in values.items(): + self._owners.setdefault(urn, {})[value] = key + for binding in self._equalities.get(key[0], {}): + self._index_equality(resource, binding) + + def _remove(self, key: Key) -> None: + """Forget a resource and its indexed values.""" + self._resources.get(key[0], {}).pop(key[1], None) + for urn, value in self._unique_values.pop(key, {}).items(): + del self._owners[urn][value] + for binding, value in self._equal_values.pop(key, {}).items(): + self._equalities[key[0]][binding][value].discard(key[1]) + + def _stored( + self, resource_type: ResourceType, resource_id: str | None + ) -> Resource[Any]: + try: + return self._resources[resource_type.name][cast(str, resource_id)] + except KeyError: + raise NotFoundException( + detail=f"{resource_type.name} {resource_id!r} not found" + ) from None @staticmethod def _check_version(resource: Resource[Any], expected_version: str | None) -> None: @@ -292,32 +424,42 @@ def _check_version(resource: Resource[Any], expected_version: str | None) -> Non if expected_version is not None and resource.meta.version != expected_version: raise PreconditionFailedException - def _check_uniqueness(self, resource: Resource[Any]) -> None: - """Refuse a resource sharing a unique value with another one of the same schema. + def _check_uniqueness( + self, resource: Resource[Any], values: dict[str, Any] + ) -> None: + """Refuse a resource sharing a unique value with another resource.""" + key = _key(resource) + for urn, value in values.items(): + if self._owners.get(urn, {}).get(value, key) != key: + raise UniquenessException() + + def _values_of(self, resource: Resource[Any]) -> dict[str, Any]: + """Return the unique values of a resource, by the URN of their attribute. Per RFC 7643 erratum 8279, the uniqueness applies to the resources using the schema that declares the attribute, whatever their resource - type. A missing value never clashes, as a SQL NULL does not. + type. The URN of an attribute holds its schema, so two resources clash + when they hold the same value under the same URN. A missing value + never clashes, as a SQL NULL does not. + + The values are compared under the policy of the write that stored + them, so a storage serves the resources of a single policy. """ unique_paths = parametrize(Path, type(resource)).iter_paths( include_subattributes=False, uniqueness=[Uniqueness.server, Uniqueness.global_], ) + values = {} for path in unique_paths: attribute = cast(AttributeBinding, path.resolve()) - value = self._unique_value(resource, attribute) - if value is None: - continue - for existing_resource in self.resources: - if ( - existing_resource.id != resource.id - and self._unique_value(existing_resource, attribute) == value - ): - raise UniquenessException() + value = self._comparable_value(resource, attribute) + if value is not None: + values[attribute.urn] = _hashable(value) + return values @staticmethod - def _unique_value(resource: Resource[Any], attribute: AttributeBinding) -> Any: - """Return the value a resource holds for a unique attribute, in the form it is compared in. + def _comparable_value(resource: Resource[Any], attribute: AttributeBinding) -> Any: + """Return the value a resource holds for an attribute, in the form it is compared in. A resource whose schemas do not declare the attribute holds no value. A string the policy cannot compare is equal to no other value, so it diff --git a/scim2_server/testing.py b/scim2_server/testing.py index 46e17ce..66af00c 100644 --- a/scim2_server/testing.py +++ b/scim2_server/testing.py @@ -501,6 +501,25 @@ def test_update_with_a_taken_unique_value( storage.update(user_type, user) assert storage.get(user_type, user.id).user_name == "jsmith" + def test_update_frees_the_former_unique_values( + self, storage: Any, user_type: ResourceType, user_model: Any + ) -> None: + """A userName a resource gave up can be taken by another resource.""" + (user,) = self.create_users(storage, user_type, user_model, "bjensen") + user.user_name = "babs" + storage.update(user_type, user) + + self.create_users(storage, user_type, user_model, "bjensen") + + def test_delete_frees_the_unique_values( + self, storage: Any, user_type: ResourceType, user_model: Any + ) -> None: + """The userName of a deleted resource can be taken by a new resource.""" + (user,) = self.create_users(storage, user_type, user_model, "bjensen") + storage.delete(user_type, user.id) + + self.create_users(storage, user_type, user_model, "bjensen") + def test_search_a_resource_type( self, storage: Any, diff --git a/tests/test_memory.py b/tests/test_memory.py index 7e8d27d..bb97e9a 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -7,10 +7,12 @@ from scim2_models import URN from scim2_models import Attribute from scim2_models import CaseExact +from scim2_models import ComplexAttribute from scim2_models import Extension from scim2_models import Filter from scim2_models import InvalidFilterException from scim2_models import InvalidPathException +from scim2_models import NotFoundException from scim2_models import Resource from scim2_models import ResourceType from scim2_models import Schema @@ -207,6 +209,87 @@ def test_versions_differ_within_the_same_instant(user_type, scim_provider): assert updated.meta.version != user.meta.version +def found_user_names(storage, user_type, User, scim_filter): + page = storage.search([user_type], SearchRequest[User](filter=scim_filter)) + return sorted(resource.user_name for resource in page.resources) + + +def test_the_resources_can_be_replaced(user_type, scim_provider): + """Resources assigned to a storage can be read and searched, and the former ones are gone.""" + storage = InMemoryStorage() + User = scim_provider.model_for(user_type) + former = storage.create(user_type, User(user_name="bjensen")) + kept = InMemoryStorage().create(user_type, User(user_name="jsmith")) + assert found_user_names(storage, user_type, User, 'userName eq "bjensen"') == [ + "bjensen" + ] + + storage.resources = [kept] + + assert storage.get(user_type, kept.id).user_name == "jsmith" + with pytest.raises(NotFoundException): + storage.get(user_type, former.id) + assert found_user_names(storage, user_type, User, 'userName eq "bjensen"') == [] + assert found_user_names(storage, user_type, User, 'userName eq "jsmith"') == [ + "jsmith" + ] + storage.create(user_type, User(user_name="bjensen")) + with pytest.raises(UniquenessException): + storage.create(user_type, User(user_name="jsmith")) + + +def test_an_equality_search_follows_the_writes(user_type, scim_provider): + """A search for an equal value finds the resources as they are after each write.""" + storage = InMemoryStorage() + User = scim_provider.model_for(user_type) + user = storage.create(user_type, User(user_name="bjensen", external_id="a")) + assert found_user_names(storage, user_type, User, 'externalId eq "a"') == [ + "bjensen" + ] + + user.external_id = "b" + storage.update(user_type, user) + assert found_user_names(storage, user_type, User, 'externalId eq "a"') == [] + assert found_user_names(storage, user_type, User, 'externalId eq "b"') == [ + "bjensen" + ] + + storage.create(user_type, User(user_name="jsmith", external_id="b")) + assert found_user_names(storage, user_type, User, 'externalId eq "b"') == [ + "bjensen", + "jsmith", + ] + + storage.delete(user_type, user.id) + assert found_user_names(storage, user_type, User, 'externalId eq "b"') == ["jsmith"] + + +def test_an_equality_search_follows_the_case_exactness(user_type, scim_provider): + """A search for an equal value ignores the case unless the attribute is case-exact.""" + storage = InMemoryStorage() + User = scim_provider.model_for(user_type) + storage.create(user_type, User(user_name="bjensen", external_id="a")) + + assert found_user_names(storage, user_type, User, 'userName eq "BJensen"') == [ + "bjensen" + ] + assert found_user_names(storage, user_type, User, 'externalId eq "A"') == [] + + +def test_an_equality_search_for_a_value_the_policy_cannot_compare( + user_type, scim_provider +): + """A search for a value the policy refuses finds nothing.""" + storage = InMemoryStorage() + User = scim_provider.model_for(user_type) + storage.create(user_type, User(user_name="bjensen!")) + + with ScimPolicy(comparison_key=refuse_exclamation_marks): + assert ( + found_user_names(storage, user_type, User, 'userName eq "bjensen!"') == [] + ) + + def test_operations_can_be_nested(user_type, scim_provider): """The lock of an operation can be taken again inside it.""" storage = InMemoryStorage() @@ -362,6 +445,27 @@ class Token(Resource): ) +def test_multi_valued_and_complex_unique_values_clash_when_equal(): + """Equal lists, or equal complex values, of a unique attribute clash.""" + + class Size(ComplexAttribute): + width: int | None = None + + class Badge(Resource): + __schema__ = URN("urn:example:2.0:Badge") + codes: Annotated[list[str] | None, Uniqueness.server] = None + size: Annotated[Size | None, Uniqueness.server] = None + + storage = InMemoryStorage() + badge_type = ResourceType.from_resource(Badge) + storage.create(badge_type, Badge(codes=["a", "b"], size=Size(width=1))) + storage.create(badge_type, Badge(codes=["a"], size=Size(width=2))) + with pytest.raises(UniquenessException): + storage.create(badge_type, Badge(codes=["a", "b"])) + with pytest.raises(UniquenessException): + storage.create(badge_type, Badge(size=Size(width=2))) + + @pytest.mark.parametrize("root", [False, True]) def test_a_filter_bound_to_no_model_is_resolved_against_the_stored_resources( user_type, scim_provider, root