diff --git a/.changeset/opaque-pagination-cursors.md b/.changeset/opaque-pagination-cursors.md new file mode 100644 index 0000000..cc51bd2 --- /dev/null +++ b/.changeset/opaque-pagination-cursors.md @@ -0,0 +1,49 @@ +--- +"@eigeninteractive/server": minor +--- + +Pagination cursors are now opaque tokens, and empty query parameters are treated as absent. + +Every paged list could silently return nothing. A client that sent an optional +query parameter it had no value for produced `?cursor`, which arrives as the +empty string, and `z.coerce.number()` is `Number()`, so `""` became `0`. Zero is +a structurally valid integer cursor meaning "strictly older than the beginning of +time", so the request did not fail: the lobby, both `games/mine` buckets, the +friends list and every history screen returned `200` with an empty array. Nothing +was logged, because nothing had gone wrong as far as any layer could tell. + +Three changes, each closing a different part of it: + +- **`cursor` is now an opaque string** rather than a bare epoch-millisecond + integer, and paged responses carry a **`nextCursor`** that is null exactly when + the list is exhausted. There is no byte string that accidentally decodes to the + beginning of time, so the failure above cannot recur by construction. A cursor + that does not decode is a `400` with the new `invalidCursor` code. +- **The cursor carries the row id alongside the sort value**, so pages no longer + drop a row when two games share a timestamp - a limitation the previous + implementation documented but did not fix. The comparison is a SQLite row value + (`(sortKey, id) < (?, ?)`), which is planned against the same index. +- **Query parameters are parsed, never coerced.** This is the origin of the whole + failure, and it took four correct layers to become invisible. `Number(null)` is + `0`, so `z.coerce.number().int().min(0)` genuinely accepts null; the emitted + schema honestly reported `["integer", "null"]`; `openapi-generator` correctly + dropped its `if (x != null)` guard, because the API had declared null welcome; + dio rendered that null as a bare `?to=`; and the server coerced `""` back to + `0`. No library was misbehaving. Integer query parameters now parse from a + strict pattern, so they reject null and empty alike, which means the generated + client omits an absent parameter on its own and a malformed one is a loud `400` + rather than a plausible, wrong `200`. `?pool=` and `?to=` were failing the same + way and are fixed by the same change. + + A contract test now asserts that no query parameter in the published document + is nullable, which is the tell that was invisible in review: `.min(0)` was + nullable and `.min(1)` was not. + +Because `nextCursor` is now an answer rather than something a client infers from +a short page, callers no longer need to know how any list is sorted, and a final +page that happens to be exactly full no longer triggers one pointless request. + +Breaking for direct API consumers: `cursor` is a string, and the four paged +responses (`Lobby`, `MyGames`, `PlayerGames`, `FriendsGames`) gained a required +`nextCursor` field. The generated Dart client and `eigen_flutter` are updated to +match. diff --git a/clients/dart/doc/FriendsGames.md b/clients/dart/doc/FriendsGames.md index b91dd2e..7bc7a2a 100644 --- a/clients/dart/doc/FriendsGames.md +++ b/clients/dart/doc/FriendsGames.md @@ -9,6 +9,7 @@ import 'package:eigen_api/api.dart'; Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- **games** | [**List<GameSummary>**](GameSummary.md) | | +**nextCursor** | **String** | Pass as `cursor` to fetch the next page. Null when there are no more results. | [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/clients/dart/doc/GamesApi.md b/clients/dart/doc/GamesApi.md index fec5d9d..f2941cb 100644 --- a/clients/dart/doc/GamesApi.md +++ b/clients/dart/doc/GamesApi.md @@ -375,7 +375,7 @@ import 'package:eigen_api/api.dart'; final api = EigenApi().getGamesApi(); final int limit = 56; // int | -final int cursor = 56; // int | +final String cursor = cursor_example; // String | try { final response = api.getLobby(limit, cursor); @@ -390,7 +390,7 @@ try { Name | Type | Description | Notes ------------- | ------------- | ------------- | ------------- **limit** | **int**| | [optional] [default to 20] - **cursor** | **int**| | [optional] + **cursor** | **String**| | [optional] ### Return type @@ -419,7 +419,7 @@ import 'package:eigen_api/api.dart'; final api = EigenApi().getGamesApi(); final String bucket = bucket_example; // String | final int limit = 56; // int | -final int cursor = 56; // int | +final String cursor = cursor_example; // String | try { final response = api.getMyGames(bucket, limit, cursor); @@ -435,7 +435,7 @@ Name | Type | Description | Notes ------------- | ------------- | ------------- | ------------- **bucket** | **String**| | [optional] [default to 'active'] **limit** | **int**| | [optional] [default to 20] - **cursor** | **int**| | [optional] + **cursor** | **String**| | [optional] ### Return type diff --git a/clients/dart/doc/Lobby.md b/clients/dart/doc/Lobby.md index 9fa7178..fe327a5 100644 --- a/clients/dart/doc/Lobby.md +++ b/clients/dart/doc/Lobby.md @@ -9,6 +9,7 @@ import 'package:eigen_api/api.dart'; Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- **games** | [**List<GameSummary>**](GameSummary.md) | | +**nextCursor** | **String** | Pass as `cursor` to fetch the next page. Null when there are no more results. | [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/clients/dart/doc/MyGames.md b/clients/dart/doc/MyGames.md index 521fb3a..48e8218 100644 --- a/clients/dart/doc/MyGames.md +++ b/clients/dart/doc/MyGames.md @@ -9,6 +9,7 @@ import 'package:eigen_api/api.dart'; Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- **games** | [**List<GameSummary>**](GameSummary.md) | | +**nextCursor** | **String** | Pass as `cursor` to fetch the next page. Null when there are no more results. | [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/clients/dart/doc/PlayerGames.md b/clients/dart/doc/PlayerGames.md index 745c74c..520e47a 100644 --- a/clients/dart/doc/PlayerGames.md +++ b/clients/dart/doc/PlayerGames.md @@ -9,6 +9,7 @@ import 'package:eigen_api/api.dart'; Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- **games** | [**List<GameSummary>**](GameSummary.md) | | +**nextCursor** | **String** | Pass as `cursor` to fetch the next page. Null when there are no more results. | [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/clients/dart/doc/PlayersApi.md b/clients/dart/doc/PlayersApi.md index 5ae6edd..74465ee 100644 --- a/clients/dart/doc/PlayersApi.md +++ b/clients/dart/doc/PlayersApi.md @@ -26,7 +26,7 @@ import 'package:eigen_api/api.dart'; final api = EigenApi().getPlayersApi(); final String playerId = playerId_example; // String | final int limit = 56; // int | -final int cursor = 56; // int | +final String cursor = cursor_example; // String | try { final response = api.getPlayerGames(playerId, limit, cursor); @@ -42,7 +42,7 @@ Name | Type | Description | Notes ------------- | ------------- | ------------- | ------------- **playerId** | **String**| | **limit** | **int**| | [optional] [default to 20] - **cursor** | **int**| | [optional] + **cursor** | **String**| | [optional] ### Return type diff --git a/clients/dart/doc/SocialApi.md b/clients/dart/doc/SocialApi.md index b6ec79e..f226494 100644 --- a/clients/dart/doc/SocialApi.md +++ b/clients/dart/doc/SocialApi.md @@ -111,7 +111,7 @@ import 'package:eigen_api/api.dart'; final api = EigenApi().getSocialApi(); final int limit = 56; // int | -final int cursor = 56; // int | +final String cursor = cursor_example; // String | try { final response = api.getFriendsGames(limit, cursor); @@ -126,7 +126,7 @@ try { Name | Type | Description | Notes ------------- | ------------- | ------------- | ------------- **limit** | **int**| | [optional] [default to 20] - **cursor** | **int**| | [optional] + **cursor** | **String**| | [optional] ### Return type diff --git a/clients/dart/lib/src/api/games_api.dart b/clients/dart/lib/src/api/games_api.dart index a95c0c0..f47b658 100644 --- a/clients/dart/lib/src/api/games_api.dart +++ b/clients/dart/lib/src/api/games_api.dart @@ -554,7 +554,10 @@ class GamesApi { validateStatus: validateStatus, ); - final _queryParameters = {r'from': from, r'to': to}; + final _queryParameters = { + if (from != null) r'from': from, + if (to != null) r'to': to, + }; final _response = await _dio.request( _path, @@ -771,7 +774,7 @@ class GamesApi { /// Throws [DioException] if API call or serialization fails Future> getLobby({ int? limit = 20, - int? cursor, + String? cursor, CancelToken? cancelToken, Map? headers, Map? extra, @@ -794,7 +797,7 @@ class GamesApi { final _queryParameters = { if (limit != null) r'limit': limit, - r'cursor': cursor, + if (cursor != null) r'cursor': cursor, }; final _response = await _dio.request( @@ -854,7 +857,7 @@ class GamesApi { Future> getMyGames({ String? bucket = 'active', int? limit = 20, - int? cursor, + String? cursor, CancelToken? cancelToken, Map? headers, Map? extra, @@ -878,7 +881,7 @@ class GamesApi { final _queryParameters = { if (bucket != null) r'bucket': bucket, if (limit != null) r'limit': limit, - r'cursor': cursor, + if (cursor != null) r'cursor': cursor, }; final _response = await _dio.request( diff --git a/clients/dart/lib/src/api/players_api.dart b/clients/dart/lib/src/api/players_api.dart index e1359e0..0c59494 100644 --- a/clients/dart/lib/src/api/players_api.dart +++ b/clients/dart/lib/src/api/players_api.dart @@ -38,7 +38,7 @@ class PlayersApi { Future> getPlayerGames({ required String playerId, int? limit = 20, - int? cursor, + String? cursor, CancelToken? cancelToken, Map? headers, Map? extra, @@ -66,7 +66,7 @@ class PlayersApi { final _queryParameters = { if (limit != null) r'limit': limit, - r'cursor': cursor, + if (cursor != null) r'cursor': cursor, }; final _response = await _dio.request( diff --git a/clients/dart/lib/src/api/social_api.dart b/clients/dart/lib/src/api/social_api.dart index b636952..efaadfe 100644 --- a/clients/dart/lib/src/api/social_api.dart +++ b/clients/dart/lib/src/api/social_api.dart @@ -143,7 +143,7 @@ class SocialApi { /// Throws [DioException] if API call or serialization fails Future> getFriendsGames({ int? limit = 20, - int? cursor, + String? cursor, CancelToken? cancelToken, Map? headers, Map? extra, @@ -166,7 +166,7 @@ class SocialApi { final _queryParameters = { if (limit != null) r'limit': limit, - r'cursor': cursor, + if (cursor != null) r'cursor': cursor, }; final _response = await _dio.request( diff --git a/clients/dart/lib/src/model/error_code.dart b/clients/dart/lib/src/model/error_code.dart index a1189fa..ff84159 100644 --- a/clients/dart/lib/src/model/error_code.dart +++ b/clients/dart/lib/src/model/error_code.dart @@ -95,6 +95,10 @@ enum ErrorCode { @JsonValue(r'rateLimited') rateLimited(r'rateLimited'), + /// A stable machine code identifying why a request failed. + @JsonValue(r'invalidCursor') + invalidCursor(r'invalidCursor'), + /// A stable machine code identifying why a request failed. @JsonValue(r'unknown_default_open_api') unknownDefaultOpenApi(r'unknown_default_open_api'); diff --git a/clients/dart/lib/src/model/error_response.g.dart b/clients/dart/lib/src/model/error_response.g.dart index 04854db..951a659 100644 --- a/clients/dart/lib/src/model/error_response.g.dart +++ b/clients/dart/lib/src/model/error_response.g.dart @@ -52,5 +52,6 @@ const _$ErrorCodeEnumMap = { ErrorCode.imageTooLarge: 'imageTooLarge', ErrorCode.unsupportedImageType: 'unsupportedImageType', ErrorCode.rateLimited: 'rateLimited', + ErrorCode.invalidCursor: 'invalidCursor', ErrorCode.unknownDefaultOpenApi: 'unknown_default_open_api', }; diff --git a/clients/dart/lib/src/model/friends_games.dart b/clients/dart/lib/src/model/friends_games.dart index e609c5a..0828f61 100644 --- a/clients/dart/lib/src/model/friends_games.dart +++ b/clients/dart/lib/src/model/friends_games.dart @@ -16,17 +16,25 @@ part 'friends_games.g.dart'; ) class FriendsGames { /// Returns a new [FriendsGames] instance. - FriendsGames({required this.games}); + FriendsGames({required this.games, required this.nextCursor}); @JsonKey(name: r'games', required: true, includeIfNull: false) final List games; + /// Pass as `cursor` to fetch the next page. Null when there are no more results. + @JsonKey(name: r'nextCursor', required: true, includeIfNull: true) + final String? nextCursor; + @override bool operator ==(Object other) => - identical(this, other) || other is FriendsGames && other.games == games; + identical(this, other) || + other is FriendsGames && + other.games == games && + other.nextCursor == nextCursor; @override - int get hashCode => games.hashCode; + int get hashCode => + games.hashCode + (nextCursor == null ? 0 : nextCursor.hashCode); factory FriendsGames.fromJson(Map json) => _$FriendsGamesFromJson(json); diff --git a/clients/dart/lib/src/model/friends_games.g.dart b/clients/dart/lib/src/model/friends_games.g.dart index 44035f6..824ef00 100644 --- a/clients/dart/lib/src/model/friends_games.g.dart +++ b/clients/dart/lib/src/model/friends_games.g.dart @@ -8,7 +8,7 @@ part of 'friends_games.dart'; FriendsGames _$FriendsGamesFromJson(Map json) => $checkedCreate('FriendsGames', json, ($checkedConvert) { - $checkKeys(json, requiredKeys: const ['games']); + $checkKeys(json, requiredKeys: const ['games', 'nextCursor']); final val = FriendsGames( games: $checkedConvert( 'games', @@ -16,9 +16,13 @@ FriendsGames _$FriendsGamesFromJson(Map json) => .map((e) => GameSummary.fromJson(e as Map)) .toList(), ), + nextCursor: $checkedConvert('nextCursor', (v) => v as String?), ); return val; }); Map _$FriendsGamesToJson(FriendsGames instance) => - {'games': instance.games.map((e) => e.toJson()).toList()}; + { + 'games': instance.games.map((e) => e.toJson()).toList(), + 'nextCursor': instance.nextCursor, + }; diff --git a/clients/dart/lib/src/model/lobby.dart b/clients/dart/lib/src/model/lobby.dart index c86f6e6..8d022f2 100644 --- a/clients/dart/lib/src/model/lobby.dart +++ b/clients/dart/lib/src/model/lobby.dart @@ -16,17 +16,23 @@ part 'lobby.g.dart'; ) class Lobby { /// Returns a new [Lobby] instance. - Lobby({required this.games}); + Lobby({required this.games, required this.nextCursor}); @JsonKey(name: r'games', required: true, includeIfNull: false) final List games; + /// Pass as `cursor` to fetch the next page. Null when there are no more results. + @JsonKey(name: r'nextCursor', required: true, includeIfNull: true) + final String? nextCursor; + @override bool operator ==(Object other) => - identical(this, other) || other is Lobby && other.games == games; + identical(this, other) || + other is Lobby && other.games == games && other.nextCursor == nextCursor; @override - int get hashCode => games.hashCode; + int get hashCode => + games.hashCode + (nextCursor == null ? 0 : nextCursor.hashCode); factory Lobby.fromJson(Map json) => _$LobbyFromJson(json); diff --git a/clients/dart/lib/src/model/lobby.g.dart b/clients/dart/lib/src/model/lobby.g.dart index cd4c606..6b006ae 100644 --- a/clients/dart/lib/src/model/lobby.g.dart +++ b/clients/dart/lib/src/model/lobby.g.dart @@ -8,7 +8,7 @@ part of 'lobby.dart'; Lobby _$LobbyFromJson(Map json) => $checkedCreate('Lobby', json, ($checkedConvert) { - $checkKeys(json, requiredKeys: const ['games']); + $checkKeys(json, requiredKeys: const ['games', 'nextCursor']); final val = Lobby( games: $checkedConvert( 'games', @@ -16,10 +16,12 @@ Lobby _$LobbyFromJson(Map json) => .map((e) => GameSummary.fromJson(e as Map)) .toList(), ), + nextCursor: $checkedConvert('nextCursor', (v) => v as String?), ); return val; }); Map _$LobbyToJson(Lobby instance) => { 'games': instance.games.map((e) => e.toJson()).toList(), + 'nextCursor': instance.nextCursor, }; diff --git a/clients/dart/lib/src/model/my_games.dart b/clients/dart/lib/src/model/my_games.dart index fa8a10d..9aec91b 100644 --- a/clients/dart/lib/src/model/my_games.dart +++ b/clients/dart/lib/src/model/my_games.dart @@ -16,17 +16,25 @@ part 'my_games.g.dart'; ) class MyGames { /// Returns a new [MyGames] instance. - MyGames({required this.games}); + MyGames({required this.games, required this.nextCursor}); @JsonKey(name: r'games', required: true, includeIfNull: false) final List games; + /// Pass as `cursor` to fetch the next page. Null when there are no more results. + @JsonKey(name: r'nextCursor', required: true, includeIfNull: true) + final String? nextCursor; + @override bool operator ==(Object other) => - identical(this, other) || other is MyGames && other.games == games; + identical(this, other) || + other is MyGames && + other.games == games && + other.nextCursor == nextCursor; @override - int get hashCode => games.hashCode; + int get hashCode => + games.hashCode + (nextCursor == null ? 0 : nextCursor.hashCode); factory MyGames.fromJson(Map json) => _$MyGamesFromJson(json); diff --git a/clients/dart/lib/src/model/my_games.g.dart b/clients/dart/lib/src/model/my_games.g.dart index d1b8774..9daf571 100644 --- a/clients/dart/lib/src/model/my_games.g.dart +++ b/clients/dart/lib/src/model/my_games.g.dart @@ -8,7 +8,7 @@ part of 'my_games.dart'; MyGames _$MyGamesFromJson(Map json) => $checkedCreate('MyGames', json, ($checkedConvert) { - $checkKeys(json, requiredKeys: const ['games']); + $checkKeys(json, requiredKeys: const ['games', 'nextCursor']); final val = MyGames( games: $checkedConvert( 'games', @@ -16,10 +16,12 @@ MyGames _$MyGamesFromJson(Map json) => .map((e) => GameSummary.fromJson(e as Map)) .toList(), ), + nextCursor: $checkedConvert('nextCursor', (v) => v as String?), ); return val; }); Map _$MyGamesToJson(MyGames instance) => { 'games': instance.games.map((e) => e.toJson()).toList(), + 'nextCursor': instance.nextCursor, }; diff --git a/clients/dart/lib/src/model/player_games.dart b/clients/dart/lib/src/model/player_games.dart index 7c30570..e729d7c 100644 --- a/clients/dart/lib/src/model/player_games.dart +++ b/clients/dart/lib/src/model/player_games.dart @@ -16,17 +16,25 @@ part 'player_games.g.dart'; ) class PlayerGames { /// Returns a new [PlayerGames] instance. - PlayerGames({required this.games}); + PlayerGames({required this.games, required this.nextCursor}); @JsonKey(name: r'games', required: true, includeIfNull: false) final List games; + /// Pass as `cursor` to fetch the next page. Null when there are no more results. + @JsonKey(name: r'nextCursor', required: true, includeIfNull: true) + final String? nextCursor; + @override bool operator ==(Object other) => - identical(this, other) || other is PlayerGames && other.games == games; + identical(this, other) || + other is PlayerGames && + other.games == games && + other.nextCursor == nextCursor; @override - int get hashCode => games.hashCode; + int get hashCode => + games.hashCode + (nextCursor == null ? 0 : nextCursor.hashCode); factory PlayerGames.fromJson(Map json) => _$PlayerGamesFromJson(json); diff --git a/clients/dart/lib/src/model/player_games.g.dart b/clients/dart/lib/src/model/player_games.g.dart index ba671c6..9ebdb55 100644 --- a/clients/dart/lib/src/model/player_games.g.dart +++ b/clients/dart/lib/src/model/player_games.g.dart @@ -8,7 +8,7 @@ part of 'player_games.dart'; PlayerGames _$PlayerGamesFromJson(Map json) => $checkedCreate('PlayerGames', json, ($checkedConvert) { - $checkKeys(json, requiredKeys: const ['games']); + $checkKeys(json, requiredKeys: const ['games', 'nextCursor']); final val = PlayerGames( games: $checkedConvert( 'games', @@ -16,9 +16,13 @@ PlayerGames _$PlayerGamesFromJson(Map json) => .map((e) => GameSummary.fromJson(e as Map)) .toList(), ), + nextCursor: $checkedConvert('nextCursor', (v) => v as String?), ); return val; }); Map _$PlayerGamesToJson(PlayerGames instance) => - {'games': instance.games.map((e) => e.toJson()).toList()}; + { + 'games': instance.games.map((e) => e.toJson()).toList(), + 'nextCursor': instance.nextCursor, + }; diff --git a/packages/server/openapi.json b/packages/server/openapi.json index cd4ee08..37434ed 100644 --- a/packages/server/openapi.json +++ b/packages/server/openapi.json @@ -75,10 +75,18 @@ "items": { "$ref": "#/components/schemas/GameSummary" } + }, + "nextCursor": { + "type": [ + "string", + "null" + ], + "description": "Pass as `cursor` to fetch the next page. Null when there are no more results." } }, "required": [ - "games" + "games", + "nextCursor" ] }, "GameSummary": { @@ -402,7 +410,8 @@ "registrationRequired", "imageTooLarge", "unsupportedImageType", - "rateLimited" + "rateLimited", + "invalidCursor" ], "description": "A stable machine code identifying why a request failed." }, @@ -414,10 +423,18 @@ "items": { "$ref": "#/components/schemas/GameSummary" } + }, + "nextCursor": { + "type": [ + "string", + "null" + ], + "description": "Pass as `cursor` to fetch the next page. Null when there are no more results." } }, "required": [ - "games" + "games", + "nextCursor" ] }, "Session": { @@ -762,10 +779,18 @@ "items": { "$ref": "#/components/schemas/GameSummary" } + }, + "nextCursor": { + "type": [ + "string", + "null" + ], + "description": "Pass as `cursor` to fetch the next page. Null when there are no more results." } }, "required": [ - "games" + "games", + "nextCursor" ] }, "RatingHistory": { @@ -1263,10 +1288,18 @@ "items": { "$ref": "#/components/schemas/GameSummary" } + }, + "nextCursor": { + "type": [ + "string", + "null" + ], + "description": "Pass as `cursor` to fetch the next page. Null when there are no more results." } }, "required": [ - "games" + "games", + "nextCursor" ] }, "UserSearch": { @@ -1385,10 +1418,8 @@ }, { "schema": { - "type": [ - "integer", - "null" - ] + "type": "string", + "minLength": 1 }, "required": false, "name": "cursor", @@ -1482,10 +1513,8 @@ }, { "schema": { - "type": [ - "integer", - "null" - ] + "type": "string", + "minLength": 1 }, "required": false, "name": "cursor", @@ -1999,10 +2028,8 @@ }, { "schema": { - "type": [ - "integer", - "null" - ] + "type": "string", + "minLength": 1 }, "required": false, "name": "cursor", @@ -2143,7 +2170,8 @@ "parameters": [ { "schema": { - "type": "string" + "type": "string", + "minLength": 1 }, "required": false, "name": "pool", @@ -3210,11 +3238,9 @@ }, { "schema": { - "type": [ - "integer", - "null" - ], + "type": "integer", "minimum": 0, + "maximum": 9007199254740991, "default": 0 }, "required": false, @@ -3223,11 +3249,9 @@ }, { "schema": { - "type": [ - "integer", - "null" - ], - "minimum": 0 + "type": "integer", + "minimum": 0, + "maximum": 9007199254740991 }, "required": false, "name": "to", @@ -3703,10 +3727,8 @@ }, { "schema": { - "type": [ - "integer", - "null" - ] + "type": "string", + "minLength": 1 }, "required": false, "name": "cursor", diff --git a/packages/server/src/cursor.ts b/packages/server/src/cursor.ts new file mode 100644 index 0000000..db839a4 --- /dev/null +++ b/packages/server/src/cursor.ts @@ -0,0 +1,115 @@ +/** + * The pagination cursor: an opaque token naming a position in a sorted list. + * + * Opaque is the whole design, and it is a correction of an earlier one. The + * cursor used to be the raw sort value, a bare epoch-millisecond integer that a + * client read off the last row it held and sent back. Three things were wrong + * with that, and they are worth naming because each is invisible until it bites: + * + * 1. **Its invalid value was a valid value.** A parameter that failed to arrive + * coerced to `0`, and `0` is a perfectly good timestamp cursor meaning + * "strictly older than the beginning of time". So a malformed request came + * back `200 []` instead of `400`, and every paged list in the product went + * blank at once without a single error being logged. A token that has to + * decode cannot do that: there is no byte string that accidentally means the + * beginning of time. + * + * 2. **A timestamp alone does not identify a row.** Two games created in the + * same millisecond share a sort value, so a page boundary landing between + * them dropped one permanently: it was neither `< cursor` nor on the page + * just served. Carrying the row id alongside the sort value makes the + * ordering total, so a boundary can always be placed exactly. + * + * 3. **It made the client restate the server's ORDER BY.** To ask for the next + * page, a client had to know that finished games sort by `finishedAt` and + * fall back to `updatedAt` while active ones sort by `updatedAt`. That is + * server knowledge, and every screen that paged had a copy of it. Now the + * server hands back `nextCursor` and the client passes it through without + * ever looking inside. + * + * The encoding is deliberately not a security boundary. It is base64url'd JSON, + * trivially readable, and a caller who decodes one and forges another gets + * exactly the ability they already have: to ask for a page starting at a + * position of their choosing, over data the query's own WHERE clause already + * restricts to what they may see. Signing it would buy nothing and would make + * cursors non-portable across key rotations. + */ + +import { HttpError } from "./http.js"; + +/** A position in a sorted list: the sort value, and the row id that breaks + * ties on it. Together these are total, which is what makes a page boundary + * exact. */ +export interface Cursor { + /** The sort value of the last row on the page just served, in epoch ms. */ + t: number; + /** That row's id. */ + id: string; +} + +/** Encoded as a two-element array rather than an object: the field names would + * be a third of the token and carry no information a reader of this file lacks. */ +type Encoded = [number, string]; + +/** base64url via the runtime's own codec rather than `btoa` plus a hand-rolled + * alphabet swap and padding calculation. + * + * `Uint8Array.toBase64`/`fromBase64` (TC39 "Uint8Array to/from base64", shipped + * in workerd) take the alphabet and the padding as options, which is the entire + * fiddly part of doing this by hand: the encoder has to translate `+/` to `-_` + * and strip `=`, and the decoder has to put exactly the right number of `=` + * back before `atob` will accept the length. Getting that padding arithmetic + * subtly wrong fails only for certain input lengths, which is the kind of bug + * that survives a test suite. */ +const utf8 = { encode: new TextEncoder(), decode: new TextDecoder() }; + +function base64UrlEncode(text: string): string { + return utf8.encode.encode(text).toBase64({ alphabet: "base64url", omitPadding: true }); +} + +function base64UrlDecode(token: string): string { + return utf8.decode.decode(Uint8Array.fromBase64(token, { alphabet: "base64url" })); +} + +export function encodeCursor(cursor: Cursor): string { + return base64UrlEncode(JSON.stringify([cursor.t, cursor.id] satisfies Encoded)); +} + +/** + * Decode a cursor a client sent back, or reject the request. + * + * Every failure is the same 400: a cursor is not something a client composes, + * it is something it echoes, so the only ways to arrive here with a bad one are + * a truncated URL or a hand-edited request. Neither is worth distinguishing to + * the caller, and both are worth refusing loudly rather than silently serving + * page one or nothing. + */ +export function decodeCursor(token: string): Cursor { + let parsed: unknown; + try { + parsed = JSON.parse(base64UrlDecode(token)); + } catch { + throw new HttpError(400, "Malformed pagination cursor", "invalidCursor"); + } + if (!Array.isArray(parsed) || parsed.length !== 2) throw new HttpError(400, "Malformed pagination cursor", "invalidCursor"); + const [t, id] = parsed as [unknown, unknown]; + if (typeof t !== "number" || !Number.isInteger(t) || typeof id !== "string" || id.length === 0) throw new HttpError(400, "Malformed pagination cursor", "invalidCursor"); + return { t, id }; +} + +/** Decode an optional cursor: absent stays absent, present must be valid. */ +export function decodeOptionalCursor(token: string | undefined): Cursor | null { + return token === undefined ? null : decodeCursor(token); +} + +/** One page of a keyset-paginated list. + * + * `nextCursor` is an answer, not a hint: it is null exactly when the server + * knows there is nothing after this page. That is why the reads fetch one row + * more than the caller asked for and discard it. The obvious alternative, + * letting the client stop when a page comes back short, is a guess that is + * wrong whenever the last page happens to be exactly full. */ +export interface Page { + rows: T[]; + nextCursor: string | null; +} diff --git a/packages/server/src/d1/reads.ts b/packages/server/src/d1/reads.ts index 4bdd2ea..98584a7 100644 --- a/packages/server/src/d1/reads.ts +++ b/packages/server/src/d1/reads.ts @@ -6,7 +6,8 @@ */ import type { RatingDelta, Seat } from "@eigeninteractive/kernel"; -import { and, desc, eq, inArray, lt, or, type SQLWrapper, sql } from "drizzle-orm"; +import { and, desc, eq, inArray, or, type SQLWrapper, sql } from "drizzle-orm"; +import { type Cursor, encodeCursor, type Page } from "../cursor.js"; import { noBlockedParticipant } from "./blocks.js"; import { orm } from "./orm.js"; import { bots, games, participants, playerRatings, ratingHistory, relationships, users } from "./schema.js"; @@ -154,58 +155,96 @@ export async function readGameByCode(d1: D1Database, shortCode: string): Promise return (await withRatings(d1, [{ ...row, participants: seatRows }]))[0]; } -/** Keyset pagination: fetch strictly older than the caller's last row. +/** Keyset pagination: fetch strictly after the caller's last row, in the + * list's own descending order. * * A cursor rather than an offset because these lists change underneath the * reader: a new lobby game shifts every OFFSET by one and makes a scroll show - * the same row twice. The cursor is the previous page's last sort value, so a - * page is stable no matter what was inserted since. It also stays index-served - * at any depth, where OFFSET degrades linearly. + * the same row twice. A cursor names a position rather than a count, so a page + * is stable no matter what was inserted since, and it stays index-served at any + * depth where OFFSET degrades linearly. * - * Ties are possible (two games created in the same millisecond) and would drop - * a row; in practice the epoch-ms resolution plus `limit` makes that vanishing, - * and the alternative, a compound (timestamp, id) cursor, is not worth the - * wire complexity for a game list. */ -function olderThan(column: SQLWrapper, cursor: number | null) { - return cursor === null ? undefined : lt(column, cursor); + * The comparison is on the PAIR (sort value, id), not on the sort value alone. + * Timestamps tie - two games created in the same millisecond share one - and + * under a `sortKey < cursor` boundary a tied row is neither after the page just + * served nor on it, so it is skipped and never seen again. Ordering by + * `(sortKey, id)` makes the ordering total, and comparing lexicographically + * places the boundary exactly between two rows however many share a timestamp. + * The id's own order is arbitrary (they are UUIDs); all a tiebreak needs is to + * be consistent, and the same expression drives both the ORDER BY and this. */ +export function afterCursor(sortKey: SQLWrapper, id: SQLWrapper, cursor: Cursor | null) { + if (cursor === null) return undefined; + // Spelled as a SQLite row value (3.15+) rather than expanded by hand into + // `sortKey < t OR (sortKey = t AND id < cursorId)`. The two are equivalent, + // but the expansion mentions `sortKey` twice, and `sortKey` is sometimes a + // COALESCE over two columns, so the hand-written form is both longer and the + // kind of thing that silently stops matching its own ORDER BY when one copy + // is edited. SQLite plans a row-value comparison against the same index it + // would use for the expansion. + return sql`(${sortKey}, ${id}) < (${cursor.t}, ${cursor.id})`; } +/** Turn an over-fetched row set into a page plus the cursor that continues it. + * + * The reads ask D1 for `limit + 1` rows and this discards the extra. That one + * wasted row is what lets `nextCursor` be null exactly when the list is + * exhausted, rather than null when a page came back short - which is a guess, + * and it is wrong precisely when the final page is exactly full. */ +export async function pageOf(d1: D1Database, rows: GameRow[], limit: number, sortValue: (row: GameRow) => number): Promise> { + const hasMore = rows.length > limit; + const kept = hasMore ? rows.slice(0, limit) : rows; + const last = kept.at(-1); + return { + rows: await withRosters(d1, kept), + nextCursor: hasMore && last !== undefined ? encodeCursor({ t: sortValue(last), id: last.id }) : null, + }; +} + +/** The sort value of a finished game: aborted rows carry no `finishedAt`, so + * they fall back to `updatedAt`. Defined once, because the SQL expression below + * and this must agree exactly or a cursor lands on the wrong row. */ +const finishedSortSql = sql`COALESCE(${games.finishedAt}, ${games.updatedAt})`; +const finishedSortValue = (row: GameRow): number => row.finishedAt ?? row.updatedAt; + /** The lobby page: public joinable games, newest first, exactly the shape * `idx_games_lobby` (the ported partial index) serves. When `caller` is given, * games seating anyone they have blocked (either direction) are hidden. The * creator counts as a participant, so this covers both games a blocked user * created and games they joined. */ -export async function readLobby(d1: D1Database, limit: number, cursor: number | null = null, caller?: string): Promise { +export async function readLobby(d1: D1Database, limit: number, cursor: Cursor | null = null, caller?: string): Promise> { const rows = await orm(d1) .select() .from(games) - .where(and(eq(games.access, "public"), inArray(games.status, ["waiting", "ready"]), caller === undefined ? undefined : noBlockedParticipant(d1, caller), olderThan(games.createdAt, cursor))) - .orderBy(desc(games.createdAt)) - .limit(limit) + .where(and(eq(games.access, "public"), inArray(games.status, ["waiting", "ready"]), caller === undefined ? undefined : noBlockedParticipant(d1, caller), afterCursor(games.createdAt, games.id, cursor))) + .orderBy(desc(games.createdAt), desc(games.id)) + .limit(limit + 1) .all(); - return await withRosters(d1, rows); + return await pageOf(d1, rows, limit, (row) => row.createdAt); } /** "My games" through the participants index (THE access path for * games-of-user). `active` = anything still alive; `finished` = the history * list, newest finish first (aborted rows carry no finishedAt, so they sort by * updatedAt). */ -export async function readMyGames(d1: D1Database, userId: string, bucket: "active" | "finished", limit: number, cursor: number | null = null): Promise { +export async function readMyGames(d1: D1Database, userId: string, bucket: "active" | "finished", limit: number, cursor: Cursor | null = null): Promise> { const db = orm(d1); const statuses: GameRow["status"][] = bucket === "active" ? ["waiting", "ready", "active"] : ["finished", "aborted"]; - const sortKey = bucket === "active" ? games.updatedAt : sql`COALESCE(${games.finishedAt}, ${games.updatedAt})`; - const order = desc(sortKey); + const active = bucket === "active"; + const sortKey = active ? games.updatedAt : finishedSortSql; + const sortValue = active ? (row: GameRow) => row.updatedAt : finishedSortValue; const rows = await db .select({ games }) .from(participants) .innerJoin(games, eq(participants.gameId, games.id)) - .where(and(eq(participants.userId, userId), inArray(games.status, statuses), olderThan(sortKey, cursor))) - .orderBy(order) - .limit(limit) + .where(and(eq(participants.userId, userId), inArray(games.status, statuses), afterCursor(sortKey, games.id, cursor))) + .orderBy(desc(sortKey), desc(games.id)) + .limit(limit + 1) .all(); - return await withRosters( + return await pageOf( d1, rows.map((r) => r.games), + limit, + sortValue, ); } @@ -216,19 +255,20 @@ export async function readMyGames(d1: D1Database, userId: string, bucket: "activ * a finished public game is already replayable by anyone who has its id. Same * participants index as `readMyGames`, matching either identity column so a * bot's game history works too. */ -export async function readPlayerPublicGames(d1: D1Database, playerId: string, limit: number, cursor: number | null = null): Promise { - const sortKey = sql`COALESCE(${games.finishedAt}, ${games.updatedAt})`; +export async function readPlayerPublicGames(d1: D1Database, playerId: string, limit: number, cursor: Cursor | null = null): Promise> { const rows = await orm(d1) .select({ games }) .from(participants) .innerJoin(games, eq(participants.gameId, games.id)) - .where(and(or(eq(participants.userId, playerId), eq(participants.botId, playerId)), eq(games.status, "finished"), eq(games.access, "public"), olderThan(sortKey, cursor))) - .orderBy(desc(sortKey)) - .limit(limit) + .where(and(or(eq(participants.userId, playerId), eq(participants.botId, playerId)), eq(games.status, "finished"), eq(games.access, "public"), afterCursor(finishedSortSql, games.id, cursor))) + .orderBy(desc(finishedSortSql), desc(games.id)) + .limit(limit + 1) .all(); - return await withRosters( + return await pageOf( d1, rows.map((r) => r.games), + limit, + finishedSortValue, ); } diff --git a/packages/server/src/d1/social.ts b/packages/server/src/d1/social.ts index b96b5de..f7e4f7d 100644 --- a/packages/server/src/d1/social.ts +++ b/packages/server/src/d1/social.ts @@ -12,10 +12,11 @@ * reusing the batch player projection. */ -import { and, desc, eq, inArray, lt, ne, notExists, or, sql } from "drizzle-orm"; +import { and, desc, eq, inArray, ne, notExists, or, sql } from "drizzle-orm"; +import type { Cursor, Page } from "../cursor.js"; import { noBlockedParticipant, pair, samePair } from "./blocks.js"; import { orm } from "./orm.js"; -import { type GameWithRoster, withRosters } from "./reads.js"; +import { afterCursor, type GameWithRoster, pageOf } from "./reads.js"; import { games, relationships, users } from "./schema.js"; /** The outcome of `sendFriendRequest`, telling the route what (if anything) to @@ -206,18 +207,18 @@ export async function acceptedFriendIds(d1: D1Database, userId: string): Promise return rows.map((r) => (r.a === userId ? r.b : r.a)); } -export async function friendsOpenGames(d1: D1Database, caller: string, limit: number, cursor: number | null = null): Promise { +export async function friendsOpenGames(d1: D1Database, caller: string, limit: number, cursor: Cursor | null = null): Promise> { const db = orm(d1); const friendIds = await acceptedFriendIds(d1, caller); - if (friendIds.length === 0) return []; + if (friendIds.length === 0) return { rows: [], nextCursor: null }; const rows = await db .select() .from(games) // A friend's game can still seat someone the caller has blocked, so filter // those out here too, not just in the plain lobby. - .where(and(inArray(games.createdBy, friendIds), inArray(games.status, ["waiting", "ready"]), noBlockedParticipant(d1, caller), cursor === null ? undefined : lt(games.createdAt, cursor))) - .orderBy(desc(games.createdAt)) - .limit(limit) + .where(and(inArray(games.createdBy, friendIds), inArray(games.status, ["waiting", "ready"]), noBlockedParticipant(d1, caller), afterCursor(games.createdAt, games.id, cursor))) + .orderBy(desc(games.createdAt), desc(games.id)) + .limit(limit + 1) .all(); - return withRosters(d1, rows); + return await pageOf(d1, rows, limit, (row) => row.createdAt); } diff --git a/packages/server/src/http.ts b/packages/server/src/http.ts index 3f26d9b..8f3fe58 100644 --- a/packages/server/src/http.ts +++ b/packages/server/src/http.ts @@ -33,7 +33,10 @@ export type ErrorCode = | "registrationRequired" | "imageTooLarge" | "unsupportedImageType" - | "rateLimited"; + | "rateLimited" + /** A pagination cursor that did not decode. Distinct because a client can + * act on it: discard the cursor and re-request the first page. */ + | "invalidCursor"; export class HttpError extends Error { readonly status: 400 | 401 | 403 | 404 | 409 | 413 | 415 | 422 | 429 | 500 | 502; diff --git a/packages/server/src/routes/games.ts b/packages/server/src/routes/games.ts index 56dd698..e5be8f8 100644 --- a/packages/server/src/routes/games.ts +++ b/packages/server/src/routes/games.ts @@ -18,6 +18,7 @@ import { HttpError, unwrap } from "../http.js"; import { gameInvitePush } from "../notify/push.js"; import type { Command, CommandResult } from "../protocol.js"; import { enforceRateLimit } from "../rate-limit.js"; +import { versionQuery } from "./query.js"; import { actionBody, addBotBody, commandAcceptedShape, createdShape, createGameBody, createSoloBody, errorShape, forfeitBody, frameShape, joinBody, joinByCodeBody, lobbyCommandBody, soloStartedShape } from "./wire.js"; // ── Route plumbing ──────────────────────────────────────────────────────────── @@ -520,8 +521,8 @@ export function registerGameRoutes(app: EngineApp, ctx: RouteContext): void { request: { params: gameIdParam, query: z.object({ - from: z.coerce.number().int().min(0).default(0), - to: z.coerce.number().int().min(0).optional(), + from: versionQuery.default(0).openapi({ type: "integer", minimum: 0, default: 0 }), + to: versionQuery.optional(), }), }, responses: responses(z.object({ frames: z.array(frameShape) }).openapi("Frames"), "The projected frames, version-ascending"), diff --git a/packages/server/src/routes/query.ts b/packages/server/src/routes/query.ts new file mode 100644 index 0000000..f2ef47e --- /dev/null +++ b/packages/server/src/routes/query.ts @@ -0,0 +1,78 @@ +/** + * The query parameters shared across the paged reads, and the one rule that + * shapes all of them: **a query parameter is parsed, never coerced**. + * + * That rule is here because breaking it cost every paged list in the product. + * `z.coerce.number()` is `Number()`, and `Number()` maps both `""` and `null` + * to `0`. Following that through four layers, each behaving correctly: + * + * 1. `z.coerce.number().int().min(0)` genuinely *accepts* `null` and yields + * `0`, so the emitted OpenAPI schema truthfully says `["integer", "null"]`. + * (With `.min(1)` it does not, which is why some parameters were affected + * and others were not - the difference looked like nothing at all.) + * 2. `openapi-generator` reads that nullability and correctly *omits* its + * `if (x != null)` guard, because the API has declared that null is a value + * it accepts. + * 3. dio renders the resulting null query value as a bare `?to=`. + * 4. The server coerces `""` straight back to `0`. + * + * Nothing in that chain is a bug in anyone's library. The whole failure is + * generated by asking `Number()` to be a parser. A cursor of `0` meant "older + * than anything that has ever happened", so the lobby, both `games/mine` + * buckets, the friends list and every history screen returned `200` with an + * empty array, and a `to` of `0` clamped replays to a single frame. + * + * Parsing instead of coercing fixes it at the origin and in both directions: + * the schema no longer accepts null, so the generated client omits the + * parameter on its own and the bad request is never built; and an empty or + * malformed value that arrives anyway is a loud `400` rather than a silent + * zero. Nothing downstream has to compensate, which is why there is no + * request-rewriting middleware in this codebase. + * + * They live in one module rather than beside their routes because the + * duplication was real and had already drifted: `reads.ts` declared a `limit` + * and a `cursor`, and `social.ts` carried an inline copy of both that any fix + * to the originals would have missed. + */ + +import { z } from "@hono/zod-openapi"; + +/** + * A non-negative integer arriving as a string, parsed strictly. + * + * The regex is the parser, and it is deliberately narrow: `Number()` also + * accepts `"1e3"` as 1000, `"0x10"` as 16, `" 5"` as 5 and `"1.0"` as 1, none + * of which any caller means to send and all of which it would silently honour. + * + * `.openapi()` restates the bounds because the runtime schema and the published + * one legitimately differ here: the wire carries an integer, and what arrives at + * the process boundary is the string spelling of one. Both are derived from the + * same two arguments, so they cannot drift apart. + */ +function intQuery(min: number, max: number) { + return z.string().regex(/^\d+$/, "must be a non-negative integer").transform(Number).pipe(z.number().int().min(min).max(max)).openapi({ type: "integer", minimum: min, maximum: max }); +} + +/** Page size. Bounded because it feeds `LIMIT` and, on the batch reads, an + * `IN (...)` list; unbounded, one request could ask D1 for everything. */ +export const limitQuery = intQuery(1, 50).default(20).openapi({ type: "integer", minimum: 1, maximum: 50, default: 20 }); + +/** A frame version: the bounds of a replay range. Zero is a legitimate value a + * caller can mean, which under coercion was indistinguishable from having sent + * nothing at all. */ +export const versionQuery = intQuery(0, Number.MAX_SAFE_INTEGER); + +/** + * Keyset cursor: the opaque `nextCursor` from the previous page, echoed back. + * Absent on the first page. + * + * A string with no coercion and no structure declared, because a client is not + * meant to build one - only to hand back what it was given. `cursor.ts` covers + * what is inside and why it is not a number. `min(1)` so that an empty cursor + * is refused by the schema rather than reaching the decoder as a mystery. + */ +export const cursorQuery = z.string().min(1).optional(); + +/** The continuation token for the next page, or null when the list is + * exhausted. Null is an answer, not a hint: see `Page` in `cursor.ts`. */ +export const nextCursorShape = z.string().nullable().openapi({ description: "Pass as `cursor` to fetch the next page. Null when there are no more results." }); diff --git a/packages/server/src/routes/reads.ts b/packages/server/src/routes/reads.ts index a34ae60..d6d16e7 100644 --- a/packages/server/src/routes/reads.ts +++ b/packages/server/src/routes/reads.ts @@ -5,9 +5,11 @@ */ import { createRoute, z } from "@hono/zod-openapi"; +import { decodeOptionalCursor } from "../cursor.js"; import { clampIds, readBots, readGame, readLobby, readMyGames, readPlayerPublicGames, readPlayers, readRatingHistory, readRatings } from "../d1/reads.js"; import type { EngineApp, RouteContext } from "../engine.js"; import { HttpError } from "../http.js"; +import { cursorQuery, limitQuery, nextCursorShape } from "./query.js"; import { botOf, botShape, errorShape, gameSummaryOf, gameSummaryShape, playerOf, playerShape, profileShape, sessionShape } from "./wire.js"; function okResponse(schema: T, description: string) { @@ -21,13 +23,6 @@ function okResponse(schema: T, description: string) { } as const; } -const limitQuery = z.coerce.number().int().min(1).max(50).default(20); - -/** Keyset cursor: the previous page's last sort value (epoch ms). Absent on the - * first page. Paging by cursor rather than offset keeps a page stable while the - * underlying list changes, which for a lobby it constantly does. */ -const cursorQuery = z.coerce.number().int().optional(); - export function registerReadRoutes(app: EngineApp, ctx: RouteContext): void { app.openapi( createRoute({ @@ -36,12 +31,12 @@ export function registerReadRoutes(app: EngineApp, ctx: RouteContext): void { operationId: "getLobby", tags: ["Games"], request: { query: z.object({ limit: limitQuery, cursor: cursorQuery }) }, - responses: okResponse(z.object({ games: z.array(gameSummaryShape) }).openapi("Lobby"), "Public joinable games, newest first"), + responses: okResponse(z.object({ games: z.array(gameSummaryShape), nextCursor: nextCursorShape }).openapi("Lobby"), "Public joinable games, newest first"), }), async (c) => { const { limit, cursor } = c.req.valid("query"); - const games = await readLobby(ctx.d1(c.env), limit, cursor ?? null, c.var.auth.user.id); - return c.json({ games: games.map(gameSummaryOf) }, 200); + const page = await readLobby(ctx.d1(c.env), limit, decodeOptionalCursor(cursor), c.var.auth.user.id); + return c.json({ games: page.rows.map(gameSummaryOf), nextCursor: page.nextCursor }, 200); }, ); @@ -53,12 +48,12 @@ export function registerReadRoutes(app: EngineApp, ctx: RouteContext): void { operationId: "getMyGames", tags: ["Games"], request: { query: z.object({ bucket: z.enum(["active", "finished"]).default("active"), limit: limitQuery, cursor: cursorQuery }) }, - responses: okResponse(z.object({ games: z.array(gameSummaryShape) }).openapi("MyGames"), "The caller's games in the requested bucket"), + responses: okResponse(z.object({ games: z.array(gameSummaryShape), nextCursor: nextCursorShape }).openapi("MyGames"), "The caller's games in the requested bucket"), }), async (c) => { const { bucket, limit, cursor } = c.req.valid("query"); - const games = await readMyGames(ctx.d1(c.env), c.var.auth.user.id, bucket, limit, cursor ?? null); - return c.json({ games: games.map(gameSummaryOf) }, 200); + const page = await readMyGames(ctx.d1(c.env), c.var.auth.user.id, bucket, limit, decodeOptionalCursor(cursor)); + return c.json({ games: page.rows.map(gameSummaryOf), nextCursor: page.nextCursor }, 200); }, ); @@ -172,12 +167,12 @@ export function registerReadRoutes(app: EngineApp, ctx: RouteContext): void { operationId: "getPlayerGames", tags: ["Players"], request: { params: z.object({ playerId: z.string().min(1) }), query: z.object({ limit: limitQuery, cursor: cursorQuery }) }, - responses: okResponse(z.object({ games: z.array(gameSummaryShape) }).openapi("PlayerGames"), "That player's finished public games, newest first"), + responses: okResponse(z.object({ games: z.array(gameSummaryShape), nextCursor: nextCursorShape }).openapi("PlayerGames"), "That player's finished public games, newest first"), }), async (c) => { const { limit, cursor } = c.req.valid("query"); - const games = await readPlayerPublicGames(ctx.d1(c.env), c.req.valid("param").playerId, limit, cursor ?? null); - return c.json({ games: games.map(gameSummaryOf) }, 200); + const page = await readPlayerPublicGames(ctx.d1(c.env), c.req.valid("param").playerId, limit, decodeOptionalCursor(cursor)); + return c.json({ games: page.rows.map(gameSummaryOf), nextCursor: page.nextCursor }, 200); }, ); @@ -218,7 +213,7 @@ export function registerReadRoutes(app: EngineApp, ctx: RouteContext): void { path: "/me/rating-history", operationId: "getMyRatingHistory", tags: ["Me"], - request: { query: z.object({ pool: z.string().optional(), limit: limitQuery }) }, + request: { query: z.object({ pool: z.string().min(1).optional(), limit: limitQuery }) }, responses: okResponse(z.object({ history: z.array(historyShape) }).openapi("RatingHistory"), "The caller's rating log, newest first"), }), async (c) => { diff --git a/packages/server/src/routes/social.ts b/packages/server/src/routes/social.ts index 5a67e67..a6d1e2c 100644 --- a/packages/server/src/routes/social.ts +++ b/packages/server/src/routes/social.ts @@ -13,12 +13,14 @@ */ import { createRoute, z } from "@hono/zod-openapi"; +import { decodeOptionalCursor } from "../cursor.js"; import { readPlayers } from "../d1/reads.js"; import { acceptFriendRequest, blockUser, friendsOpenGames, listFriends, listPendingRequests, removeRelationship, searchUsers, sendFriendRequest, unblockUser } from "../d1/social.js"; import type { Authed, EngineApp, RouteContext } from "../engine.js"; import { HttpError } from "../http.js"; import { friendAcceptedPush, friendRequestPush } from "../notify/push.js"; import { enforceRateLimit } from "../rate-limit.js"; +import { cursorQuery, limitQuery, nextCursorShape } from "./query.js"; import { errorShape, friendRequestShape, friendShape, friendTargetBody, gameSummaryOf, gameSummaryShape, playerShape } from "./wire.js"; const err = (what: string) => ({ content: { "application/json": { schema: errorShape } }, description: what }); @@ -64,12 +66,13 @@ export function registerSocialRoutes(app: EngineApp, ctx: RouteContext): void { path: "/friends/games", operationId: "getFriendsGames", tags: ["Social"], - request: { query: z.object({ limit: z.coerce.number().int().min(1).max(50).default(20), cursor: z.coerce.number().int().optional() }) }, - responses: okResponse(z.object({ games: z.array(gameSummaryShape) }).openapi("FriendsGames"), "Joinable games created by the caller's friends"), + request: { query: z.object({ limit: limitQuery, cursor: cursorQuery }) }, + responses: okResponse(z.object({ games: z.array(gameSummaryShape), nextCursor: nextCursorShape }).openapi("FriendsGames"), "Joinable games created by the caller's friends"), }), async (c) => { - const rows = await friendsOpenGames(ctx.d1(c.env), c.var.auth.user.id, c.req.valid("query").limit, c.req.valid("query").cursor ?? null); - return c.json({ games: rows.map(gameSummaryOf) }, 200); + const { limit, cursor } = c.req.valid("query"); + const page = await friendsOpenGames(ctx.d1(c.env), c.var.auth.user.id, limit, decodeOptionalCursor(cursor)); + return c.json({ games: page.rows.map(gameSummaryOf), nextCursor: page.nextCursor }, 200); }, ); @@ -80,7 +83,7 @@ export function registerSocialRoutes(app: EngineApp, ctx: RouteContext): void { path: "/users/search", operationId: "searchUsers", tags: ["Social"], - request: { query: z.object({ q: z.string().min(1), limit: z.coerce.number().int().min(1).max(50).default(20) }) }, + request: { query: z.object({ q: z.string().min(1), limit: limitQuery }) }, responses: okResponse(z.object({ users: z.array(playerShape) }).openapi("UserSearch"), "Matching registered users, best match first"), }), async (c) => { diff --git a/packages/server/src/routes/wire.ts b/packages/server/src/routes/wire.ts index 29e1f68..7c84964 100644 --- a/packages/server/src/routes/wire.ts +++ b/packages/server/src/routes/wire.ts @@ -57,6 +57,7 @@ const errorCodeDocs: Record = { imageTooLarge: "The uploaded avatar exceeds the size limit", unsupportedImageType: "The uploaded avatar is not an accepted image type", rateLimited: "Too many requests in a short window; retry after the interval in the Retry-After header", + invalidCursor: "The pagination cursor did not decode; drop it and request the first page", }; /** The closed set of stable error codes, published as an enum so a client can diff --git a/packages/server/test/engine.spec.ts b/packages/server/test/engine.spec.ts index 92635b7..77ef9cd 100644 --- a/packages/server/test/engine.spec.ts +++ b/packages/server/test/engine.spec.ts @@ -281,6 +281,16 @@ describe("active play & frames", () => { const ratings = await json<{ ratings: { pool: string }[] }>(await api(u.b, "GET", "/me/ratings")); expect(ratings.ratings).toHaveLength(1); + // The two parameters that used to fail silently on an empty value: `?pool=` + // became `WHERE pool = ''` and matched nothing, and `?to=` became `to: 0`, + // which clamped the replay to a single frame. Both are now refused outright + // rather than answered with a plausible, wrong 200. + expect((await api(u.b, "GET", "/me/rating-history?pool=")).status).toBe(400); + expect((await api(u.c, "GET", `/games/${gameId}/frames?from=0&to=`)).status).toBe(400); + // And the range still works when it is actually given. + const ranged = await json<{ frames: { version: number }[] }>(await api(u.c, "GET", `/games/${gameId}/frames?from=0&to=10`)); + expect(ranged.frames.map((f) => f.version)).toEqual([0, 1, 2, 3]); + // The summary carries every identity's delta, so a history list needs no // second read to annotate a row, and the same projection is correct when // viewing someone else's games, since this is per-game not per-viewer. @@ -520,6 +530,46 @@ describe("reads", () => { expect(notMine.games.some((g) => g.id === gameId)).toBe(false); }); + // Absent is the first page. Empty is a MALFORMED request, and the difference + // between those two is the whole story of this bug: `Number("")` is 0, so an + // empty cursor used to parse as a real cursor of zero, which is older than + // every timestamp there has ever been. The lobby, both my-games buckets and + // the friends list all returned 200 with an empty list. Nothing coerces now, + // so the same request is a 400 that names itself. + it("serves the first page when a cursor is absent", async () => { + const u = makeUsers(); + const { gameId } = await createGame(u.a, { rated: false }); + + const lobby = await json<{ games: { id: string }[] }>(await api(u.c, "GET", "/lobby")); + expect(lobby.games.some((g) => g.id === gameId)).toBe(true); + const mine = await json<{ games: { id: string }[] }>(await api(u.a, "GET", "/games/mine?bucket=active")); + expect(mine.games.some((g) => g.id === gameId)).toBe(true); + }); + + it.each(["/lobby?cursor=", "/lobby?limit=", "/games/mine?cursor=", "/games/mine?bucket=active&cursor=", "/me/rating-history?pool=", "/players/x/games?cursor="])("refuses an empty query value rather than reading it as zero (%s)", async (path) => { + const u = makeUsers(); + expect((await api(u.a, "GET", path)).status).toBe(400); + }); + + // The other half of the same rule: a cursor that is actually sent still pages. + // Without this, "ignore the cursor entirely" would pass the test above. + it("a server-issued cursor excludes everything on the page it came from", async () => { + const u = makeUsers(); + const older = await createGame(u.a, { rated: false }); + const newer = await createGame(u.a, { rated: false }); + + const page = await json<{ games: { id: string }[]; nextCursor: string | null }>(await api(u.a, "GET", "/games/mine?bucket=active&limit=1")); + expect(page.games).toHaveLength(1); + expect(page.nextCursor).not.toBeNull(); + + const next = await json<{ games: { id: string }[]; nextCursor: string | null }>(await api(u.a, "GET", `/games/mine?bucket=active&limit=1&cursor=${encodeURIComponent(page.nextCursor ?? "")}`)); + expect(next.games).toHaveLength(1); + expect(next.games[0].id).not.toBe(page.games[0].id); + // The two pages together are the two games, each exactly once. + expect(new Set([page.games[0].id, next.games[0].id])).toEqual(new Set([older.gameId, newer.gameId])); + expect(next.nextCursor).toBeNull(); + }); + it("players batch endpoint returns public identity only", async () => { const u = makeUsers(); await api(u.a, "GET", "/me"); diff --git a/packages/server/test/query-contract.spec.ts b/packages/server/test/query-contract.spec.ts new file mode 100644 index 0000000..2b0c7a3 --- /dev/null +++ b/packages/server/test/query-contract.spec.ts @@ -0,0 +1,68 @@ +/** + * One invariant over the whole published surface: **no query parameter may be + * nullable.** + * + * This is the guard that replaced a request-rewriting middleware, and it is a + * better guard because it fails at build time instead of compensating at + * runtime. The bug it exists to prevent is not obvious from any single line of + * code, because every layer involved behaves correctly: + * + * `z.coerce.number()` is `Number()`, and `Number(null)` is `0`. So + * `z.coerce.number().int().min(0)` genuinely accepts null, the emitted schema + * honestly reports `["integer", "null"]`, and `openapi-generator` correctly + * drops its `if (x != null)` guard - the API said null was welcome. dio then + * renders that null as a bare `?to=`, which the server coerces straight back to + * `0`. A caller asking for the first page of a list gets an empty one, with a + * 200, and nothing anywhere logs a problem. + * + * The tell is upstream of all of it and is visible right here: a query + * parameter that accepts null. A parameter can be *optional* - you may omit it + * - without being *nullable* - you may send it as null. Conflating those is + * what generates the whole chain, and `.min(1)` versus `.min(0)` was enough to + * flip it, which is not a difference anyone would notice in review. + */ + +import { describe, expect, it } from "vitest"; +import { openApiDocument } from "../src/openapi.js"; + +interface QueryParam { + operationId: string; + name: string; + type: unknown; +} + +function queryParams(): QueryParam[] { + const doc = openApiDocument("0.0.0-test") as unknown as { + paths: Record>; + }; + const found: QueryParam[] = []; + for (const operations of Object.values(doc.paths)) { + for (const operation of Object.values(operations)) { + for (const parameter of operation.parameters ?? []) { + if (parameter.in === "query") found.push({ operationId: operation.operationId ?? "?", name: parameter.name, type: parameter.schema?.type }); + } + } + } + return found; +} + +describe("query parameter contract", () => { + it("finds the query parameters at all", () => { + // Guards the guard: if the document shape ever changes under this, the + // assertion below would pass by inspecting nothing. + expect(queryParams().length).toBeGreaterThan(10); + }); + + it("declares no query parameter as nullable", () => { + const nullable = queryParams().filter((p) => Array.isArray(p.type) && (p.type as unknown[]).includes("null")); + expect(nullable.map((p) => `${p.operationId}.${p.name}`)).toEqual([]); + }); + + // The mechanism, pinned separately so the diagnosis survives even if someone + // "fixes" the assertion above by hand-editing a type. + it("is what z.coerce would have broken", async () => { + const { z } = await import("zod"); + expect(z.coerce.number().int().min(0).safeParse(null).success).toBe(true); + expect(z.coerce.number().int().min(0).safeParse("").success).toBe(true); + }); +}); diff --git a/packages/server/test/social.spec.ts b/packages/server/test/social.spec.ts index 576c079..0c6e952 100644 --- a/packages/server/test/social.spec.ts +++ b/packages/server/test/social.spec.ts @@ -159,26 +159,71 @@ describe("username edit", () => { }); describe("keyset pagination", () => { - it("pages my games by cursor without repeating or skipping a row", async () => { + interface PageBody { + games: { id: string; updatedAt: number }[]; + nextCursor: string | null; + } + + /** Walk every page the way a client does: follow `nextCursor` until it is + * null, never deriving a cursor from a row. */ + async function drain(who: TestTokenOptions, path: string, limit: number): Promise { + const seen: string[] = []; + let cursor: string | null = null; + for (let guard = 0; guard < 20; guard++) { + const suffix: string = cursor === null ? "" : `&cursor=${encodeURIComponent(cursor)}`; + const page: PageBody = await json(await api(who, "GET", `${path}?limit=${limit}${suffix}`)); + seen.push(...page.games.map((g) => g.id)); + if (page.nextCursor === null) return seen; + cursor = page.nextCursor; + } + throw new Error("pagination did not terminate"); + } + + // These games are created in a tight loop, so they very often share an + // `updatedAt` millisecond. That tie is the whole point: under a cursor that + // was the bare sort value, a page boundary landing between two rows with the + // same timestamp dropped one permanently, because it was neither strictly + // older than the cursor nor on the page already served. The cursor now + // carries the row id as a tiebreak, so every row is on exactly one page. + it("pages my games without repeating or skipping a row, including across a timestamp tie", async () => { const a = await user("a"); const created: string[] = []; - for (let i = 0; i < 3; i++) { + for (let i = 0; i < 5; i++) { const game = await json<{ gameId: string }>(await api(a, "POST", "/games", { access: "public", schemaVersion: 1, config: { target: 3 }, minPlayers: 2, maxPlayers: 2, rated: false }), 201); created.push(game.gameId); } - const first = await json<{ games: { id: string; updatedAt: number }[] }>(await api(a, "GET", "/games/mine?limit=2")); + const first = await json(await api(a, "GET", "/games/mine?limit=2")); expect(first.games).toHaveLength(2); + // The tie this test exists for. If the clock happened to tick between every + // create the assertion below still holds, it is just less interesting. + const timestamps = new Set(first.games.map((g) => g.updatedAt)); + expect(timestamps.size).toBeGreaterThanOrEqual(1); + + const seen = await drain(a, "/games/mine", 2); + expect(seen).toHaveLength(new Set(seen).size); // no row served twice + expect(new Set(seen)).toEqual(new Set(created)); // and none skipped + }); - const cursor = first.games[1].updatedAt; - const second = await json<{ games: { id: string }[] }>(await api(a, "GET", `/games/mine?limit=2&cursor=${cursor}`)); + // `nextCursor` is an answer, not a hint. The list here is exactly as long as + // the page size, the case where "stop when a page comes back short" - the + // heuristic this replaced - would have asked for one page too many. + it("reports nextCursor null when the last page is exactly full", async () => { + const a = await user("a"); + for (let i = 0; i < 2; i++) { + await json<{ gameId: string }>(await api(a, "POST", "/games", { access: "public", schemaVersion: 1, config: { target: 3 }, minPlayers: 2, maxPlayers: 2, rated: false }), 201); + } - // Strictly older than the cursor: no overlap with the first page, and the - // remaining game is present rather than skipped. - const firstIds = first.games.map((g) => g.id); - const secondIds = second.games.map((g) => g.id); - expect(secondIds.some((id) => firstIds.includes(id))).toBe(false); - expect(new Set([...firstIds, ...secondIds]).size).toBe(created.length); + const page = await json(await api(a, "GET", "/games/mine?limit=2")); + expect(page.games).toHaveLength(2); + expect(page.nextCursor).toBeNull(); + }); + + it("refuses a cursor that did not come from the server", async () => { + const a = await user("a"); + const res = await api(a, "GET", "/games/mine?limit=2&cursor=not-a-cursor"); + expect(res.status).toBe(400); + expect(((await res.json()) as { code: string }).code).toBe("invalidCursor"); }); });