Skip to content

Add tests for less common entity routes #920

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fix: some tests and policies error return with invalid params
  • Loading branch information
avallete committed Apr 2, 2025
commit 9ac9b0a1ff628a29c4d4201158f33ec36bd700e7
3 changes: 3 additions & 0 deletions src/lib/PostgresMetaPolicies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ export default class PostgresMetaPolicies {
command?: string
roles?: string[]
}): Promise<PostgresMetaResult<PostgresPolicy>> {
if (!table || !name) {
return { data: null, error: { message: 'Missing required name or table parameter' } }
}
Comment on lines +115 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore

Avoid ident to raise an exception ending with 500 if those params are empty. TODO: migrate all this to zod for better validation of the params.

const definitionClause = definition === undefined ? '' : `USING (${definition})`
const checkClause = check === undefined ? '' : `WITH CHECK (${check})`
const sql = `
Expand Down
17 changes: 0 additions & 17 deletions test/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,6 @@ test('config version endpoint', async () => {
expect(data.version).toContain('PostgreSQL')
})

// Skip tests for endpoints that don't exist
test.skip('config max_result_size endpoint', async () => {
const res = await app.inject({
method: 'GET',
path: '/config/max_result_size',
})
expect(res.statusCode).toBe(200)
})

test.skip('config health endpoint', async () => {
const res = await app.inject({
method: 'GET',
path: '/config/health',
})
expect(res.statusCode).toBe(200)
})

test('config with invalid endpoint', async () => {
const res = await app.inject({
method: 'GET',
Expand Down
12 changes: 0 additions & 12 deletions test/server/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,6 @@ test('extension list filtering', async () => {
expect(extensions.length).toBeLessThanOrEqual(5)
})

test('extension list with specific included schema', async () => {
const res = await app.inject({
method: 'GET',
path: '/extensions?includedSchemas=public',
})
expect(res.statusCode).toBe(200)
const extensions = res.json()
expect(Array.isArray(extensions)).toBe(true)
// Just make sure we get extensions, don't check schema as it depends on environment
expect(extensions.length).toBeGreaterThanOrEqual(0)
})

test('extension with invalid id', async () => {
const res = await app.inject({
method: 'GET',
Expand Down
41 changes: 35 additions & 6 deletions test/server/format.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,16 @@ test('format SQL query', async () => {
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('text/plain')
const formattedQuery = res.body
expect(formattedQuery).toContain('SELECT')
expect(formattedQuery).toContain('FROM')
expect(formattedQuery).toContain('WHERE')
expect(formattedQuery).toMatchInlineSnapshot(`
"SELECT
id,
name
FROM
users
WHERE
status = 'ACTIVE'
"
`)
})

test('format complex SQL query', async () => {
Expand All @@ -26,7 +33,24 @@ test('format complex SQL query', async () => {
})
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('text/plain')
expect(res.body).toBeTruthy()
expect(res.body).toMatchInlineSnapshot(`
"SELECT
u.id,
u.name,
p.title,
p.created_at
FROM
users u
JOIN posts p ON u.id = p.user_id
WHERE
u.status = 'ACTIVE'
AND p.published = true
ORDER BY
p.created_at DESC
LIMIT
10
"
`)
})

test('format invalid SQL query', async () => {
Expand All @@ -35,12 +59,17 @@ test('format invalid SQL query', async () => {
path: '/query/format',
payload: { query: 'SELECT FROM WHERE;' },
})
// Even invalid SQL can be formatted
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('text/plain')
expect(res.body).toBeTruthy()
expect(res.body).toMatchInlineSnapshot(`
"SELECT
FROM
WHERE;
"
`)
})

// TODO(andrew): Those should return 400 error code for invalid parameter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe 200 and do nothing?
empty string isnt smth bad really

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed 200 is fine here. I think just like formatters in code editors it shouldn't fail when there are syntax errors; rather it should try to format the rest of the code despite the errors.

test('format empty query', async () => {
const res = await app.inject({
method: 'POST',
Expand Down
16 changes: 6 additions & 10 deletions test/server/generators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ test('typescript generator route', async () => {
})
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('text/plain')
expect(res.body).toBeTruthy()
expect(res.body).contain('public')
})

test('go generator route', async () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, not a test

Expand All @@ -34,22 +34,18 @@ test('swift generator route', async () => {
test('generator routes with includedSchemas parameter', async () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid, but a lot of actual issues in the code may be missed, would be better to include one or a few of multiple existing schemas and check that only they are included

const res = await app.inject({
method: 'GET',
path: '/generators/typescript?includedSchemas=public',
path: '/generators/typescript?included_schemas=private',
})
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('text/plain')
expect(res.body).toBeTruthy()
// the only schema is excluded database should be empty
expect(res.body).toContain('Database = {}')
})

// Skip this test as the OpenAPI endpoint is not implemented
test.skip('openapi generator route', async () => {
test('invalid generator route', async () => {
const res = await app.inject({
method: 'GET',
path: '/generators/openapi',
})
expect(res.statusCode).toBe(200)
expect(res.headers['content-type']).toContain('application/json')
const body = JSON.parse(res.body)
expect(body.openapi).toBeTruthy()
expect(body.paths).toBeTruthy()
expect(res.statusCode).toBe(404)
})
5 changes: 2 additions & 3 deletions test/server/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ test('policy list filtering', async () => {
test('policy list with specific included schema', async () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to create some in public and in other schema(s) first (we prob have only 1)

const res = await app.inject({
method: 'GET',
path: '/policies?includedSchemas=public',
path: '/policies?included_schema=public',
})
expect(res.statusCode).toBe(200)
const policies = res.json()
Expand Down Expand Up @@ -48,8 +48,7 @@ test('create policy with missing required field', async () => {
command: 'PERMISSIVE',
},
})
// The API returns 500 instead of 400 for invalid parameters
expect(res.statusCode).toBe(500)
expect(res.statusCode).toBe(400)
})

test('update policy with invalid id', async () => {
Expand Down
1 change: 1 addition & 0 deletions test/server/publications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ test('create publication with invalid options', async () => {
},
})
// API accepts invalid type and converts it
// TODO: This should error out with invalid parameter
expect(res.statusCode).toBe(200)
})

Expand Down
38 changes: 6 additions & 32 deletions test/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,6 @@ test('type list with specific included schema', async () => {
})
})

// Skip this test as it seems array types aren't included in the way we expect
test.skip('type list with includeArrayTypes parameter', async () => {
const res = await app.inject({
method: 'GET',
path: '/types?includeArrayTypes=true',
})
expect(res.statusCode).toBe(200)
const types = res.json()
expect(Array.isArray(types)).toBe(true)
// Should include array types
const arrayTypes = types.filter((type) => type.name.startsWith('_'))
expect(arrayTypes.length).toBeGreaterThan(0)
})

test('type list excluding array types', async () => {
const res = await app.inject({
method: 'GET',
Expand All @@ -61,28 +47,16 @@ test('type with invalid id', async () => {
expect(res.statusCode).toBe(404)
})

// Skip enum test as we're having issues finding a valid enum type
test.skip('type with enum values', async () => {
test('type with enum values', async () => {
// Find an enum type first
const listRes = await app.inject({
method: 'GET',
path: '/types?filter.type_type=eq.e',
path: '/types',
})
expect(listRes.statusCode).toBe(200)
const enumTypes = listRes.json()
const types = listRes.json()
const enumType = types.find((t) => t.name === 'meme_status')

if (enumTypes.length > 0) {
const enumTypeId = enumTypes[0].id
const res = await app.inject({
method: 'GET',
path: `/types/${enumTypeId}`,
})
expect(res.statusCode).toBe(200)
const type = res.json()
expect(type).toHaveProperty('enums')
expect(Array.isArray(type.enums)).toBe(true)
} else {
// Skip if no enum types are found
console.log('No enum types found, skipping enum values test')
}
expect(Array.isArray(enumType.enums)).toBe(true)
expect(enumType.enums.length).toBeGreaterThan(0)
})