Refactor key types #1917
Refactor key types #1917
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1917 +/- ##
==========================================
- Coverage 99.03% 99.03% -0.01%
==========================================
Files 18 18
Lines 1449 1447 -2
Branches 305 303 -2
==========================================
- Hits 1435 1433 -2
Misses 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe changes overhaul the API key management types and methods in the codebase. Types related to API keys are refactored and moved to a new module, and the MeiliSearch class methods are updated to use these new types. The test suite for API key management is rewritten to use data-driven parameterized tests and new assertion helpers. Utility functions for typed object key and entry operations are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MeiliSearch
participant HTTP_API
Client->>MeiliSearch: getKeys(listApiKeys)
MeiliSearch->>HTTP_API: GET /keys with params
HTTP_API-->>MeiliSearch: KeyViewList (raw)
MeiliSearch-->>Client: KeyViewList
Client->>MeiliSearch: createKey(createApiKey)
MeiliSearch->>HTTP_API: POST /keys with payload
HTTP_API-->>MeiliSearch: KeyView (raw)
MeiliSearch-->>Client: KeyView
Client->>MeiliSearch: updateKey(keyUid, patchApiKey)
MeiliSearch->>HTTP_API: PATCH /keys/:keyUid
HTTP_API-->>MeiliSearch: KeyView (raw)
MeiliSearch-->>Client: KeyView
Client->>MeiliSearch: getKey(keyUid)
MeiliSearch->>HTTP_API: GET /keys/:keyUid
HTTP_API-->>MeiliSearch: KeyView (raw)
MeiliSearch-->>Client: KeyView
Client->>MeiliSearch: deleteKey(keyUid)
MeiliSearch->>HTTP_API: DELETE /keys/:keyUid
HTTP_API-->>MeiliSearch: void
MeiliSearch-->>Client: void
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/keys.test.ts (7)
17-18: Reduce brittleness of the structural assertionHard-coding the exact property count (
9) makes the test fragile:
any harmless future addition of a field inKeyViewwill break the whole suite.-extAssert.lengthOf(Object.keys(value), 9); +extAssert.isAtLeast(Object.keys(value).length, 9);By checking at least the required fields you still guarantee the current contract while allowing non-breaking extensions.
30-37: Nit: consolidate repeating assertionsThe
nameanddescriptionchecks duplicate the same null-or-string logic.
Packaging them in a tiny helper (e.g.isNullableString) keepsisKeyViewshort and self-documenting.- extAssert( - name === null || typeof name === "string", - "expected name to be null or string", - ); + const isNullableString = (v: unknown) => + v === null || typeof v === "string"; + extAssert(isNullableString(name), "expected name to be null or string");
55-56: Consider validating ISO-8601 timestamps
createdAtandupdatedAtmust be ISO strings, not just any string.-extAssert.typeOf(createdAt, "string"); -extAssert.typeOf(updatedAt, "string"); +const isoRegex = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d+Z$/; +extAssert.match(createdAt, isoRegex); +extAssert.match(updatedAt, isoRegex);This prevents silent acceptance of malformed dates.
60-63: Shadowing may confuse intellisenseRe-exporting
assertwith additional methods is handy but can bewilder IDE
resolution (assertnow has more members than vitest’s type knows).If you keep this pattern often, consider creating a dedicated assertion module
(e.g.tests/assert.ts) that explicitly merges and exports once, then import it everywhere.
178-182: Mutation inside data-setup loop obscures intentMutating the
nameelement (testValue[0]) in-place makes the data harder to
follow. Compute the formatted title on the fly to keep the source immutable:-for (const testValues of Object.values(testRecord)) { - for (const testValue of testValues) { - testValue[0] = testValue[0] === undefined ? "" : ` with "${testValue[0]}"`; - } -} +const formatTitle = (raw: string | undefined) => + raw === undefined ? "" : ` with "${raw}"`;Then call
formatTitle(...)where you build the test name.
184-184: Top-levelawaitties the file to ESM
await getClient()at the file root requires ESM support in the test runner.
Vitest handles this, but the pattern prevents running the same tests in other
runners (e.g. Jest) or under CommonJS tools.Safest alternative:
let ms: MeiliSearch; beforeAll(async () => { ms = await getClient("Master"); });
190-195: Duplicate object keys rely on evaluation orderWhen
key === "actions"(orexpiresAt) the spread later in the literal
overrides earlier values. While it currently works, the mix of defaults plus
conditional overwrite invites subtle bugs.Consider constructing the request with a shallow merge:
const body: CreateApiKey = { actions: ["*"], indexes: ["*"], expiresAt: null, ...({ [key]: value } as Partial<CreateApiKey>), }; const keyView = await ms.createKey(body);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
src/meilisearch.ts(2 hunks)src/types/index.ts(1 hunks)src/types/keys.ts(1 hunks)src/types/types.ts(0 hunks)tests/keys.test.ts(1 hunks)tests/utils/meilisearch-test-utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/types/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/meilisearch.ts (1)
src/types/keys.ts (5)
ListApiKeys(64-67)KeyViewList(78-78)KeyView(47-57)CreateApiKey(33-40)PatchApiKey(85-85)
src/types/keys.ts (1)
src/http-requests.ts (1)
T(199-264)
tests/keys.test.ts (2)
src/types/keys.ts (4)
KeyView(47-57)CreateApiKey(33-40)Action(12-26)ListApiKeys(64-67)tests/utils/meilisearch-test-utils.ts (4)
assert(131-134)objectKeys(257-257)getClient(269-269)objectEntries(258-258)
🔇 Additional comments (18)
tests/utils/meilisearch-test-utils.ts (3)
247-249: Well-implemented type-safe helper function.This is a useful utility function that extracts keys from an object with string literal keys, returning them as a typed array. This ensures type safety when working with object keys in tests.
251-254: Nice type-safety enhancement for Object.entries.This typed alias for
Object.entriesis a good addition that preserves precise key and value typings for record types, which the nativeObject.entriesdoesn't provide. The proper typing will help catch potential errors during development.
257-258: Good export of new utility functions.Properly exposing the new utility functions through exports makes them available for use in test files.
src/types/index.ts (1)
2-2: Properly exports the new keys module.The addition of this export statement ensures that the newly created key-related types from
keys.jsare accessible through the central types index, which is essential for their use throughout the codebase.src/types/keys.ts (8)
1-6: Good use of type aliases for better readability.The use of type aliases for common string literals improves code readability and maintainability. These will be useful when composing the more complex
Actiontype.
12-26: Well-structured permission actions with template literal types.The
Actiontype effectively uses template literal types to create a comprehensive and type-safe representation of all possible API key permissions. This approach is more precise than using simple string literals and helps prevent errors when specifying permissions.
33-40: Clear API key creation structure with proper documentation.The
CreateApiKeytype accurately represents the payload structure required to create a new API key, with proper JSDoc documentation linking to official resources. Note that theexpiresAtfield is now a string instead ofDate, which is a breaking change as mentioned in the PR description.
47-57: Good API key view model with string timestamp representation.The
KeyViewtype correctly represents the API key object returned by the server. The timestamps (createdAt,updatedAt,expiresAt) are defined as strings rather thanDateobjects, which aligns with the server response but is a breaking change for consumers that expectDateobjects. This change is mentioned in the PR description.
64-67: Simple and effective pagination parameters.The
ListApiKeystype provides a clean interface for optional pagination parameters when listing keys.
69-75: Reusable pagination view type.Creating a generic
PaginationView<T>type is a good practice for reusability across different paginated resources. This approach allows for consistent pagination handling across the API.
77-78: Concise key list type using composition.Using the generic
PaginationView<T>to create theKeyViewListtype is a good example of type composition. This approach makes the code more maintainable and ensures consistency.
80-85: Efficient use of Pick for limited updates.The
PatchApiKeytype efficiently usesPickto limit updatable fields to justnameanddescription. This approach ensures type safety by preventing updates to fields that shouldn't be modified.src/meilisearch.ts (6)
10-31: Updated imports to include the new key-related types.The import statement has been properly updated to include the new key-related types from the keys module. Type imports are organized logically, which helps with code readability.
306-311: Updated getKeys method with new type signatures.The
getKeysmethod has been updated to use the newListApiKeysandKeyViewListtypes, which provide more precise type checking. The function documentation has been updated to use a concise link to the official API reference, which is a good practice.
314-319: Updated getKey method with new return type.The
getKeymethod now returnsKeyViewinstead of the deprecatedKeytype, aligning with the new type system. The function documentation has been updated to use a concise link to the official API reference.
321-327: Updated createKey method with new parameter and return types.The
createKeymethod now acceptsCreateApiKeyand returnsKeyView, aligning with the new type system. The function documentation has been updated to use a concise link to the official API reference.
329-338: Updated updateKey method with new parameter and return types.The
updateKeymethod now acceptskeyOrUidandPatchApiKeyparameters and returnsKeyView, aligning with the new type system. The function documentation has been updated to use a concise link to the official API reference.
340-340: Consistent documentation style for deleteKey method.The
deleteKeymethod's documentation has been updated to match the concise style of the other key-related methods, using a direct link to the official API reference. This ensures documentation consistency across related methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/keys.test.ts (4)
16-59: Well-structured custom assertion helperThe
isKeyViewhelper effectively encapsulates validation logic for API key objects, making tests more maintainable and consistent. This is a good application of the DRY principle.A minor improvement would be to replace the individual field type assertions with a more generic property type validation approach to reduce repetition when checking string/null fields.
- extAssert( - name === null || typeof name === "string", - "expected name to be null or string", - ); - extAssert( - description === null || typeof description === "string", - "expected description to be null or string", - ); - - extAssert.typeOf(key, "string"); - extAssert.typeOf(uid, "string"); - - for (const action of actions) { - extAssert.oneOf(action, possibleActions); - } - - for (const index of indexes) { - extAssert.typeOf(index, "string"); - } - - extAssert( - expiresAt === null || typeof expiresAt === "string", - "expected expiresAt to be null or string", - ); - - extAssert.typeOf(createdAt, "string"); - extAssert.typeOf(updatedAt, "string"); + // Helper for nullable string validation + const isNullableString = (val: unknown, fieldName: string) => + extAssert( + val === null || typeof val === "string", + `expected ${fieldName} to be null or string` + ); + + // Validate nullable string fields + isNullableString(name, "name"); + isNullableString(description, "description"); + isNullableString(expiresAt, "expiresAt"); + + // Validate string fields + extAssert.typeOf(key, "string"); + extAssert.typeOf(uid, "string"); + extAssert.typeOf(createdAt, "string"); + extAssert.typeOf(updatedAt, "string"); + + // Validate arrays + actions.forEach(action => extAssert.oneOf(action, possibleActions)); + indexes.forEach(index => extAssert.typeOf(index, "string"));
88-176: Comprehensive test data for API key propertiesThe test record provides a thorough set of test cases for all key properties, creating a solid foundation for data-driven tests. The assertions are correctly matched to each property's type.
However, there's a potential confusion with the
expiresAtassertion where the non-null assertion operator (!) is used. It would be more type-safe to check for null first.(a, b) => { - assert.strictEqual(Date.parse(a!), Date.parse(b!)); + assert.strictEqual( + a === null ? null : Date.parse(a), + b === null ? null : Date.parse(b) + ); },
178-183: Test name transformation could be more flexibleThis section transforms test names, but it makes assumptions about the structure of test values that might not hold if the data format changes.
Consider creating a more explicit mapping function that handles different test value formats more predictably:
// transform names -for (const testValues of Object.values(testRecord)) { - for (const testValue of testValues) { - testValue[0] = testValue[0] === undefined ? "" : ` with "${testValue[0]}"`; - } -} +function formatTestName(name: string | undefined): string { + return name === undefined ? "" : ` with "${name}"`; +} + +// Apply formatting to all test names +for (const testValues of Object.values(testRecord)) { + for (const testValue of testValues) { + testValue[0] = formatTestName(testValue[0]); + } +}
223-250: Combined test for multiple API operationsThis test combines verification of
getKeys,getKey, anddeleteKeyin a single test case. While functional, it makes tracking failures more difficult as a failure in one operation will obscure whether the others would have passed.Consider separating these into individual test cases, potentially with a shared setup:
-test(`${ms.getKeys.name}, ${ms.getKey.name} and ${ms.deleteKey.name} methods`, async () => { +test(`${ms.getKeys.name} should return a valid key list with correct pagination`, async () => { const keyList = await ms.getKeys({ offset: 0, limit: 10_000, } satisfies Required<ListApiKeys>); + assert.lengthOf(Object.keys(keyList), 4); + const { results, offset, limit, total } = keyList; + + for (const keyView of results) { + assert.isKeyView(keyView); + } + + assert.typeOf(offset, "number"); + assert.typeOf(limit, "number"); + assert.typeOf(total, "number"); +}); + +test(`${ms.getKey.name} should return valid individual keys`, async () => { + const keyList = await ms.getKeys({ + limit: 10_000, + }); + for (const { uid, name } of keyList.results) { const keyView = await ms.getKey(uid); + assert.isKeyView(keyView); + } +}); + +test(`${ms.deleteKey.name} should delete non-default keys`, async () => { + const keyList = await ms.getKeys({ + limit: 10_000, + }); + + for (const { uid, name } of keyList.results) { // avoid deleting default keys that might be used by other tests if (name !== "Default Search API Key" && name !== "Default Admin API Key") { await ms.deleteKey(uid); } } - - assert.lengthOf(Object.keys(keyList), 4); - const { results, offset, limit, total } = keyList; - - for (const keyView of results) { - assert.isKeyView(keyView); - } - - assert.typeOf(offset, "number"); - assert.typeOf(limit, "number"); - assert.typeOf(total, "number"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/keys.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/keys.test.ts (2)
src/types/keys.ts (4)
KeyView(47-57)CreateApiKey(33-40)Action(12-26)ListApiKeys(64-67)tests/utils/meilisearch-test-utils.ts (4)
assert(131-134)objectKeys(257-257)getClient(269-269)objectEntries(258-258)
🔇 Additional comments (4)
tests/keys.test.ts (4)
66-66: UUID is now dynamically generatedGreat! Using
randomUUID()addresses the previous review comment about potential collisions between test runs.
68-86: Excellent type definitions for test dataThe
TestRecordandSimplifiedTestRecordtypes provide strong typing for the test cases, ensuring that test data structure matches the API requirements. This level of type safety helps catch errors during development rather than at runtime.
187-203: Well-structured parameterized tests for createKeyUsing
describe.forandtest.forwithobjectEntriesis an excellent approach for data-driven testing. The tests are concise and clearly validate that each property is correctly handled when creating keys.
205-221:✅ Verification successful
Limited updateKey testing scope
The test only verifies
nameanddescriptionproperties forupdateKey, but doesn't explain why other properties aren't tested. This might not represent all valid update scenarios.Consider adding a comment explaining why only these properties are tested, or expand the test to include other updateable properties. Also verify which properties can be updated through the API:
🏁 Script executed:
#!/bin/bash # Check which properties are allowed in the PatchApiKey type definition rg -A 10 "export\s+type\s+PatchApiKey" src/Length of output: 133
updateKey tests cover all permitted fields
The
PatchApiKeytype only includesnameanddescription(seesrc/types/keys.ts), so the existing tests already cover every updateable property. No changes needed.
| ); | ||
| expect(updatedKey).toHaveProperty("name", "Product admin"); | ||
| }); | ||
| assert.isKeyView(keyView); | ||
|
|
||
| test(`${permission} key: delete a key`, async () => { | ||
| const client = await getClient(permission); | ||
| assertion(keyView[key as keyof typeof keyView], value); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| const key = await client.createKey({ | ||
| description: "Indexing Products API key", | ||
| actions: ["documents.add"], | ||
| indexes: ["products"], | ||
| expiresAt: new Date("2050-11-13T00:00:00Z"), // Test will fail in 2050 | ||
| }); | ||
| test(`${ms.getKeys.name}, ${ms.getKey.name} and ${ms.deleteKey.name} methods`, async () => { | ||
| const keyList = await ms.getKeys({ | ||
| offset: 0, | ||
| limit: 10_000, | ||
| } satisfies Required<ListApiKeys>); | ||
|
|
||
| const deletedKey = await client.deleteKey(key.key); | ||
| for (const { uid, name } of keyList.results) { | ||
| const keyView = await ms.getKey(uid); | ||
|
|
||
| expect(deletedKey).toBeUndefined(); | ||
| }); | ||
| }, | ||
| ); | ||
| // avoid deleting default keys that might be used by other tests | ||
| if (name !== "Default Search API Key" && name !== "Default Admin API Key") { | ||
| await ms.deleteKey(uid); | ||
| } | ||
|
|
||
| describe.each([{ permission: "Search" }])( | ||
| "Test on keys with search key", | ||
| ({ permission }) => { | ||
| test(`${permission} key: get keys denied`, async () => { | ||
| const client = await getClient(permission); | ||
| await expect(client.getKeys()).rejects.toHaveProperty( | ||
| "cause.code", | ||
| ErrorStatusCode.INVALID_API_KEY, | ||
| ); | ||
| }); | ||
| assert.isKeyView(keyView); | ||
| } | ||
|
|
||
| test(`${permission} key: create key denied`, async () => { | ||
| const client = await getClient(permission); | ||
| await expect( | ||
| client.createKey({ | ||
| description: "Indexing Products API key", | ||
| actions: ["documents.add"], | ||
| indexes: ["products"], | ||
| expiresAt: null, | ||
| }), | ||
| ).rejects.toHaveProperty("cause.code", ErrorStatusCode.INVALID_API_KEY); | ||
| }); | ||
| }, | ||
| ); | ||
| assert.lengthOf(Object.keys(keyList), 4); | ||
| const { results, offset, limit, total } = keyList; | ||
|
|
||
| describe.each([{ permission: "No" }])( | ||
| "Test on keys with No key", | ||
| ({ permission }) => { | ||
| test(`${permission} key: get keys denied`, async () => { | ||
| const client = await getClient(permission); | ||
| await expect(client.getKeys()).rejects.toHaveProperty( | ||
| "cause.code", | ||
| ErrorStatusCode.MISSING_AUTHORIZATION_HEADER, | ||
| ); | ||
| }); | ||
| for (const keyView of results) { | ||
| assert.isKeyView(keyView); | ||
| } | ||
|
|
||
| test(`${permission} key: create key denied`, async () => { | ||
| const client = await getClient(permission); | ||
| await expect( | ||
| client.createKey({ | ||
| description: "Indexing Products API key", | ||
| actions: ["documents.add"], | ||
| indexes: ["products"], | ||
| expiresAt: null, | ||
| }), | ||
| ).rejects.toHaveProperty( | ||
| "cause.code", | ||
| ErrorStatusCode.MISSING_AUTHORIZATION_HEADER, | ||
| ); | ||
| }); | ||
| }, | ||
| ); | ||
| assert.typeOf(offset, "number"); | ||
| assert.typeOf(limit, "number"); | ||
| assert.typeOf(total, "number"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling tests are missing
The test suite thoroughly tests the happy paths but lacks tests for error scenarios, such as invalid inputs or unauthorized access.
Consider adding tests for error cases like:
- Creating a key with invalid properties
- Updating a non-existent key
- Accessing a key with insufficient permissions
- Using invalid pagination parameters
Example structure:
test('createKey should fail with invalid properties', async () => {
await assert.rejects(
async () => ms.createKey({
actions: ["invalid_action"],
indexes: ["*"],
expiresAt: null
}),
(err) => {
assert.include(err.message, 'invalid_action');
return true;
}
);
});🤖 Prompt for AI Agents
In tests/keys.test.ts around lines 1 to 250, the test suite currently covers
only successful scenarios and lacks tests for error handling. Add new tests to
cover error cases such as creating a key with invalid properties, updating a
non-existent key, accessing a key without proper permissions, and using invalid
pagination parameters. For each, write tests that call the relevant method with
invalid inputs and assert that the promise rejects with an error containing
expected messages, using assert.rejects or equivalent to verify proper error
handling.
Pull Request
What does this PR do?
Caution
Breaking changes
Datetypes are nowstringtypes, conversion has to be done manuallyKeysQuery->ListApiKeysKeysResults->KeyViewListKey->KeyViewKeyCreation->CreateApiKeyKeyUpdate->PatchApiKeyPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit