Conversation
ddb4721 to
75adf75
Compare
| "required_error", | ||
| "Field 'total_results' is required but value is missing or null", | ||
| ) | ||
|
|
There was a problem hiding this comment.
According to RFC7644 §3.4.2 totalResults is required:
totalResults The total number of results returned by the list or
query operation. The value may be larger than the number of
resources returned, such as when returning a single page (see
Section 3.4.2.4) of results where multiple pages are available.
REQUIRED.
But according to RFC9865 §2 totalResults is optional:
As described in Section 3.4.1 of [RFC7644], service providers should return an accurate value for totalResults, which is the total number of resources for all pages. Service providers implementing cursor pagination that are unable to estimate totalResults MAY choose to omit the totalResults attribute.
We should probably enforce total_results only when nextCursor or previousCursor are present. What do you think?
There was a problem hiding this comment.
We should probably enforce total_results only when nextCursor or previousCursor are present. What do you think?
Actually, according to RFC9865 §2, both nextCursor and previousCursor are optional, so they are probably not very reliable.
There was a problem hiding this comment.
The §1 of RFC9865 says:
These changes are invoked when using the "cursor" parameter when making SCIM search requests using GET or POST methods.
That does not really help since it would mean that we would have to remember what was passed in the request to know how to validate the response 🤔
There was a problem hiding this comment.
In the other hand, a bit later we can read:
The cursor value MUST be empty or omitted for the first request of a cursor-paginated query.
So there are situations where cursor can be absent, so that don't seem to be a good discriminant.
There was a problem hiding this comment.
Ok I think I know what we can do. We can maybe read the pagination.cursor parameter from the ServiceProviderConfiguration object newly passed in context["scim_spc"].
So if pagination.cursor is True, total_result is optional, and it is mandatory otherwise.
b72cb45 to
3067d54
Compare
# Conflicts: # doc/changelog.rst
| @model_validator(mode="wrap") | ||
| @classmethod | ||
| def default_start_index( | ||
| cls, value: Any, handler: ValidatorFunctionWrapHandler, info: ValidationInfo | ||
| ) -> Self: | ||
| """Default to start_index 1 if no start_index or cursor is provided.""" | ||
| obj = handler(value) | ||
| assert isinstance(obj, cls) | ||
|
|
||
| if obj.cursor is None and obj.start_index is None: | ||
| obj.start_index = 1 | ||
|
|
||
| return obj |
There was a problem hiding this comment.
Which part of the RFC tells to set 1 as the default value?
There was a problem hiding this comment.
RFC 7644 does in the definition of the index based pagination here https://www.rfc-editor.org/info/rfc7644/#section-3.4.2.4
There was a problem hiding this comment.
Ok so this part:
+------------+----------------------------+-------------------------+
| Parameter | Description | Default |
+------------+----------------------------+-------------------------+
| startIndex | The 1-based index of the | 1 |
| | first query result. A | |
| | value less than 1 SHALL be | |
| | interpreted as 1. | |
I am not so sure about this validator though, it injects a startIndex value on every payload, where users might want to explicitly omit it. This is what causes this:
I wonder if there is a way that the param takes the value 1 when unset only when it is read, but is not injected in payloads when it is unset. Maybe pydantic has something to offer in that direction?
| index: Annotated[bool | None, Mutability.read_only, Required.true] = None | ||
| """A Boolean value specifying whether or not the operation is supported.""" | ||
|
|
||
| default_pagination_method: Annotated[str | None, Mutability.read_only] = None |
There was a problem hiding this comment.
Should be a ExtensibleStringEnum instead of str. You can find several examples of usage in the lib.
| "schemas": ["urn:ietf:params:scim:api:messages:2.0:SearchRequest"], | ||
| "cursor": "", | ||
| "count": 10, | ||
| "start_index": 1, |
There was a problem hiding this comment.
It should be camelCase in payloads (startIndex).
| default_page_size: Annotated[int | None, Mutability.read_only] = None | ||
| """An integer value specifying the default page size.""" |
There was a problem hiding this comment.
I think we can take the verbatim description from the RFC here (and same thing for the two other params with their own descriptions) without the OPTIONAL.
Positive integer value specifying the default number of results returned in a page when a count is not specified in the query.
Plus, the RFC enforces that the integer should be positive, pydantic has a native type for that
| @model_validator(mode="after") | ||
| def check_cursor_and_index(self, info: ValidationInfo) -> Self: | ||
| if self.cursor is not None and self.start_index is not None: | ||
| raise PydanticCustomError( | ||
| "index_and_cursor_error", | ||
| "'cursor' and 'start_index' are mutually exclusive", | ||
| ) | ||
| return self |
There was a problem hiding this comment.
Does the RFC enforces those values as mutually exclusive?
If not, maybe the decision of which method to use should be left to the server implementation.
There was a problem hiding this comment.
it doesn't explicitly, the RFC just states "When a service provider supports both index-based and cursor-based pagination, clients can use the 'startIndex' or 'cursor' query parameters to request a specific method. "
so if both start index and cursor are sent, it would be up to the server to fallback to its default/preferred method, the same as if none of the two are sent ?
2 participants
Add support for cursor based pagination based on RFC 9865
Resolves #113