-
-
Notifications
You must be signed in to change notification settings - Fork 14
feat: implement tags for keys and fix some typescript and biome errors #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
92d2382
054f15b
b0e39b9
b31d4b2
3e1a436
756b9bc
cad6983
f9125d9
689cac9
c74e819
d39fc01
93b7298
a053b8a
60ad9ae
daec392
0b4c8fa
266b493
6c64d79
bc52bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -188,6 +188,41 @@ describe("DrizzleStore", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("findByTag", () => { | ||
| it("should find all records by one tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test"); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by multiple tags", async () => { | ||
|
||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag(["test", "key"]); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by owner and tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test", "user_123"); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
| }); | ||
|
|
||
| describe("updateMetadata", () => { | ||
| it("should update metadata", async () => { | ||
| const { record } = await keys.create({ | ||
|
|
@@ -608,7 +643,11 @@ describe("DrizzleStore", () => { | |
| await Promise.all(promises); | ||
|
|
||
| for (let i = 0; i < records.length; i++) { | ||
| const found = await store.findById(records[i].record.id); | ||
| const record = records.at(i); | ||
| if (!record) { | ||
| continue; | ||
| } | ||
| const found = await store.findById(record.record.id); | ||
| expect(found?.metadata.name).toBe(`Updated ${i}`); | ||
| } | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,41 @@ describe("MemoryStore", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("findByTag", () => { | ||
| it("should find all records by one tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test"); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by multiple tags", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag(["test", "key"]); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by owner and tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test", "user_123"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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✅ Addressed in
max-programming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
| }); | ||
|
|
||
| describe("updateMetadata", () => { | ||
| it("should update metadata for a record", async () => { | ||
| const { record } = await keys.create({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,23 @@ export class MemoryStore implements Storage { | |
| ); | ||
| } | ||
|
|
||
| async findByTag( | ||
| tag: string | string[], | ||
| ownerId?: string | ||
| ): Promise<ApiKeyRecord[]> { | ||
| return Array.from(await this.keys.values()).filter((record) => { | ||
| if (ownerId && record.metadata.ownerId !== ownerId) { | ||
|
||
| return false; | ||
| } | ||
| const recordTags = record.metadata.tags?.map((t) => t.toLowerCase()); | ||
| // case insensitive tag matching | ||
| if (Array.isArray(tag)) { | ||
| return tag.some((t) => recordTags?.includes(t.toLowerCase())); | ||
| } | ||
| return recordTags?.includes(tag.toLowerCase()); | ||
| }); | ||
| } | ||
|
|
||
| async updateMetadata( | ||
| id: string, | ||
| metadata: Partial<ApiKeyMetadata> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,6 +167,41 @@ describe("RedisStore", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("findByTag", () => { | ||
| it("should find all records by one tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test"); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by multiple tags", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag(["test", "key"]); | ||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
|
|
||
| it("should find all records by owner and tag", async () => { | ||
| const { record } = await keys.create({ | ||
| ownerId: "user_123", | ||
| tags: ["test", "key", "more", "tags"], | ||
| }); | ||
|
|
||
| const found = await store.findByTag("test", "user_123"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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✅ Addressed in |
||
| expect(found).toHaveLength(1); | ||
| expect(found[0]?.id).toBe(record.id); | ||
| }); | ||
| }); | ||
|
|
||
| describe("updateMetadata", () => { | ||
| it("should update metadata for a record", async () => { | ||
| const { record } = await keys.create({ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.