-
Notifications
You must be signed in to change notification settings - Fork 138
Fix duplicate resource errors from throwing 500 instead of user frien… #2262
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ad3d165
Fix duplicate resource errors from throwing 500 instead of user frien…
sarah-inkeep a11b1e9
Address claude comments
sarah-inkeep 246359f
Merge branch 'main' of github.com:inkeep/agents into prd-6088
sarah-inkeep 5ee3c77
Add tests
sarah-inkeep 85f6790
Merge branch 'main' of github.com:inkeep/agents into prd-6088
sarah-inkeep File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| "@inkeep/agents-work-apps": patch | ||
| "@inkeep/agents-core": patch | ||
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| agents-core: Add isUniqueConstraintError and throwIfUniqueConstraintError helpers to normalize unique constraint error detection across PostgreSQL and Doltgres | ||
|
|
||
| agents-api: Fix duplicate resource creation returning 500 instead of 409 when Doltgres reports unique constraint violations as MySQL errno 1062 | ||
|
|
||
| agents-work-apps: Fix concurrent user mapping creation returning 500 instead of succeeding silently when a duplicate mapping already exists |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { isUniqueConstraintError, throwIfUniqueConstraintError } from '../error'; | ||
|
|
||
| describe('isUniqueConstraintError', () => { | ||
| describe('PostgreSQL unique violation (23505)', () => { | ||
| it('returns true when cause.code is 23505', () => { | ||
| const error = { cause: { code: '23505' }, message: 'Failed query: INSERT ...' }; | ||
| expect(isUniqueConstraintError(error)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false when cause.code is a different PG error code', () => { | ||
| const error = { cause: { code: '40001' }, message: 'Failed query: INSERT ...' }; | ||
| expect(isUniqueConstraintError(error)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Doltgres MySQL errno 1062', () => { | ||
| it('returns true when cause.message contains 1062', () => { | ||
| const error = { | ||
| cause: { code: 'XX000', message: "1062: Duplicate entry 'x' for key 'PRIMARY'" }, | ||
| message: 'Failed query: INSERT ...', | ||
| }; | ||
| expect(isUniqueConstraintError(error)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false when cause.message contains a different MySQL errno', () => { | ||
| const error = { | ||
| cause: { code: 'XX000', message: '1213: Deadlock found when trying to get lock' }, | ||
| message: 'Failed query: INSERT ...', | ||
| }; | ||
| expect(isUniqueConstraintError(error)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('"already exists" message fallback', () => { | ||
| it('returns true when error message contains "already exists"', () => { | ||
| const error = { message: "Branch 'main' already exists" }; | ||
| expect(isUniqueConstraintError(error)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false when error message does not contain "already exists"', () => { | ||
| const error = { message: 'Branch creation failed' }; | ||
| expect(isUniqueConstraintError(error)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('non-matching errors', () => { | ||
| it('returns false for a generic Error', () => { | ||
| expect(isUniqueConstraintError(new Error('Something went wrong'))).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for null', () => { | ||
| expect(isUniqueConstraintError(null)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for undefined', () => { | ||
| expect(isUniqueConstraintError(undefined)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for a plain string', () => { | ||
| expect(isUniqueConstraintError('duplicate key error')).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for an error with no cause and no matching message', () => { | ||
| expect(isUniqueConstraintError({ message: 'Connection timeout' })).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('throwIfUniqueConstraintError', () => { | ||
| it('throws for a PostgreSQL 23505 error', () => { | ||
| const error = { cause: { code: '23505' }, message: 'Failed query: INSERT ...' }; | ||
| expect(() => throwIfUniqueConstraintError(error, 'Resource already exists')).toThrow(); | ||
| }); | ||
|
|
||
| it('throws for a Doltgres 1062 error', () => { | ||
| const error = { | ||
| cause: { code: 'XX000', message: "1062: Duplicate entry 'x' for key 'PRIMARY'" }, | ||
| message: 'Failed query: INSERT ...', | ||
| }; | ||
| expect(() => throwIfUniqueConstraintError(error, 'Resource already exists')).toThrow(); | ||
| }); | ||
|
|
||
| it('throws with conflict code and provided message', async () => { | ||
| const error = { cause: { code: '23505' }, message: 'Failed query: INSERT ...' }; | ||
| let caught: any; | ||
| try { | ||
| throwIfUniqueConstraintError(error, "Agent with ID 'x' already exists"); | ||
| } catch (e) { | ||
| caught = e; | ||
| } | ||
| const body = JSON.parse(await caught.getResponse().text()); | ||
| expect(body.error.code).toBe('conflict'); | ||
| expect(body.error.message).toBe("Agent with ID 'x' already exists"); | ||
| }); | ||
|
|
||
| it('does not throw for a non-unique-constraint error', () => { | ||
| expect(() => | ||
| throwIfUniqueConstraintError(new Error('Connection timeout'), 'Resource already exists') | ||
| ).not.toThrow(); | ||
| }); | ||
|
|
||
| it('does not throw for null', () => { | ||
| expect(() => throwIfUniqueConstraintError(null, 'Resource already exists')).not.toThrow(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Minor: Error logging at wrong level for expected user errors
Issue:
logger.error()is called before checking for unique constraint errors, meaning expected duplicate creation attempts (which return 409 to users) are logged at ERROR level.Why: Logging expected user errors at ERROR level creates noise in monitoring, potentially triggering unnecessary alerts and making it harder to spot real issues.
Fix: Move the error log after the unique constraint check, or log at INFO level for duplicates:
Note: This would require importing
isUniqueConstraintErrorseparately fromthrowIfUniqueConstraintError.Refs: