Skip to content

Commit 43f357c

Browse files
UlisesGasconUlisesGascon
committed
refactor: add additional test and improve the endpoint logic
1 parent f2f7917 commit 43f357c

File tree

3 files changed

+116
-13
lines changed

3 files changed

+116
-13
lines changed

‎__tests__/httpServer/apiV1.test.js‎

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,13 +246,41 @@ describe('HTTP Server API V1', () => {
246246
expect(response.status).toBe(201)
247247
expect(response.body).toHaveProperty('id')
248248
expect(response.body).toHaveProperty('login', 'expressjs')
249-
expect(response.body).toHaveProperty('html_url', githubOrgUrl)
249+
expect(response.body).toHaveProperty('html_url', githubOrgUrl.toLowerCase())
250250
expect(response.body).toHaveProperty('project_id', projectId)
251251

252+
// Verify the Location header is set correctly
253+
expect(response.headers).toHaveProperty('location', `/api/v1/project/${projectId}/gh-org/${response.body.id}`)
254+
252255
// Verify organization was added to the database
253256
const orgs = await getAllGithubOrganizationsByProjectsId([projectId])
254257
expect(orgs.length).toBe(1)
255-
expect(orgs[0].html_url).toBe(githubOrgUrl)
258+
expect(orgs[0].html_url).toBe(githubOrgUrl.toLowerCase())
259+
})
260+
261+
test('should correctly extract organization login from URL with trailing slash', async () => {
262+
const githubOrgUrl = 'https://github.com/expressjs/'
263+
264+
const response = await app
265+
.post(`/api/v1/project/${projectId}/gh-org`)
266+
.send({ githubOrgUrl })
267+
268+
expect(response.status).toBe(201)
269+
expect(response.body).toHaveProperty('login', 'expressjs')
270+
expect(response.body).toHaveProperty('html_url', githubOrgUrl.toLowerCase().replace(/\/$/, ''))
271+
})
272+
273+
test('should correctly extract organization login from URL with query parameters', async () => {
274+
const githubOrgUrl = 'https://github.com/expressjs?tab=repositories'
275+
276+
const response = await app
277+
.post(`/api/v1/project/${projectId}/gh-org`)
278+
.send({ githubOrgUrl })
279+
280+
expect(response.status).toBe(201)
281+
expect(response.body).toHaveProperty('login', 'expressjs')
282+
// The stored URL should be normalized without query parameters
283+
expect(response.body.html_url).not.toContain('?')
256284
})
257285

258286
test('should return 400 for invalid project ID', async () => {
@@ -262,7 +290,27 @@ describe('HTTP Server API V1', () => {
262290

263291
expect(response.status).toBe(400)
264292
expect(response.body).toHaveProperty('errors')
265-
expect(response.body.errors[0]).toHaveProperty('message', 'Invalid project ID')
293+
expect(response.body.errors[0]).toHaveProperty('message', 'must be integer')
294+
})
295+
296+
test('should return 400 for zero project ID', async () => {
297+
const response = await app
298+
.post('/api/v1/project/0/gh-org')
299+
.send({ githubOrgUrl: 'https://github.com/expressjs' })
300+
301+
expect(response.status).toBe(400)
302+
expect(response.body).toHaveProperty('errors')
303+
expect(response.body.errors[0]).toHaveProperty('message', 'Invalid project ID. Must be a positive integer.')
304+
})
305+
306+
test('should return 400 for negative project ID', async () => {
307+
const response = await app
308+
.post('/api/v1/project/-1/gh-org')
309+
.send({ githubOrgUrl: 'https://github.com/expressjs' })
310+
311+
expect(response.status).toBe(400)
312+
expect(response.body).toHaveProperty('errors')
313+
expect(response.body.errors[0]).toHaveProperty('message', 'Invalid project ID. Must be a positive integer.')
266314
})
267315

268316
test('should return 400 for invalid GitHub organization URL', async () => {
@@ -305,6 +353,38 @@ describe('HTTP Server API V1', () => {
305353
expect(response.body.errors[0]).toHaveProperty('message', 'GitHub organization already exists for this project')
306354
})
307355

356+
test('should return 409 for duplicate GitHub organization with different case', async () => {
357+
// First add the organization with lowercase
358+
await app
359+
.post(`/api/v1/project/${projectId}/gh-org`)
360+
.send({ githubOrgUrl: 'https://github.com/expressjs' })
361+
362+
// Try to add it again with uppercase
363+
const response = await app
364+
.post(`/api/v1/project/${projectId}/gh-org`)
365+
.send({ githubOrgUrl: 'https://github.com/ExpressJS' })
366+
367+
expect(response.status).toBe(409)
368+
expect(response.body).toHaveProperty('errors')
369+
expect(response.body.errors[0]).toHaveProperty('message', 'GitHub organization already exists for this project')
370+
})
371+
372+
test('should return 409 for duplicate GitHub organization with trailing slash', async () => {
373+
// First add the organization without trailing slash
374+
await app
375+
.post(`/api/v1/project/${projectId}/gh-org`)
376+
.send({ githubOrgUrl: 'https://github.com/expressjs' })
377+
378+
// Try to add it again with trailing slash
379+
const response = await app
380+
.post(`/api/v1/project/${projectId}/gh-org`)
381+
.send({ githubOrgUrl: 'https://github.com/expressjs/' })
382+
383+
expect(response.status).toBe(409)
384+
expect(response.body).toHaveProperty('errors')
385+
expect(response.body.errors[0]).toHaveProperty('message', 'GitHub organization already exists for this project')
386+
})
387+
308388
test.todo('should return 500 for internal server error')
309389
})
310390
})

