Skip to content
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

Introduce zod for server-side validations #4397

Merged
merged 24 commits into from Nov 24, 2022

Conversation

apoorv-mishra
Copy link
Collaborator

@apoorv-mishra apoorv-mishra commented Nov 7, 2022

Todo

  • Cover missing tests for existing assertions
  • Replace DocumentSchema in favor of separate input schemas for each of the routes
    • documents.list
    • documents.archived
    • documents.deleted
    • documents.viewed
    • documents.drafts
    • documents.info
    • documents.export
    • documents.restore
    • documents.search_titles
    • documents.search
    • documents.templatize
    • documents.update
    • documents.move
    • documents.archive
    • documents.delete
    • documents.unpublish
    • documents.import
    • documents.create
  • Type input
  • Middleware to validate input
  • Fix type for ctx in route handlers to accommodate input
  • Replace ctx.request.body refs with ctx.input in all of the route handlers
  • Collate duplicate schemas
  • Restructure /api to accommodate document schema

Towards #4284

@apoorv-mishra apoorv-mishra force-pushed the refactor/4284/server-side-validatations branch 5 times, most recently from 53d0477 to 198d3b8 Compare November 8, 2022 11:14
@tommoor
Copy link
Member

tommoor commented Nov 8, 2022

Refactor to use DocumentSchema in a top level blanket middleware for all documents APIs

I don't think we want this, the validated payload needs to be specific to each endpoint.

@tommoor
Copy link
Member

tommoor commented Nov 8, 2022

I'm not sure if this is already the case but we should make sure that the body is typed correctly as part of this work, this is one of the big advantages of using zod.

@apoorv-mishra
Copy link
Collaborator Author

I don't think we want this, the validated payload needs to be specific to each endpoint.

Thought of doing this earlier, but not pursuing it now. Makes sense to keep it to individual routes.

@apoorv-mishra apoorv-mishra force-pushed the refactor/4284/server-side-validatations branch from be13eba to 70b47bb Compare November 10, 2022 12:15
@apoorv-mishra apoorv-mishra changed the title Use zod for server-side validations Introduce zod for server-side validations Nov 10, 2022
@apoorv-mishra apoorv-mishra force-pushed the refactor/4284/server-side-validatations branch from f0933d9 to 2c11b47 Compare November 11, 2022 15:20
@apoorv-mishra apoorv-mishra marked this pull request as ready for review November 11, 2022 15:27
@auto-assign auto-assign bot requested a review from tommoor November 11, 2022 15:27
Copy link
Member

@tommoor tommoor left a comment

Choose a reason for hiding this comment

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

This is great work 🙏

@@ -0,0 +1,284 @@
import { isEmpty } from "lodash";
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this file into @shared, we can then use it to ensure that requests from the frontend are also valid at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are two things - types and schemas. I assume you're referring to move only types? Not sure if it's required to move schemas into @shared.

Copy link
Member

Choose a reason for hiding this comment

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

I did mean both but it would just be to keep them colocated in that case, as you say I doubt schema would be needed on frontend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we decide to restructure /api as follows, for example,

/api
  \--/attachments
        \--index.ts
        \--attachments.ts
        \--attachments.test.ts
        \--schema.ts

and if we also decide to share the types with frontend, it'd entail separating schemas and their types.

I say we keep them colocated under schema.ts and think about the frontend sharing part later. It's probably overengineering at this point. Thoughts @tommoor ?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, works for me 👍 - one improvement at a time

sort: z
.string()
.refine((val) =>
[...Object.keys(Document.rawAttributes), "index"].includes(val)
Copy link
Member

Choose a reason for hiding this comment

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

Moving to @shared will mean removing the import of Document but that's okay, we should probably be more selective over which attributes can be used for sorting here anyway. Lets start with createdAt, updatedAt, and index

DocumentsImportReq,
DocumentsCreateReqSchema,
DocumentsCreateReq,
} from "@server/routes/api/types";
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a case where it's better to just do something like...

import * as T from "@server/routes/api/types";

T.DocumentsListSchema

Maybe we can lose Req as they all have it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we can lose Req as they all have it?

Didn't quite get. You mean rename, for example DocumentsCreateReqSchema -> DocumentsCreateSchema but keep DocumentsCreateReq unchanged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have Req there purposely to distinguish with Res, in case we have that in future, something like

APIContext<DocumentsCreateReq, DocumentsCreateRes>

Copy link
Member

Choose a reason for hiding this comment

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

ok

Copy link
Member

Choose a reason for hiding this comment

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

rename, for example DocumentsCreateReqSchema -> DocumentsCreateSchema but keep DocumentsCreateReq unchanged?

Sorry, to be clear – this sounds good.

@socket-security
Copy link

socket-security bot commented Nov 12, 2022

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

@apoorv-mishra apoorv-mishra force-pushed the refactor/4284/server-side-validatations branch from c5fde92 to b82c22d Compare November 12, 2022 05:26
@tommoor
Copy link
Member

tommoor commented Nov 12, 2022

@SocketSecurity ignore koa-webpack-hot-middleware@1.0.3

wtf?

@tommoor
Copy link
Member

tommoor commented Nov 15, 2022

Okay, I'm thankful for the extensive tests in api/documents – makes this a little less scary to merge but in hindsight I do wish we'd started with a less important set of endpoints 😅

@apoorv-mishra apoorv-mishra force-pushed the refactor/4284/server-side-validatations branch from 7e27e60 to 9c9ff87 Compare November 23, 2022 11:42
Comment on lines +50 to +62
it("should return published document for urlId", async () => {
const { user, document } = await seed();
const res = await server.post("/api/documents.info", {
body: {
token: user.getJwtToken(),
id: document.urlId,
},
});
const body = await res.json();
expect(res.status).toEqual(200);
expect(body.data.id).toEqual(document.id);
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missed this! Thanks for adding 🙏

@apoorv-mishra apoorv-mishra merged commit a6125be into main Nov 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the refactor/4284/server-side-validatations branch November 24, 2022 04:41
@apoorv-mishra apoorv-mishra mentioned this pull request Nov 29, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants