Feedback #22
Description
Hey OultimoCoder, I've been working with your boilerplate for a while now and have some initial feedback on it. First of all though, thanks for your effort in putting it together - it's a definite time saver.
- API contracts
At the moment your API contracts are enforced in the validations folder by code such as user.validation.ts. E.g. you have:
export const createUser = z.strictObject({
email: z.string().email(),
password: z.string().superRefine(password).transform(hashPassword),
name: z.string(),
is_email_verified: z
.any()
.optional()
.transform(() => false),
role: roleZodType
})
export type CreateUser = z.infer<typeof createUser>
The naming convention I think could do with some improvement. E.g. I have renamed createUser to createUserValidator and CreateUser to CreateUserRequest. I have also renamed the validations folder to contracts and changed the filenames, e.g. auth.contract.ts. Having these is great, but I feel they should also be used in your integration tests. Your tests use this:
let newUser: MockUser which in turn is based on export type MockUser = Insertable<UserTable>. I can see why you do it, because you need to insert the test data and create the post request. However if you refactor your user fixture along these lines, you can stay true to the contracts but also use them for data population. (Extra user fields in my version!)
export const admin: CreateUserRequest = {
username: '3',
name: faker.person.fullName(),
email: faker.internet.email().toLowerCase(),
password,
role: 'admin',
language: 'en'
}
export const insertUsers = async (
users: CreateUserRequest[],
databaseConfig: Config['database'],
additionalFields: Partial<UserTable> = {} // Pass any additional fields for data population
) => {
const hashedUsers = users.map((user) => ({
...user,
password: user.password ? hashedPassword : null
}))
const client = getDBClient(databaseConfig)
const results: string[] = []
for await (const user of hashedUsers) {
let userId = nanoid()
const result = await client.insertInto('user').values({ id: userId, ...user, ...additionalFields }).executeTakeFirst()
results.push(userId)
}
return results
}
Your user fixture also implements the UserResponse interface which I think should be made official in the contract file. I implement mine like this:
export const userResponseValidator = z.object({
id: z.string(),
verified_at: z.string().nullable()
})
.merge(createUserRequestValidator)
.omit({ password: true })
export type UserResponse = z.infer<typeof userResponseValidator>
Ideally there should probably be a corresponding response type for each of the possible requests. This makes your API schema contract pretty solid and you can then:
- Return typed JSON from your controllers (see here):
return c.jsonT<UserResponse>(user, httpStatus.OK as StatusCode)(although I haven't tried this yet) - Test against it.
Not sure this is a great explanation, so happy to upload you better examples if you want.
- Object IDs
One less thing to think about is using something like nanoid to generate random IDs for objects, in particular user IDs - it just eliminates an attack surface by not having predictable user IDs.
- Email verification
I changed is_email_verified to verified_at just to give me more info as to when it was verified.
- Snake case versus camel case
I personally don't like leaking snake case verified_at in JSON responses. You can use middleware to convert snake case inbound to camelcase and the reverse on the way out. This is how it looks:
import camelcaseKeys from 'camelcase-keys';
import snakecaseKeys from 'snakecase-keys';
app.use('*', async (c, next) => {
if (c.req.method === 'POST') {
let bodyText = await c.req.raw.clone().text()
let jsonObj = tryParseJSONObject(bodyText)
if (jsonObj && !(jsonObj instanceof Error)) {
c.json(snakecaseKeys(jsonObj))
}
}
await next()
try {
if (c.res.headers.get('Content-Type')?.startsWith('application/json')) {
const obj = await c.res.json()
c.res = new Response(JSON.stringify(camelcaseKeys(obj, {deep: true}), null, 2), c.res)
}
} catch(e) { console.log(e)}
})
- Handling sensitive fields
If the user wants to change their email address, they should only be allowed to do this following verification. Typically you would create a separate route for this and create a persisted token linked to the event. When the user confirms via the email message, only then is the email changed. I haven't got too far into it, but it looks as if once verified, the user can change his own email address without automatic reverification.
That's it. As I said, thanks for creating this in the first place. Saved me a lot of time.