‎src/httpServer/routers/apiV1.js‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,13 @@ function createApiRouter (knex, express) {
3636
})
3737

3838
router.post('/project/:projectId/gh-org', async (req, res) => {
39-
const projectId = parseInt(req.params.projectId)
39+
// Parse projectId with explicit radix 10 to avoid issues with leading zeros
40+
const projectId = parseInt(req.params.projectId, 10)
4041
const { githubOrgUrl } = req.body
4142

42-
if (!projectId || !Number.isInteger(projectId)) {
43-
return res.status(400).json({ errors: [{ message: 'Invalid project ID' }] })
43+
// Stricter validation: check for NaN, non-integers, and non-positive values
44+
if (isNaN(projectId) || !Number.isInteger(projectId) || projectId <= 0) {
45+
return res.status(400).json({ errors: [{ message: 'Invalid project ID. Must be a positive integer.' }] })
4446
}
4547
if (!githubOrgUrl || !validateGithubUrl(githubOrgUrl)) {
4648
return res.status(400).json({ errors: [{ message: 'Invalid GitHub organization name' }] })
@@ -51,19 +53,40 @@ function createApiRouter (knex, express) {
5153
return res.status(404).json({ errors: [{ message: 'Project not found' }] })
5254
}
5355

56+
// Normalize the input URL by parsing it with URL object
57+
// This allows us to handle different protocols, query parameters, and trailing slashes
58+
const urlObj = new URL(githubOrgUrl)
59+
// Create a normalized URL: lowercase, no trailing slash, no query parameters
60+
const normalizedGithubOrgUrl = `${urlObj.protocol}//${urlObj.host}${urlObj.pathname.replace(/\/$/, '')}`.toLowerCase()
61+
5462
const existingGhOrg = await getAllGithubOrganizationsByProjectsId([projectId])
55-
if (existingGhOrg.some(org => org.html_url === githubOrgUrl)) {
63+
if (existingGhOrg.some(org => {
64+
try {
65+
// Apply the same normalization to stored URLs for consistent comparison
66+
const storedUrl = new URL(org.html_url)
67+
const normalizedStoredUrl = `${storedUrl.protocol}//${storedUrl.host}${storedUrl.pathname.replace(/\/$/, '')}`.toLowerCase()
68+
return normalizedStoredUrl === normalizedGithubOrgUrl
69+
} catch (e) {
70+
// Fallback to simple comparison if URL parsing fails
71+
return org.html_url.toLowerCase().replace(/\/$/, '') === normalizedGithubOrgUrl
72+
}
73+
})) {
5674
return res.status(409).json({ errors: [{ message: 'GitHub organization already exists for this project' }] })
5775
}
5876

5977
try {
78+
// Get the pathname from the already created URL object, remove leading/trailing slashes, and take the first segment
79+
const login = urlObj.pathname.replace(/^\/|\/$/, '').split('/')[0]
80+
6081
const org = await addGithubOrganization({
61-
html_url: githubOrgUrl,
62-
login: githubOrgUrl.split('https://github.com/')[1],
82+
html_url: normalizedGithubOrgUrl,
83+
login,
6384
project_id: project.id
6485
})
6586

66-
return res.status(201).json(org)
87+
return res.status(201)
88+
.header('Location', `/api/v1/project/${projectId}/gh-org/${org.id}`)
89+
.json(org)
6790
} catch (err) {
6891
logger.error(err)
6992
return res.status(500).json({ errors: [{ message: 'Internal server error' }] })

‎src/httpServer/swagger/api-v1.yml‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,8 @@ paths:
159159
required: true
160160
description: The ID of the project
161161
schema:
162-
type: string
163-
example: express
162+
type: integer
163+
example: 42
164164
requestBody:
165165
required: true
166166
content:
@@ -171,7 +171,7 @@ paths:
171171
properties:
172172
githubOrgUrl:
173173
type: string
174-
pattern: 'https://github.com/[^/]+'
174+
pattern: '^https://github.com/[^/]+$'
175175
example: 'https://github.com/expressjs'
176176
required:
177177
- githubOrgUrl

0 commit comments

Comments
 (0)