feat: implement tags for keys and fix some typescript and biome errors #1
Conversation
…ns, enabling retrieval of API keys by tags
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant ApiKeyManager
participant Storage
note over Storage: Memory / Drizzle / Redis implementations
Client->>ApiKeyManager: findByTag(tag, ownerId?)
ApiKeyManager->>ApiKeyManager: normalize tag(s) to lowercase
ApiKeyManager->>Storage: findByTag/findByTags(tags[], ownerId)
alt Memory backend
Storage->>Storage: in-memory filter by metadata.tags overlap
else Drizzle backend
Storage->>Storage: SQL JSONB array-overlap query
else Redis backend
Storage->>Storage: SUNION(tagKeys) (+ optional owner set) → fetch records
end
Storage-->>ApiKeyManager: ApiKeyRecord[]
ApiKeyManager-->>Client: ApiKeyRecord[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 12 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="src/storage/memory.test.ts">
<violation number="1" location="src/storage/memory.test.ts:162">
This test never creates a non-matching owner, so it would still pass even if findByTag ignored the ownerId filter. Please add a second record with the same tag but a different owner and assert it is excluded.</violation>
</file>
<file name="src/storage/redis.test.ts">
<violation number="1" location="src/storage/redis.test.ts:199">
With only one key created for owner_123, this test never verifies that the owner filter is actually applied. Because RedisStore.findByTag currently unions owner and tag sets, an owner_123 key without the tag would still be returned, yet this assertion would still pass. Please add another key that should be filtered out to confirm the owner scoping works.</violation>
</file>
<file name="src/storage/redis.ts">
<violation number="1" location="src/storage/redis.ts:96">
Using `sunion` with the owner set causes `findByTag` to return keys that either match the tag or merely belong to the owner, including keys owned by other users. This breaks owner scoping and can leak cross-tenant data.</violation>
</file>
<file name="src/storage/drizzle.test.ts">
<violation number="1" location="src/storage/drizzle.test.ts:203">
This new test never exercises the “OR” semantics it claims to cover. Because the only fixture record contains all of the requested tags, a regression that requires *all* tags to match would still pass here. Please add distinct records or expectations that demonstrate any-tag matches to actually validate the intended behavior.</violation>
</file>
<file name="src/storage/memory.ts">
<violation number="1" location="src/storage/memory.ts:40">
The owner filter only runs when ownerId is truthy. Passing an empty string (still a valid ownerId) would bypass scoping and return other owners' keys.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test", "user_123"); |
There was a problem hiding this comment.
This test never creates a non-matching owner, so it would still pass even if findByTag ignored the ownerId filter. Please add a second record with the same tag but a different owner and assert it is excluded.
Prompt for AI agents
Address the following comment on src/storage/memory.test.ts at line 162:
<comment>This test never creates a non-matching owner, so it would still pass even if findByTag ignored the ownerId filter. Please add a second record with the same tag but a different owner and assert it is excluded.</comment>
<file context>
@@ -130,6 +130,41 @@ describe("MemoryStore", () => {
+ tags: ["test", "key", "more", "tags"],
+ });
+
+ const found = await store.findByTag("test", "user_123");
+ expect(found).toHaveLength(1);
+ expect(found[0]?.id).toBe(record.id);
</file context>
✅ Addressed in 689cac9
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test", "user_123"); |
There was a problem hiding this comment.
With only one key created for owner_123, this test never verifies that the owner filter is actually applied. Because RedisStore.findByTag currently unions owner and tag sets, an owner_123 key without the tag would still be returned, yet this assertion would still pass. Please add another key that should be filtered out to confirm the owner scoping works.
Prompt for AI agents
Address the following comment on src/storage/redis.test.ts at line 199:
<comment>With only one key created for owner_123, this test never verifies that the owner filter is actually applied. Because RedisStore.findByTag currently unions owner and tag sets, an owner_123 key without the tag would still be returned, yet this assertion would still pass. Please add another key that should be filtered out to confirm the owner scoping works.</comment>
<file context>
@@ -167,6 +167,41 @@ describe("RedisStore", () => {
+ tags: ["test", "key", "more", "tags"],
+ });
+
+ const found = await store.findByTag("test", "user_123");
+ expect(found).toHaveLength(1);
+ expect(found[0]?.id).toBe(record.id);
</file context>
✅ Addressed in c74e819
src/storage/redis.ts
Outdated
| : [this.tagKey(tag.toLowerCase())]; | ||
| const ownerKey = ownerId ? this.ownerKey(ownerId) : null; | ||
|
|
||
| const tagIds = await this.redis.sunion( |
There was a problem hiding this comment.
Using sunion with the owner set causes findByTag to return keys that either match the tag or merely belong to the owner, including keys owned by other users. This breaks owner scoping and can leak cross-tenant data.
Prompt for AI agents
Address the following comment on src/storage/redis.ts at line 96:
<comment>Using `sunion` with the owner set causes `findByTag` to return keys that either match the tag or merely belong to the owner, including keys owned by other users. This breaks owner scoping and can leak cross-tenant data.</comment>
<file context>
@@ -73,6 +84,37 @@ export class RedisStore implements Storage {
+ : [this.tagKey(tag.toLowerCase())];
+ const ownerKey = ownerId ? this.ownerKey(ownerId) : null;
+
+ const tagIds = await this.redis.sunion(
+ ownerKey ? [...tagKeys, ownerKey] : tagKeys
+ );
</file context>
✅ Addressed in 689cac9
src/storage/drizzle.test.ts
Outdated
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by multiple tags", async () => { |
There was a problem hiding this comment.
This new test never exercises the “OR” semantics it claims to cover. Because the only fixture record contains all of the requested tags, a regression that requires all tags to match would still pass here. Please add distinct records or expectations that demonstrate any-tag matches to actually validate the intended behavior.
Prompt for AI agents
Address the following comment on src/storage/drizzle.test.ts at line 203:
<comment>This new test never exercises the “OR” semantics it claims to cover. Because the only fixture record contains all of the requested tags, a regression that requires *all* tags to match would still pass here. Please add distinct records or expectations that demonstrate any-tag matches to actually validate the intended behavior.</comment>
<file context>
@@ -188,6 +188,41 @@ describe("DrizzleStore", () => {
+ expect(found[0]?.id).toBe(record.id);
+ });
+
+ it("should find all records by multiple tags", async () => {
+ const { record } = await keys.create({
+ ownerId: "user_123",
</file context>
✅ Addressed in 689cac9
src/storage/memory.ts
Outdated
| ownerId?: string | ||
| ): Promise<ApiKeyRecord[]> { | ||
| return Array.from(await this.keys.values()).filter((record) => { | ||
| if (ownerId && record.metadata.ownerId !== ownerId) { |
There was a problem hiding this comment.
The owner filter only runs when ownerId is truthy. Passing an empty string (still a valid ownerId) would bypass scoping and return other owners' keys.
Prompt for AI agents
Address the following comment on src/storage/memory.ts at line 40:
<comment>The owner filter only runs when ownerId is truthy. Passing an empty string (still a valid ownerId) would bypass scoping and return other owners' keys.</comment>
<file context>
@@ -31,6 +32,23 @@ export class MemoryStore implements Storage {
+ ownerId?: string
+ ): Promise<ApiKeyRecord[]> {
+ return Array.from(await this.keys.values()).filter((record) => {
+ if (ownerId && record.metadata.ownerId !== ownerId) {
+ return false;
+ }
</file context>
✅ Addressed in 689cac9
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
README.md (1)
270-270: Consider adding usage examples for the new findByTag method.The custom storage interface now correctly includes
findByTag, but the README lacks usage examples demonstrating how users can query keys by tags. Consider adding a section showing tag-based queries similar to the existing API examples.Example addition:
// Find keys by tag const prodKeys = await keys.findByTag('production') const testKeys = await keys.findByTag(['test', 'staging']) // OR logic // Find keys by tag for specific owner const userProdKeys = await keys.findByTag('production', 'user_123')src/types/storage-types.ts (1)
27-32: Refactor JSDoc to follow TypeScript conventions.The JSDoc embeds type information in the description text, which is non-standard for TypeScript. Since TypeScript already provides type information, the JSDoc should omit explicit type annotations.
Apply this diff:
/** * Find all API keys by tag(s) - * @param tag - The tag(s) to search for {string | string[]} - * @param ownerId - The owner ID to filter by (optional) {string} + * @param tag - The tag(s) to search for + * @param ownerId - Optional owner ID to filter results */ findByTag(tag: string | string[], ownerId?: string): Promise<ApiKeyRecord[]>;src/storage/drizzle.test.ts (1)
646-651: Refactor appears unrelated to tag functionality.This change from direct array access to
.at(i)with an undefined guard is safer but appears unrelated to the tag feature implementation. While it's a good defensive coding practice, it might belong in a separate commit for clarity.src/storage/drizzle.ts (1)
92-97: Optional: Unnecessary length check.The condition
if (tagArray.length > 0)will always be true sincetagArrayis constructed from either a single tag or an array of tags. Consider removing this check for cleaner code.src/manager.ts (1)
273-273: Consider adding validation for tag values.Tags are normalized to lowercase (line 273) and assigned to the record (line 290), but there's no validation to prevent empty or whitespace-only strings. Users could create keys with tags like
["", " ", " "], which may not be useful and could cause confusion.Apply this diff to add validation:
-const tags = metadata.tags?.map((t) => t.toLowerCase()); +const tags = metadata.tags + ?.map((t) => t.trim().toLowerCase()) + .filter((t) => t.length > 0);This ensures tags are trimmed and non-empty before storage.
Also applies to: 290-290
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
FEATURE_SUGGESTIONS.md(7 hunks)README.md(1 hunks)biome.jsonc(1 hunks)src/manager.ts(4 hunks)src/storage/drizzle.test.ts(2 hunks)src/storage/drizzle.ts(4 hunks)src/storage/memory.test.ts(1 hunks)src/storage/memory.ts(1 hunks)src/storage/redis.test.ts(1 hunks)src/storage/redis.ts(5 hunks)src/types/api-key-types.ts(1 hunks)src/types/storage-types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/storage/memory.test.ts (1)
examples/drizzle-schema.ts (1)
keys(15-19)
src/types/storage-types.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/manager.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/redis.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/memory.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/drizzle.test.ts (1)
examples/drizzle-schema.ts (1)
keys(15-19)
src/storage/drizzle.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
🔇 Additional comments (16)
biome.jsonc (1)
9-20: LGTM!The cognitive complexity rule configuration is a good addition for maintaining code quality. A threshold of 50 with warning level is reasonable and aligns with the PR's broader code quality improvements.
src/storage/drizzle.test.ts (1)
191-224: LGTM!The test coverage for
findByTagis comprehensive, covering single tag, multiple tags (OR logic), and owner-filtered scenarios. The test structure is consistent with similar tests in other storage backends.FEATURE_SUGGESTIONS.md (2)
7-7: LGTM!Correctly reflects that key tags/labels functionality has been implemented and moved from the suggestions list.
48-107: LGTM!The renumbering of remaining feature suggestions is correct after removing the now-implemented tags feature.
src/storage/memory.ts (1)
34-49: LGTM!The implementation correctly handles all specified requirements:
- Case-insensitive tag matching via normalization
- OR logic for multiple tags using
some()- Optional owner filtering
- Proper handling of undefined tags via optional chaining
src/storage/memory.test.ts (1)
133-166: LGTM!Comprehensive test coverage for
findByTagthat mirrors the test structure in other storage backends. The three scenarios validate single tag, multiple tag (OR logic), and owner-filtered queries effectively.src/storage/redis.test.ts (1)
170-203: LGTM!Excellent test coverage that maintains consistency with the other storage backend tests. The three test cases validate all the key scenarios for tag-based queries, and the assertions correctly verify record identity.
src/types/api-key-types.ts (1)
41-42: LGTM! Clean schema addition.The optional
tagsfield follows the established pattern for array fields in the schema (similar toscopeson line 17).src/storage/redis.ts (4)
18-20: LGTM! Helper follows naming convention.The
tagKeyhelper is consistent with existinghashKeyandownerKeymethods.
41-45: LGTM! Tag associations stored correctly.Records are added to tag-specific sets with proper lowercase normalization for case-insensitive matching.
146-150: LGTM! Tag cleanup on delete.Properly removes the record from all tag sets to maintain consistency.
167-171: LGTM! Tag cleanup on owner deletion.Correctly removes tag associations when deleting all keys for an owner.
src/storage/drizzle.ts (2)
1-1: LGTM! Necessary imports for tag query functionality.The added imports (
and,arrayContains,sql,SQL) are all used in the newfindByTagimplementation.
60-60: LGTM! Improved null safety.The enhanced checks (
rows.length > 0 && rows[0]) provide better defensive coding compared to just checkingrows[0].Also applies to: 70-70
src/manager.ts (2)
306-311: LGTM! Clean delegation to storage layer.The
findByTagmethod properly delegates to the storage implementation, maintaining the abstraction.
406-406: LGTM! Tags propagated through rotation.The rotation logic correctly carries forward tags from the old record or accepts new tags from the metadata parameter.
|
@cubic-dev-ai rerun the review, some code is updated |
@max-programming I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 12 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name="src/storage/redis.ts">
<violation number="1" location="src/storage/redis.ts:91">
Using SUNION with the owner set breaks owner scoping—queries with ownerId will include other owners' keys that share the tag and also return this owner's keys that lack it. Please intersect the owner set with the tag matches instead of unioning them.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:270">
The Storage interface now requires both findByTag and findByTags, so this example is incomplete—users copying it will hit a type error. Please add the findByTags stub alongside findByTag to keep the snippet accurate.</violation>
</file>
<file name="src/storage/drizzle.ts">
<violation number="1" location="src/storage/drizzle.ts:90">
Multi-tag searches always miss results because the SQL builds `ARRAY['tag1,tag2']`, so `?|` checks only the concatenated string instead of each tag.</violation>
</file>
<file name="src/storage/memory.test.ts">
<violation number="1" location="src/storage/memory.test.ts:151">
This test never validates the intended OR semantics for findByTags—because it only creates one record containing every queried tag, the assertion would still pass even if the implementation required all tags. Please add a contrasting record missing one of the tags (or similar) so the test fails when OR matching is broken.</violation>
<violation number="2" location="src/storage/memory.test.ts:162">
This owner-scoped tag test never verifies that ownerId filtering works—it only creates a record for the matching owner, so the assertion would still pass even if findByTag ignored the owner argument. Please add a record for a different owner with the same tag(s) and assert it is excluded.</violation>
</file>
<file name="src/manager.ts">
<violation number="1" location="src/manager.ts:407">
Updating metadata writes tags without lowercasing, so changing tags to mixed/upper case breaks case-insensitive lookups. Normalize the tags array before persisting.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
Have you checked the comments? cubic left a few good notes in there |
|
@izadoesdev making these changes |
… logic, tag normalization, empty owner handlin
|
I've commited some fixes |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/storage/drizzle.ts (1)
17-28: Pass the array directly to leverage Drizzle's parameter binding.Manual SQL escaping with
sql.rawworks but is less idiomatic than letting Drizzle handle parameter binding. PostgreSQL's?|operator accepts atext[]parameter, and Drizzle'ssqltemplate will bind JS arrays as proper PostgreSQL parameters.As per coding guidelines
Replace the manual quoting:
private jsonbArrayOverlaps(tags: string[]): SQL { const lowercasedTags = tags.map((t) => t.toLowerCase()); - const quotedTags = lowercasedTags - .map((tag) => `'${tag.replace(/'/g, "''")}'`) - .join(","); - return sql`${this.table.metadata}->'tags' ?| ARRAY[${sql.raw(quotedTags)}]`; + return sql`${this.table.metadata}->'tags' ?| ${lowercasedTags}`; }
🧹 Nitpick comments (1)
src/storage/memory.test.ts (1)
179-188: Add a record with a different owner to verify filtering.This test still doesn't prove that owner filtering works—it only creates a record for the matching owner (user_123), so the assertion would pass even if
findByTagignored theownerIdparameter.Apply this diff:
it("should find all records by owner and multiple tags", async () => { const { record } = await keys.create({ ownerId: "user_123", tags: ["test", "key", "more", "tags"], }); + + // Create a key with same tags but different owner + await keys.create({ + ownerId: "user_456", + tags: ["test", "key"], + }); const found = await store.findByTag(["test", "key"], "user_123"); expect(found).toHaveLength(1); expect(found[0]?.id).toBe(record.id); });Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/manager.ts(4 hunks)src/storage/drizzle.test.ts(3 hunks)src/storage/drizzle.ts(5 hunks)src/storage/memory.test.ts(1 hunks)src/storage/memory.ts(1 hunks)src/storage/redis.test.ts(1 hunks)src/storage/redis.ts(5 hunks)src/types/storage-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/types/storage-types.ts
- src/manager.ts
- src/storage/redis.test.ts
- src/storage/drizzle.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/storage/redis.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/drizzle.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/memory.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
🔇 Additional comments (4)
src/storage/memory.test.ts (1)
145-177: LGTM! Test coverage improved.These tests now properly verify:
- OR semantics for multiple tags (record r2 has only "test", not "key", so it proves OR matching works)
- Owner filtering (creating a key for user_456 with the same tag proves the filter excludes other owners)
src/storage/memory.ts (1)
34-45: LGTM! Owner filtering fixed.The
ownerId !== undefinedcheck correctly handles all falsy values including empty strings, addressing the previous review concern. The tag matching logic properly normalizes to lowercase and implements OR semantics across multiple tags.src/storage/redis.ts (2)
87-125: LGTM! Intersection logic correctly implemented.The tag-to-owner filtering is now correct:
- Lines 98-101: Retrieves IDs matching any of the requested tags (OR semantics)
- Lines 103-106: Filters those IDs to only include ones owned by the specified user (AND semantics)
This properly addresses the previous critical issue where SUNION was incorrectly combining tag and owner sets.
155-159: LGTM! Tag cleanup is thorough.Tag associations are properly removed during both
deleteanddeleteByOwneroperations, ensuring no orphaned tag entries remain in Redis.Also applies to: 176-180
|
@cubic-dev-ai re run the review |
@max-programming I've started the AI code review. It'll take a few minutes to complete. |
|
some of my commits got overriden but no problem |
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="README.md">
<violation number="1" location="README.md:271">
`Storage` only exposes findByTag(tag: string | string[], ownerId?), so documenting a separate findByTags method makes the sample invalid and will fail type-checking. Please remove or correct this method.</violation>
</file>
<file name="src/storage/drizzle.ts">
<violation number="1" location="src/storage/drizzle.ts:112">
findByTag may return all API keys when called with empty filters (missing WHERE), enabling data exposure</violation>
</file>
<file name="src/storage/redis.ts">
<violation number="1" location="src/storage/redis.ts:87">
findByTag relies on Redis tag sets that we only populate during save(); updateMetadata never updates those sets, so changing metadata.tags leaves this index stale—new tags are missed and old tags still match. Please re-sync the tag sets whenever metadata.tags changes.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/redis.ts (1)
127-142: Critical: Tag sets not synchronized when metadata.tags is updated.When
metadata.tagsis modified viaupdateMetadata, the Redis tag sets (maintained bytagKey()) are not updated. This causes:
- Stale lookups:
findByTagreturns records that no longer have those tags- Missing results: Records with new tags won't be found
The
save(lines 41-45) anddelete(lines 155-159) methods correctly maintain tag sets, butupdateMetadatadoes not.Apply this diff to synchronize tag sets:
async updateMetadata( id: string, metadata: Partial<ApiKeyMetadata> ): Promise<void> { const record = await this.findById(id); if (!record) { throw new Error(`API key with id ${id} not found`); } + const oldTags = record.metadata.tags || []; + const newTags = metadata.tags; + record.metadata = { ...record.metadata, ...metadata }; - await this.redis.set(this.key(id), JSON.stringify(record)); + + const pipeline = this.redis.pipeline(); + pipeline.set(this.key(id), JSON.stringify(record)); if (metadata.revokedAt) { - await this.redis.del(this.hashKey(record.keyHash)); + pipeline.del(this.hashKey(record.keyHash)); } + + // Update tag sets if tags changed + if (newTags !== undefined) { + // Remove from old tag sets + for (const tag of oldTags) { + pipeline.srem(this.tagKey(tag.toLowerCase()), id); + } + // Add to new tag sets + for (const tag of newTags) { + pipeline.sadd(this.tagKey(tag.toLowerCase()), id); + } + } + + await pipeline.exec(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/manager.ts(4 hunks)src/storage/drizzle.test.ts(3 hunks)src/storage/drizzle.ts(5 hunks)src/storage/memory.test.ts(1 hunks)src/storage/memory.ts(1 hunks)src/storage/redis.test.ts(1 hunks)src/storage/redis.ts(5 hunks)src/types/storage-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/storage/memory.ts
- src/storage/redis.test.ts
- src/storage/memory.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/storage/drizzle.test.ts (1)
examples/drizzle-schema.ts (1)
keys(15-19)
src/storage/drizzle.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/manager.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/storage/redis.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
src/types/storage-types.ts (1)
src/types/api-key-types.ts (1)
ApiKeyRecord(50-59)
🔇 Additional comments (10)
src/types/storage-types.ts (1)
27-39: LGTM! Clean API design for tag-based queries.The new methods are well-documented and follow the established patterns in the Storage interface. The optional
ownerIdparameter provides flexible filtering.src/storage/drizzle.test.ts (2)
191-252: LGTM! Comprehensive tag query test coverage.The test suite properly validates:
- Single tag searches
- Multiple tag OR semantics (lines 203-218 correctly test that keys matching any tag are returned)
- Owner-scoped filtering for both single and multiple tags
673-680: LGTM! Defensive array access prevents edge-case failures.The
.at(i)with null checks guards against race conditions in concurrent test scenarios. Good defensive programming.Also applies to: 1123-1131
src/storage/drizzle.ts (2)
94-119: LGTM! Tag query logic correctly handles filtering and empty conditions.The implementation properly:
- Returns empty array when no conditions are provided (lines 105-107), preventing data exposure
- Combines tag and owner filters with AND logic via
and(...conditions)- Delegates single-tag queries to the multi-tag method for code reuse
72-72: LGTM! Defensive null checks prevent undefined access.Also applies to: 82-82
src/manager.ts (3)
273-290: LGTM! Tags properly normalized to lowercase on creation.Tag normalization happens at the manager layer before persistence, ensuring case-insensitive tag matching works correctly across all storage backends.
306-312: LGTM! Clean delegation to storage layer.The new query methods follow the established pattern of delegating to storage, maintaining separation of concerns.
407-409: LGTM! Tag normalization consistent in rotate operation.New tags are lowercased, or existing (already normalized) tags are preserved, maintaining consistency.
src/storage/redis.ts (2)
87-125: LGTM! Tag query correctly implements OR logic with optional owner filtering.The implementation properly:
- Uses
SUNIONfor OR semantics across multiple tags (lines 94-97)- Applies owner filtering via client-side intersection when needed (lines 99-102)
- Efficiently batches record fetches with pipeline (lines 108-120)
41-45: LGTM! Tag sets correctly maintained on save and delete.The tag index maintenance is properly implemented in save, delete, and deleteByOwner operations.
Also applies to: 155-159, 176-180
|
@cubic-dev-ai re-run the review now |
@max-programming I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="src/storage/drizzle.ts">
<violation number="1" location="src/storage/drizzle.ts:89">
Dynamic IN clause incorrectly embeds a JavaScript array, which will bind as a single parameter and can produce invalid SQL or no matches. Use ANY(array) or expand placeholders with sql.join to properly bind each tag.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
This PR implements the feature of adding tags to keys, and retrieving keys based on one or more tags
ownerIdparameter is optional for scoping searchesSummary by cubic
Adds tag support to API keys and a new findByTag method to fetch keys by tag(s). Matching is case-insensitive with OR logic, and searches can be scoped by owner.
New Features
Migration
Summary by CodeRabbit
New Features
Documentation
Tests
Chores