From f5830d6f9c4ea6e79f217fc80eb7639da2749bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89loi=20Rivard?= Date: Sat, 10 Oct 2026 20:14:24 +0200 Subject: [PATCH] feat: link an attribute to several resource types An attribute can map to a list of Links, one per resource type, so a group can hold users and groups. A Link can give its entries a type, which clients may send and filters compare. --- doc/changelog.rst | 3 +- doc/how-to/map-a-data-model.rst | 30 ++++ doc/reference/errors.rst | 3 +- doc/reference/searches.rst | 2 +- src/scim2_sqlalchemy/conversion.py | 34 +++-- src/scim2_sqlalchemy/mapping.py | 189 ++++++++++++++++++------- src/scim2_sqlalchemy/query.py | 97 ++++++++++--- src/scim2_sqlalchemy/storage.py | 118 +++++++++++++--- tests/models.py | 19 ++- tests/test_link.py | 216 +++++++++++++++++++++++++++++ tests/test_mapping.py | 48 ++++++- tests/test_storage.py | 11 -- 12 files changed, 647 insertions(+), 123 deletions(-) create mode 100644 tests/test_link.py diff --git a/doc/changelog.rst b/doc/changelog.rst index ee3c3bb..a5bd5c7 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -10,7 +10,8 @@ Added records of a SQLAlchemy model, with :class:`~scim2_sqlalchemy.Attribute`, :class:`~scim2_sqlalchemy.Many` and :class:`~scim2_sqlalchemy.Link`. A :class:`~scim2_sqlalchemy.Link` links to one resource, such as a ``manager``, or to - several, such as ``members``. + several, such as ``members``. A list of Links links to several resource types, such as the + users and the groups of a group. A Link can give its entries a ``type``. - :class:`~scim2_sqlalchemy.SqlAlchemyStorage` and :class:`~scim2_sqlalchemy.AsyncSqlAlchemyStorage`, which serve the mappings to :doc:`scim2-server `. They filter, sort and page the resources in the diff --git a/doc/how-to/map-a-data-model.rst b/doc/how-to/map-a-data-model.rst index 16c604f..a623473 100644 --- a/doc/how-to/map-a-data-model.rst +++ b/doc/how-to/map-a-data-model.rst @@ -198,6 +198,36 @@ without ``value``, raises a 400 error. The linked resources are never created or A storage serves the resource type of every :class:`~scim2_sqlalchemy.Link`. It raises a :class:`ValueError` otherwise. +Link to several resource types +------------------------------ + +Map an attribute whose entries are resources of several types, such as the ``members`` of a +group holding users and groups, to a list of :class:`~scim2_sqlalchemy.Link`, one per resource +type: + +.. code-block:: python + + { + "members": [ + Link(Team.members, "User", type="User"), + Link(Team.subteams, "Group", type="Group"), + ], + } + +The storage reads the entries of each Link in the order of the list. For a single-valued +attribute, it reads the first Link holding a record. It writes each entry in the Link holding +the resource of its ``value``: per :rfc:`RFC 7643 ยง3.1 <7643#section-3.1>`, two resources never +share an ``id``. The storage reads the direct members only, so a group may hold itself. + +Give each Link a ``type`` to fill the ``type`` sub-attribute of its entries. A client may send +``type``: the storage compares it as filters compare strings, and refuses a type other than the +one of the Link with a 400 error. Filters such as ``members[type eq "Group"]`` compare it too. +Give a ``type`` to every Link of an attribute, or to none: without it, the storage does not +store the ``type`` sub-attribute. + +A Link to one resource type takes a ``type`` too. ``Link(Account.teams, "Group", +type="direct")`` gives the groups of a user the type ``direct``. + Map a custom resource type -------------------------- diff --git a/doc/reference/errors.rst b/doc/reference/errors.rst index 5962acd..77d93fd 100644 --- a/doc/reference/errors.rst +++ b/doc/reference/errors.rst @@ -18,7 +18,8 @@ into error responses: - Another resource holds a unique value of the resource, such as its ``userName``. * - :class:`~scim2_models.InvalidValueException` - The resource holds a value no mapping stores, an entry of a - :class:`~scim2_sqlalchemy.Link` without ``value``, or a link to an unknown resource. + :class:`~scim2_sqlalchemy.Link` without ``value``, a link to an unknown resource, or a + ``type`` other than the one of its :class:`~scim2_sqlalchemy.Link`. * - :class:`~scim2_models.MutabilityException` - The resource changes an attribute the mapping only reads, or removes a value whose column cannot be ``NULL``. diff --git a/doc/reference/searches.rst b/doc/reference/searches.rst index 4e7ca95..5bcca94 100644 --- a/doc/reference/searches.rst +++ b/doc/reference/searches.rst @@ -15,7 +15,7 @@ what a search supports: - ``and``, ``or``, ``not``, and value selections such as ``emails[type eq "work"]``. * - Filtered attributes - Columns, :class:`hybrid properties `, the entries of a :class:`~scim2_sqlalchemy.Many`, and the - ``value`` and the display name of a :class:`~scim2_sqlalchemy.Link`. + ``value``, the display name and the ``type`` of a :class:`~scim2_sqlalchemy.Link`. * - Sorted attributes - Columns, hybrid properties, and the entries of a :class:`~scim2_sqlalchemy.Many`, on the ``primary`` entry or else the first one. diff --git a/src/scim2_sqlalchemy/conversion.py b/src/scim2_sqlalchemy/conversion.py index 20b96e1..cd06c10 100644 --- a/src/scim2_sqlalchemy/conversion.py +++ b/src/scim2_sqlalchemy/conversion.py @@ -79,17 +79,19 @@ def _to_scim( if value is not None: Path[model](column.binding.urn).set(resource, value) # type: ignore[valid-type] + read: dict[str, tuple[bool, list[dict[str, Any]]]] = {} for collection in collections: endpoint = None if Path[model](f"{collection.binding.urn}.$ref").resolve() is not None: # type: ignore[valid-type] endpoint = endpoints.get(collection.resource_type) # type: ignore[arg-type] - entries = [ + _, entries = read.setdefault(collection.binding.urn, (collection.single, [])) + entries.extend( _read_entry(collection, entry, endpoint) for entry in _related(collection, record) - ] + ) + for urn, (single, entries) in read.items(): if entries: - value = entries[0] if collection.single else entries - Path[model](collection.binding.urn).set(resource, value) # type: ignore[valid-type] + Path[model](urn).set(resource, entries[0] if single else entries) # type: ignore[valid-type] resource.id = str(_read(mapping._id, record)) resource.meta = Meta( @@ -132,6 +134,8 @@ def _read_entry( if endpoint is not None: values["$ref"] = f"{endpoint}/{value}" values[column.binding.urn.rsplit(".", 1)[-1]] = value + if collection.link_type is not None: + values["type"] = collection.link_type return values @@ -158,7 +162,7 @@ def _from_scim( continue setattr(record, column.key, None if value is None else _as_stored(value)) - for collection in mapping._collections.values(): + for collection in mapping._all_collections(): if not collection.writable: continue if collection.owned: @@ -182,16 +186,17 @@ def _write_entry(collection: _Collection, entry: Any) -> Any: return record -def _link_ids( +def _link_entries( mapping: ResourceMapping, resource: Resource[Any] -) -> dict[_Collection, list[str]]: - """Return the identifiers each writable Link of a resource links to. +) -> list[tuple[list[_Collection], list[Any]]]: + """Return the entries of each written attribute of a resource linking to other resources, with the collections of its Links. A link is stored by its value, so an entry without one, such as an entry with a $ref only, raises InvalidValueException rather than being lost. """ - links = {} - for collection in mapping._collections.values(): + links = [] + for collections in mapping._collections.values(): + collection = collections[0] if collection.owned or not collection.writable: continue entries = _entries(collection, resource) @@ -199,7 +204,7 @@ def _link_ids( raise InvalidValueException( detail=f"'{collection.binding.urn}' holds an entry without value" ) - links[collection] = [entry.value for entry in entries] + links.append((collections, entries)) return links @@ -247,7 +252,10 @@ def _check_storable( detail=f"'{binding.urn}' cannot be removed on this server", ) continue - if storage in (_Storage.derived, _Storage.ref) or value in (None, current): + if storage in (_Storage.derived, _Storage.ref, _Storage.link_type) or value in ( + None, + current, + ): continue if storage == _Storage.read_only: raise MutabilityException( @@ -264,7 +272,7 @@ def _check_enumerated( mapping: ResourceMapping, binding: AttributeBinding, value: Any ) -> None: """Refuse a string that an Enum column does not store, rather than failing in the database.""" - _, column = mapping._lookup(binding) + _, column = mapping._lookup(binding)[0] if column is None or column.enumeration is None: return stored = column.enumeration.enums diff --git a/src/scim2_sqlalchemy/mapping.py b/src/scim2_sqlalchemy/mapping.py index 257209c..f90cfe5 100644 --- a/src/scim2_sqlalchemy/mapping.py +++ b/src/scim2_sqlalchemy/mapping.py @@ -21,6 +21,7 @@ from sqlalchemy import String from sqlalchemy import cast from sqlalchemy import inspect +from sqlalchemy import literal from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.orm import InstrumentedAttribute from sqlalchemy.orm import Mapper @@ -34,13 +35,15 @@ class _Storage(Enum): A derived attribute is read from another resource, such as the display of a member, so writing it changes nothing. A ref is the $ref of a link, built from its value: clients may send it, and writing it changes nothing - either. + either. A link type is the type a Link gives its entries: clients may send + it, and the storage checks it against the linked resource. """ written = "written" read_only = "read only" derived = "derived" ref = "ref" + link_type = "link type" def _holds_text(expression: Any) -> bool: @@ -108,15 +111,27 @@ class Link: Writing the resource changes the links, and never creates nor deletes the linked resources. + A Link links to the resources of one resource type. An attribute linking + to several resource types, such as the ``members`` of a group holding users + and groups, maps to a list of Links, one per resource type. The entries are + read in the order of the list. + :param relationship: The :func:`~sqlalchemy.orm.relationship` from the resource record to the records of the linked resources. :param resource_type: The name of the resource type of the linked resources, as the storage knows it. + :param type: The ``type`` sub-attribute of the entries, such as ``"User"`` + for the members of a group, or ``"direct"`` for the groups of a user. + A client may send it, and a write with another value is refused. + Without it, the storage does not store ``type``. """ - def __init__(self, relationship: Any, resource_type: str) -> None: + def __init__( + self, relationship: Any, resource_type: str, *, type: str | None = None + ) -> None: self.relationship = relationship self.resource_type = resource_type + self.type = type @dataclass(eq=False) @@ -187,6 +202,9 @@ class _Collection: The entries of an owned collection belong to the resource. The others are links to the resources of another mapping, the target. A single collection holds one entry at most, such as the manager of a user. + + link_type is the type a Link gives its entries, and type_column the + constant filters compare it as. """ binding: AttributeBinding @@ -194,6 +212,8 @@ class _Collection: columns: dict[str, _Column] = field(default_factory=dict) resource_type: str | None = None target: "ResourceMapping | None" = None + link_type: str | None = None + type_column: _Column | None = None @property def owned(self) -> bool: @@ -211,6 +231,14 @@ def relationship_property(self) -> RelationshipProperty[Any]: def single(self) -> bool: return not self.binding.is_multivalued + def column(self, sub_field_name: str | None) -> _Column | None: + """Return the column of a sub-attribute, or None for the collection named alone.""" + if sub_field_name is None: + return None + if sub_field_name == "type" and self.type_column is not None: + return self.type_column + return self.columns.get(sub_field_name) + def exists(self, condition: Any = None) -> ColumnElement[bool]: """Return the condition that an entry exists, and matches condition when given.""" method = self.relationship.has if self.single else self.relationship.any @@ -235,8 +263,8 @@ class ResourceMapping: ``User[EnterpriseUser]``. :param record: The SQLAlchemy model of the records. :param attributes: The attributes, by SCIM path. A value is a column, a - :class:`~sqlalchemy.ext.hybrid.hybrid_property`, an :class:`Attribute`, a :class:`Many` or a - :class:`Link`. ``id``, ``meta.created`` and ``meta.lastModified`` + :class:`~sqlalchemy.ext.hybrid.hybrid_property`, an :class:`Attribute`, a :class:`Many`, a + :class:`Link`, or a list of :class:`Link`. ``id``, ``meta.created`` and ``meta.lastModified`` are required. :param version: The ``version_id_col`` of the mapper of ``record``, which gives ``meta.version`` (:ref:`sqlalchemy:mapper_version_counter`). @@ -257,7 +285,7 @@ def __init__( self.model = model self.record = record self._columns: dict[tuple[type[BaseModel], str, str | None], _Column] = {} - self._collections: dict[tuple[type[BaseModel], str], _Collection] = {} + self._collections: dict[tuple[type[BaseModel], str], list[_Collection]] = {} for required in self._REQUIRED: if required not in attributes: @@ -334,11 +362,11 @@ def _restrict_attribute(self, urn: str, attribute: Any) -> Any: storage = self._storage(urn) if storage is None: return None - if storage == _Storage.written: + if storage in (_Storage.written, _Storage.link_type): return attribute if storage == _Storage.ref: - linked = self._linked_resource_type(urn) - return attribute.model_copy(update={"reference_types": [linked]}) + linked = self._linked_resource_types(urn) + return attribute.model_copy(update={"reference_types": linked}) return attribute.model_copy(update={"mutability": Mutability.read_only}) def _storage(self, urn: str) -> _Storage | None: @@ -347,7 +375,7 @@ def _storage(self, urn: str) -> _Storage | None: for column in self._columns.values(): if column.binding.urn.lower() == key: return _Storage.written if column.writable else _Storage.read_only - for collection in self._collections.values(): + for collection in self._all_collections(): if not collection.owned: head = collection.binding.urn.lower() if key == f"{head}.value": @@ -356,6 +384,8 @@ def _storage(self, urn: str) -> _Storage | None: return _Storage.derived if key == f"{head}.$ref": return _Storage.ref + if key == f"{head}.type" and collection.link_type is not None: + return _Storage.link_type continue for column in collection.columns.values(): if column.binding.urn.lower() == key: @@ -371,14 +401,20 @@ def _nullable(self, urn: str) -> bool: if column.binding.urn.lower() == key ) - def _linked_resource_type(self, urn: str) -> str | None: - """Return the resource type a Link links to, from the URN of its $ref.""" + def _linked_resource_types(self, urn: str) -> list[str]: + """Return the resource types the Links of an attribute link to, from the URN of its $ref.""" head = urn.lower().removesuffix(".$ref") - return next( + return [ collection.resource_type - for collection in self._collections.values() + for collection in self._all_collections() if collection.binding.urn.lower() == head - ) + and collection.resource_type is not None + ] + + def _all_collections(self) -> Iterator[_Collection]: + """Yield the collections of every attribute, each Link of an attribute being a collection.""" + for collections in self._collections.values(): + yield from collections def _column(self, path: str) -> _Column: binding = self._resolve(path) @@ -388,19 +424,22 @@ def _column(self, path: str) -> _Column: def _lookup( self, binding: AttributeBinding - ) -> tuple[_Collection | None, _Column | None]: - """Return the collection holding an attribute, if any, and the column of the attribute. + ) -> list[tuple[_Collection | None, _Column | None]]: + """Return each collection holding an attribute, if any, with the column of the attribute in it. - The column is None for an attribute stored nowhere, and for a - collection named alone, such as emails. + An attribute outside collections gives one pair, without collection. + An attribute linking to several resource types gives a pair for each + of its Links. The column is None for an attribute stored nowhere, and + for a collection named alone, such as emails. """ - collection = self._collections.get((binding.model, binding.field_name)) - if collection is not None: - if binding.sub_field_name is None: - return collection, None - return collection, collection.columns.get(binding.sub_field_name) - key = (binding.model, binding.field_name, binding.sub_field_name) - return None, self._columns.get(key) + collections = self._collections.get((binding.model, binding.field_name)) + if collections is None: + key = (binding.model, binding.field_name, binding.sub_field_name) + return [(None, self._columns.get(key))] + return [ + (collection, collection.column(binding.sub_field_name)) + for collection in collections + ] def _sub_columns(self, binding: AttributeBinding) -> list[_Column]: """Return the mapped sub-attributes of a complex attribute holding a single value.""" @@ -417,10 +456,12 @@ def _string_columns(self) -> Iterator[_Column]: for column in self._columns.values(): if column.is_string: yield column - for collection in self._collections.values(): + for collection in self._all_collections(): for column in collection.columns.values(): if column.is_string: yield column + if collection.type_column is not None: + yield collection.type_column def _unique_columns(self) -> Iterator[_Column]: """Yield the readable columns whose values no two resources share.""" @@ -435,7 +476,7 @@ def _returned_collections( self, parameters: ResponseParameters[Any] | None = None ) -> list[_Collection]: """Return the collections a response keeps, all of them without parameters.""" - collections = list(self._collections.values()) + collections = list(self._all_collections()) if parameters is None: return collections paths = Path[self.model].iter_paths( # type: ignore[name-defined] @@ -467,7 +508,7 @@ def _bind(self, mappings: Mapping[str, "ResourceMapping"]) -> None: Several storages may share a mapping, as long as they link it to the same mappings. """ - for collection in self._collections.values(): + for collection in self._all_collections(): if collection.owned: continue target = mappings.get(collection.resource_type) # type: ignore[arg-type] @@ -508,7 +549,7 @@ def _display(self) -> _Column | None: binding = self._find(path) if binding is None: continue - _, column = self._lookup(binding) + ((_, column),) = self._lookup(binding) if column is not None and column.readable: return column return None @@ -527,7 +568,11 @@ def _add(self, path: str, value: Any) -> None: target = binding.target_type is_complex = isclass(target) and issubclass(target, BaseModel) - if isinstance(value, Many | Link): + if isinstance(value, Link): + value = [value] + if isinstance(value, Many) or ( + isinstance(value, list) and all(isinstance(item, Link) for item in value) + ): if not (is_complex and binding.sub_field_name is None) or ( isinstance(value, Many) and not binding.is_multivalued ): @@ -537,27 +582,11 @@ def _add(self, path: str, value: Any) -> None: raise ValueError( f"{path!r} is not {kind} attribute of {self.model.__name__}" ) - collection = _Collection(binding, value.relationship) - if collection.relationship_property.uselist == collection.single: - raise ValueError( - f"{path!r} holds {'one entry' if collection.single else 'several entries'}, " - f"but its relationship holds {'several' if collection.single else 'one'}" - ) if isinstance(value, Many): - if not collection.relationship_property.cascade.delete_orphan: - raise ValueError( - f"The relationship of {path!r} needs the delete-orphan cascade" - ) - for sub_path, expression in value.attributes.items(): - sub_binding = self._resolve(f"{path}.{sub_path}") - collection.columns[sub_binding.sub_field_name] = ( # type: ignore[index] - self._build_column( - f"{path}.{sub_path}", sub_binding, expression - ) - ) + collections = [self._many(path, binding, value)] else: - collection.resource_type = value.resource_type - self._collections[(binding.model, binding.field_name)] = collection + collections = self._links(path, binding, value) + self._collections[(binding.model, binding.field_name)] = collections return if binding.is_multivalued or is_complex: @@ -569,6 +598,68 @@ def _add(self, path: str, value: Any) -> None: self._build_column(path, binding, value) ) + def _collection( + self, path: str, binding: AttributeBinding, relationship: Any + ) -> _Collection: + """Build the collection of a relationship, holding as many records as the attribute holds entries.""" + collection = _Collection(binding, relationship) + if collection.relationship_property.uselist == collection.single: + raise ValueError( + f"{path!r} holds {'one entry' if collection.single else 'several entries'}, " + f"but its relationship holds {'several' if collection.single else 'one'}" + ) + return collection + + def _many(self, path: str, binding: AttributeBinding, many: Many) -> _Collection: + """Build the collection of a Many, with the columns of its entries.""" + collection = self._collection(path, binding, many.relationship) + if not collection.relationship_property.cascade.delete_orphan: + raise ValueError( + f"The relationship of {path!r} needs the delete-orphan cascade" + ) + for sub_path, expression in many.attributes.items(): + sub_binding = self._resolve(f"{path}.{sub_path}") + collection.columns[sub_binding.sub_field_name] = self._build_column( # type: ignore[index] + f"{path}.{sub_path}", sub_binding, expression + ) + return collection + + def _links( + self, path: str, binding: AttributeBinding, links: list[Link] + ) -> list[_Collection]: + """Build the collections of the Links of an attribute, one per resource type. + + The type of the entries is compared, in filters, as a constant of each + Link. + """ + resource_types = [link.resource_type for link in links] + if not links or len(set(resource_types)) != len(resource_types): + raise ValueError( + f"{path!r} needs one Link per resource type, and at least one" + ) + typed = {link.type is not None for link in links} + if len(typed) > 1: + raise ValueError(f"Either every Link of {path!r} has a type, or none") + type_binding = self._find(f"{path}.type") + if typed == {True} and type_binding is None: + raise ValueError(f"{path!r} has no type sub-attribute") + + collections = [] + for link in links: + collection = self._collection(path, binding, link.relationship) + collection.resource_type = link.resource_type + if link.type is not None: + collection.link_type = link.type + collection.type_column = _Column( + type_binding, # type: ignore[arg-type] + literal(link.type), + "type", + True, + False, + ) + collections.append(collection) + return collections + def _build_column( self, path: str, binding: AttributeBinding, value: Any ) -> _Column: diff --git a/src/scim2_sqlalchemy/query.py b/src/scim2_sqlalchemy/query.py index 6b28ce7..cb4a273 100644 --- a/src/scim2_sqlalchemy/query.py +++ b/src/scim2_sqlalchemy/query.py @@ -113,11 +113,30 @@ def _path(self, attr_path: AttrPath) -> AttrPath: scope_path, _ = self.scope return AttrPath(scope_path.attr, attr_path.attr, scope_path.uri) - def _target(self, binding: AttributeBinding) -> tuple[_Collection | None, _Column]: - collection, column = self.mapping._lookup(binding) - if column is None or not column.readable: + def _lookup( + self, binding: AttributeBinding + ) -> list[tuple[_Collection | None, _Column | None]]: + """Return the collections holding an attribute, with its column in each, as the mapping does. + + Within a value selection, only the collection of the selection holds + the attribute. + """ + pairs = self.mapping._lookup(binding) + if self.scope is None: + return pairs + _, scope = self.scope + return [ + (collection, column) for collection, column in pairs if collection is scope + ] + + def _targets( + self, binding: AttributeBinding + ) -> list[tuple[_Collection | None, _Column]]: + """Return the collections holding an attribute with its column in each, refusing an attribute that one of them does not let filters read.""" + pairs = self._lookup(binding) + if any(column is None or not column.readable for _, column in pairs): raise _unmapped(binding) - return collection, column + return pairs # type: ignore[return-value] def _in_collection( self, collection: _Collection | None, condition: ColumnElement[bool] @@ -132,35 +151,55 @@ def visit_comparison(self, node: Comparison) -> ColumnElement[bool]: ) if binding is None: return false() - collection, column = self._target(binding) + targets = self._targets(binding) value = coerce_value(binding, node.value, node.op) # 'emails ne "x"' holds when no email is "x", while # 'emails[value ne "x"]' holds when one email is not "x". if ( node.op == CompareOperator.ne - and collection is not None + and targets[0][0] is not None and self.scope is None ): - equal = _compare(self.comparator, column, CompareOperator.eq, value) - return not_(collection.exists(equal)) - condition = _compare(self.comparator, column, node.op, value) - return self._in_collection(collection, condition) + return not_( + or_( + *( + collection.exists( # type: ignore[union-attr] + _compare(self.comparator, column, CompareOperator.eq, value) + ) + for collection, column in targets + ) + ) + ) + return or_( + *( + self._in_collection( + collection, _compare(self.comparator, column, node.op, value) + ) + for collection, column in targets + ) + ) def visit_present(self, node: Present) -> ColumnElement[bool]: binding = self.filter.resolve(self._path(node.attr_path), strict=False) if binding is None: return false() - collection, column = self.mapping._lookup(binding) - if collection is not None and binding.sub_field_name is None: - return collection.exists() if self.scope is None else true() - if column is not None and column.readable: - return self._in_collection(collection, _present(column)) + pairs = self._lookup(binding) + in_collection = pairs[0][0] is not None + if in_collection and binding.sub_field_name is None: + return or_(*(collection.exists() for collection, _ in pairs)) # type: ignore[union-attr] + if all(column is not None and column.readable for _, column in pairs): + return or_( + *( + self._in_collection(collection, _present(column)) # type: ignore[arg-type] + for collection, column in pairs + ) + ) sub_columns = [ column for column in self.mapping._sub_columns(binding) if column.readable ] - if collection is not None or not sub_columns: + if in_collection or not sub_columns: raise _unmapped(binding) return or_(*(_present(column) for column in sub_columns)) @@ -175,13 +214,26 @@ def visit_value_path(self, node: ValuePath) -> ColumnElement[bool]: binding = self.filter.resolve(node.attr_path, strict=False) if binding is None: return false() - collection, _ = self.mapping._lookup(binding) - if collection is None: + collections = [ + collection + for collection, _ in self.mapping._lookup(binding) + if collection is not None + ] + if not collections: raise _unmapped(binding) - scoped = _FilterTranslator( - self.mapping, self.filter, self.comparator, (node.attr_path, collection) + return or_( + *( + collection.exists( + _FilterTranslator( + self.mapping, + self.filter, + self.comparator, + (node.attr_path, collection), + ).visit(node.val_filter) + ) + for collection in collections + ) ) - return collection.exists(scoped.visit(node.val_filter)) def _present(column: _Column) -> ColumnElement[bool]: @@ -255,7 +307,7 @@ def _sort_key( binding = search_request.sort_binding(mapping.model) if binding is None: return None - collection, column = mapping._lookup(binding) + collection, column = mapping._lookup(binding)[0] if ( column is None or not column.readable @@ -442,7 +494,6 @@ def _load_statement( select(mapping.record) .where(mapping._id.expression == key) .options(*mapping._loader_options(mapping._returned_collections(parameters))) - .execution_options(populate_existing=True) ) diff --git a/src/scim2_sqlalchemy/storage.py b/src/scim2_sqlalchemy/storage.py index c90c472..5d7e0a3 100644 --- a/src/scim2_sqlalchemy/storage.py +++ b/src/scim2_sqlalchemy/storage.py @@ -34,7 +34,7 @@ from .comparison import _comparator from .conversion import _check_storable from .conversion import _from_scim -from .conversion import _link_ids +from .conversion import _link_entries from .conversion import _to_scim from .conversion import _version from .keyset import _Anchor @@ -42,6 +42,7 @@ from .keyset import _position from .mapping import ResourceMapping from .mapping import _Collection +from .query import _as_key from .query import _links_statement from .query import _load_statement from .query import _records_statement @@ -54,6 +55,28 @@ def _utcnow() -> datetime.datetime: return datetime.datetime.now(datetime.UTC) +def _expire( + session: Session | AsyncSession, mapping: ResourceMapping, resource_id: str | None +) -> None: + """Expire the record of an identifier when the session holds it, so that loading it reads the database again. + + Only this record is read again: a record linking to itself, such as a + group holding itself, is then loaded once, with all its collections. + """ + key = _as_key(mapping._id, resource_id) + if key is None: + return + record = session.identity_map.get(session.identity_key(mapping.record, key)) + if record is not None: + session.expire(record) + + +def _holds_id(collection: _Collection, record: Any, value: str) -> bool: + """Tell whether a record a Link links to has an identifier.""" + target_id = collection.target._id # type: ignore[union-attr] + return bool(getattr(record, target_id.key) == _as_key(target_id, value)) + + def _by_position(rows: Sequence[Any]) -> dict[int, list[str]]: """Group the identifiers of a page at the root by position of their resource type.""" ids: dict[int, list[str]] = {} @@ -265,14 +288,57 @@ def _unique_statements( if value is not None: yield _taken_statement(mapping, column, value, resource.id, comparator) - @staticmethod - def _check_links( - collection: _Collection, ids: list[str], records: list[Any] - ) -> None: - if len(records) != len(set(ids)): - raise InvalidValueException( - detail=f"'{collection.binding.urn}' links to an unknown resource" + def _assign_links( + self, found: list[tuple[_Collection, list[Any]]], entries: list[Any] + ) -> dict[_Collection, list[Any]]: + """Give each entry of an attribute the record it links to, in the collection of the first Link holding that record. + + found holds the records each Link of the attribute holds among the + values of the entries. An entry with a type links to a Link giving + the same type. + """ + links: dict[_Collection, list[Any]] = { + collection: [] for collection, _ in found + } + urn = found[0][0].binding.urn + for entry in entries: + matches = [ + (collection, record) + for collection, records in found + for record in records + if _holds_id(collection, record, entry.value) + ] + if not matches: + raise InvalidValueException( + detail=f"'{urn}' links to an unknown resource" + ) + entry_type = getattr(entry, "type", None) + if entry_type is not None: + matches = [ + (collection, record) + for collection, record in matches + if self._same_type(collection, entry_type) + ] + if not matches: + raise InvalidValueException( + detail=f"'{urn}' links to {entry.value!r}, which is not " + f"of type {entry_type!r}" + ) + collection, record = matches[0] + if record not in links[collection]: + links[collection].append(record) + return links + + def _same_type(self, collection: _Collection, value: str) -> bool: + """Tell whether a type is the one a Link gives, compared as filters compare it.""" + binding = collection.type_column.binding # type: ignore[union-attr] + try: + return bool( + binding.comparable(value, self._policy) + == binding.comparable(collection.link_type, self._policy) ) + except ValueError: + return False def _new_record(self, resource_type: ResourceType, resource: Resource[Any]) -> Any: """Build an empty record for a new resource, with its dates. @@ -451,6 +517,7 @@ def _load( """Load a record, with the collections the response keeps.""" mapping = self._mapping(resource_type) statement = _load_statement(mapping, resource_id, parameters) + _expire(self.session(), mapping, resource_id) record = None if statement is None else self.session().scalar(statement) if record is None: raise self._not_found(resource_type, resource_id) @@ -475,12 +542,19 @@ def _ready_comparator(self, session: Session) -> _Comparator: def _links( self, mapping: ResourceMapping, resource: Resource[Any] ) -> dict[_Collection, list[Any]]: - links = {} - for collection, ids in _link_ids(mapping, resource).items(): - statement = _links_statement(collection, ids) - records = list(self.session().scalars(statement).all()) - self._check_links(collection, ids, records) - links[collection] = records + links: dict[_Collection, list[Any]] = {} + for collections, entries in _link_entries(mapping, resource): + ids = [entry.value for entry in entries] + found = [ + ( + collection, + list( + self.session().scalars(_links_statement(collection, ids)).all() + ), + ) + for collection in collections + ] + links |= self._assign_links(found, entries) return links def _flush( @@ -647,6 +721,7 @@ async def _load( """Load a record, with the collections the response keeps.""" mapping = self._mapping(resource_type) statement = _load_statement(mapping, resource_id, parameters) + _expire(self.session(), mapping, resource_id) record = None if statement is None else await self.session().scalar(statement) if record is None: raise self._not_found(resource_type, resource_id) @@ -672,12 +747,15 @@ async def _ready_comparator(self, session: AsyncSession) -> _Comparator: async def _links( self, mapping: ResourceMapping, resource: Resource[Any] ) -> dict[_Collection, list[Any]]: - links = {} - for collection, ids in _link_ids(mapping, resource).items(): - statement = _links_statement(collection, ids) - records = list((await self.session().scalars(statement)).all()) - self._check_links(collection, ids, records) - links[collection] = records + links: dict[_Collection, list[Any]] = {} + for collections, entries in _link_entries(mapping, resource): + ids = [entry.value for entry in entries] + found = [] + for collection in collections: + statement = _links_statement(collection, ids) + records = (await self.session().scalars(statement)).all() + found.append((collection, list(records))) + links |= self._assign_links(found, entries) return links async def _flush( diff --git a/tests/models.py b/tests/models.py index 7850527..3669b42 100644 --- a/tests/models.py +++ b/tests/models.py @@ -33,6 +33,13 @@ class Base(DeclarativeBase): Column("user_id", ForeignKey("users.id", ondelete="CASCADE"), primary_key=True), ) +nesting = Table( + "nesting", + Base.metadata, + Column("parent_id", ForeignKey("groups.id", ondelete="CASCADE"), primary_key=True), + Column("child_id", ForeignKey("groups.id", ondelete="CASCADE"), primary_key=True), +) + class UserRecord(Base): __tablename__ = "users" @@ -121,6 +128,11 @@ class GroupRecord(Base): members: Mapped[list[UserRecord]] = relationship( secondary=membership, back_populates="groups" ) + subgroups: Mapped[list["GroupRecord"]] = relationship( + secondary=nesting, + primaryjoin=lambda: GroupRecord.id == nesting.c.parent_id, + secondaryjoin=lambda: GroupRecord.id == nesting.c.child_id, + ) __mapper_args__ = {"version_id_col": version} @@ -157,7 +169,7 @@ def users_mapping(changes=None): "primary": EmailRecord.primary, }, ), - "groups": Link(UserRecord.groups, "Group"), + "groups": Link(UserRecord.groups, "Group", type="direct"), f"{ENTERPRISE}:employeeNumber": UserRecord.employee_number, f"{ENTERPRISE}:manager": Link(UserRecord.manager, "User"), "meta.created": UserRecord.created, @@ -178,7 +190,10 @@ def groups_mapping(changes=None): "id": GroupRecord.id, "externalId": GroupRecord.external_id, "displayName": GroupRecord.display_name, - "members": Link(GroupRecord.members, "User"), + "members": [ + Link(GroupRecord.members, "User", type="User"), + Link(GroupRecord.subgroups, "Group", type="Group"), + ], "meta.created": GroupRecord.created, "meta.lastModified": GroupRecord.last_modified, } diff --git a/tests/test_link.py b/tests/test_link.py new file mode 100644 index 0000000..0bc094c --- /dev/null +++ b/tests/test_link.py @@ -0,0 +1,216 @@ +import warnings + +import pytest +from scim2_models import EnterpriseUser +from scim2_models import Group +from scim2_models import InvalidFilterException +from scim2_models import InvalidValueException +from scim2_models import ResourceType +from scim2_models import ScimPolicy +from scim2_models import ScimProvider +from scim2_models import SearchRequest +from scim2_models import User +from scim2_server.utils import load_default_service_provider_config + +from scim2_sqlalchemy import Link + +from .models import GROUPS +from .models import USERS +from .models import GroupRecord +from .models import groups_mapping +from .models import users_mapping + +UserModel = User[EnterpriseUser] + + +@pytest.fixture +def alice(storage, user_type): + return storage.create(user_type, UserModel(user_name="alice", display_name="Alice")) + + +@pytest.fixture +def admins(storage, group_type): + return storage.create(group_type, Group(display_name="admins")) + + +def display_names(storage, group_type, **parameters): + page = storage.search([group_type], SearchRequest(**parameters)) + return sorted(resource.display_name for resource in page.resources) + + +def test_a_group_holds_users_and_groups(storage, group_type, alice, admins): + """The members are read in the order of the Links, each with the type of its Link.""" + group = storage.create( + group_type, + Group( + display_name="staff", + members=[{"value": admins.id}, {"value": alice.id}], + ), + ) + + assert [(m.value, m.type, m.display) for m in group.members] == [ + (alice.id, "User", "Alice"), + (admins.id, "Group", "admins"), + ] + + +def test_a_member_gets_the_ref_of_its_resource_type( + storage_factory, restricted_provider, user_type, group_type +): + """With a provider, each member gets the endpoint of its own resource type.""" + storage = storage_factory(provider=restricted_provider) + alice = storage.create(user_type, UserModel(user_name="alice")) + admins = storage.create(group_type, Group(display_name="admins")) + + group = storage.create( + group_type, Group(members=[{"value": alice.id}, {"value": admins.id}]) + ) + + assert [m.ref for m in group.members] == [ + f"Users/{alice.id}", + f"Groups/{admins.id}", + ] + + +@pytest.mark.parametrize("member_type", ["Group", "group"]) +def test_a_member_with_the_type_of_its_resource_is_accepted( + storage, group_type, admins, member_type +): + """The type a client sends is compared as filters compare it, without its case.""" + group = storage.create( + group_type, Group(members=[{"value": admins.id, "type": member_type}]) + ) + + assert [(m.value, m.type) for m in group.members] == [(admins.id, "Group")] + + +def test_a_member_with_another_type_is_refused(storage, group_type, admins): + """A group sent as a user raises a 400.""" + with pytest.raises(InvalidValueException, match="not of type 'User'"): + storage.create( + group_type, Group(members=[{"value": admins.id, "type": "User"}]) + ) + + +def refusing_key(binding, value): + """Fold the case, and refuse the strings holding an exclamation mark.""" + if "!" in value: + raise ValueError("'!' cannot be compared") + return value.casefold() + + +def test_a_type_the_policy_cannot_prepare_is_refused( + storage_factory, user_type, group_type +): + """A type the comparison key refuses matches no Link, and raises a 400, while the others are compared with the key.""" + provider = ScimProvider.from_discovery( + [*USERS.schemas(), *GROUPS.schemas()], + [ + ResourceType.from_resource(USERS.model), + ResourceType.from_resource(GROUPS.model), + ], + config=load_default_service_provider_config(), + policy=ScimPolicy(comparison_key=refusing_key), + ) + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + storage = storage_factory(provider=provider) + admins = storage.create(group_type, Group(display_name="admins")) + storage.create( + group_type, Group(members=[{"value": admins.id, "type": "GROUP"}]) + ) + + with pytest.raises(InvalidValueException): + storage.create( + group_type, Group(members=[{"value": admins.id, "type": "Group!"}]) + ) + + +def test_a_member_sent_twice_is_stored_once(storage, group_type, alice): + """Two entries with the same value link to the same user once.""" + group = storage.create( + group_type, Group(members=[{"value": alice.id}, {"value": alice.id}]) + ) + + assert [m.value for m in group.members] == [alice.id] + + +def test_a_group_can_hold_itself(storage, group_type, admins): + """RFC 7643 forbids no cycle, and the storage reads the direct members only.""" + admins.members = [{"value": admins.id}] + + group = storage.update(group_type, admins) + + assert [(m.value, m.type) for m in group.members] == [(admins.id, "Group")] + + +def test_the_subgroups_are_replaced(storage, group_type, alice, admins): + """Writing the members replaces the users and the groups.""" + group = storage.create( + group_type, + Group(display_name="staff", members=[{"value": admins.id}]), + ) + group.members = [{"value": alice.id}] + + group = storage.update(group_type, group) + + assert [m.value for m in group.members] == [alice.id] + + +def test_the_groups_of_a_user_are_direct(storage, user_type, group_type, alice): + """User.groups gets the type of its Link.""" + admins = storage.create(group_type, Group(members=[{"value": alice.id}])) + + groups = storage.get(user_type, alice.id).groups + + assert [(g.value, g.type) for g in groups] == [(admins.id, "direct")] + + +@pytest.mark.parametrize( + ("scim_filter", "expected"), + [ + ('members[type eq "Group"]', ["staff"]), + ('members[type eq "user"]', ["admins", "staff"]), + ('members.type eq "Group"', ["staff"]), + ('members.type ne "Group"', ["admins", "empty"]), + ('members[type eq "Group" and display eq "admins"]', ["staff"]), + ("members.type pr", ["admins", "staff"]), + ("members pr", ["admins", "staff"]), + ("members[display pr]", ["admins", "staff"]), + ('members.display eq "admins"', ["staff"]), + ('members.value eq "{admins}"', ["staff"]), + ('members.value eq "{alice}"', ["admins", "staff"]), + ('members.value ne "{alice}"', ["empty"]), + ], +) +def test_the_members_are_filtered_on_both_resource_types( + storage, group_type, alice, admins, scim_filter, expected +): + """A filter holds when a user or a group among the members matches it.""" + admins.members = [{"value": alice.id}] + storage.update(group_type, admins) + storage.create( + group_type, + Group( + display_name="staff", + members=[{"value": alice.id}, {"value": admins.id}], + ), + ) + storage.create(group_type, Group(display_name="empty")) + scim_filter = scim_filter.format(alice=alice.id, admins=admins.id) + + assert display_names(storage, group_type, filter=scim_filter) == expected + + +def test_the_type_of_an_untyped_link_is_not_stored( + storage_factory, user_type, group_type +): + """Without a type on its Link, a member type sent by a client raises a 400, and filters refuse it.""" + groups = groups_mapping({"members": Link(GroupRecord.members, "User")}) + storage = storage_factory({"User": users_mapping(), "Group": groups}) + alice = storage.create(user_type, UserModel(user_name="alice")) + + with pytest.raises(InvalidValueException): + storage.create(group_type, Group(members=[{"value": alice.id, "type": "User"}])) + with pytest.raises(InvalidFilterException): + display_names(storage, group_type, filter='members.type eq "User"') diff --git a/tests/test_mapping.py b/tests/test_mapping.py index 90e3c51..c290c38 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -214,20 +214,64 @@ def test_an_attribute_the_mapping_only_reads_is_published_read_only(): assert locale.mutability == Mutability.read_only -def test_a_link_publishes_its_value_its_ref_and_a_read_only_display(): - """A member is published before any storage links the mapping, with its $ref to users only.""" +def test_a_link_publishes_its_value_its_ref_its_type_and_a_read_only_display(): + """A member is published before any storage links the mapping, with its $ref to the resource types of its Links.""" (group,) = groups_mapping().schemas() (members,) = [a for a in group.attributes if a.name == "members"] assert [(sub.name, sub.mutability) for sub in members.sub_attributes] == [ ("value", Mutability.immutable), ("$ref", Mutability.immutable), + ("type", Mutability.immutable), ("display", Mutability.read_only), ] (ref,) = [sub for sub in members.sub_attributes if sub.name == "$ref"] + assert ref.reference_types == ["User", "Group"] + + +def test_a_link_without_type_does_not_publish_the_type(): + """The type of a member is published only when its Links give one.""" + (group,) = groups_mapping({"members": Link(GroupRecord.members, "User")}).schemas() + + (members,) = [a for a in group.attributes if a.name == "members"] + assert "type" not in [sub.name for sub in members.sub_attributes] + (ref,) = [sub for sub in members.sub_attributes if sub.name == "$ref"] assert ref.reference_types == ["User"] +@pytest.mark.parametrize( + "links, message", + [ + ([], "at least one"), + ( + [Link(GroupRecord.members, "User"), Link(GroupRecord.subgroups, "User")], + "one Link per resource type", + ), + ( + [ + Link(GroupRecord.members, "User", type="User"), + Link(GroupRecord.subgroups, "Group"), + ], + "every Link of 'members' has a type, or none", + ), + ([Link(GroupRecord.members, "User"), "User"], "map it with Many or Link"), + ], + ids=["empty", "same resource type", "type on some", "not a Link"], +) +def test_the_links_of_an_attribute_are_checked(links, message): + """The Links of an attribute link to distinct resource types, and give a type all or none.""" + with pytest.raises(ValueError, match=message): + groups_mapping({"members": links}) + + +def test_a_link_type_needs_a_type_sub_attribute(): + """The manager has no type sub-attribute, so its Link cannot give one.""" + with pytest.raises(ValueError, match="has no type sub-attribute"): + users_mapping( + {f"{ENTERPRISE}:manager": Link(UserRecord.manager, "User", type="User")} + ) + + def test_a_complex_attribute_only_read_is_published_read_only(): """Entries whose every mapped sub-attribute is only read cannot be written.""" emails = Many( diff --git a/tests/test_storage.py b/tests/test_storage.py index fea8abf..eca4047 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -184,17 +184,6 @@ def test_the_display_of_a_member_is_derived(storage, user_type, group_type): assert group.members[0].display == "Alice" -def test_the_type_of_a_member_is_not_stored(storage, user_type, group_type): - """members.type is not mapped, so a value for it raises a 400.""" - alice = storage.create(user_type, UserModel(user_name="alice")) - - with pytest.raises(InvalidValueException): - storage.create( - group_type, - Group(display_name="admins", members=[{"value": alice.id, "type": "User"}]), - ) - - def test_a_group_links_to_its_members(storage, user_type, group_type): """A member is read back with the id and the displayName of the user.""" alice = storage.create(