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

Implement api schema and docs #250

Merged
merged 56 commits into from
Oct 24, 2024
Merged

Conversation

Timothy-Gonzalez
Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez commented Oct 23, 2024

Resolves #218
Resolves #219
Resolves #222

This is a massive pr, and massive is an understatement. In essense, every endpoint now uses a special specification middleware.

This middleware takes in zod schemas and other metadata in order to both validate incoming requests, type the req and res express objects, and generate documentation with openapi + swagger.

Here's an example:

eventsRouter.get(
    "/staff/",
    specification({
        method: "get",
        path: "/event/staff/",
        tag: Tag.EVENT,
        role: Role.STAFF,
        summary: "Gets all staff events",
        responses: {
            [StatusCode.SuccessOK]: {
                description: "The events",
                schema: PublicEventsSchema,
            },
            [StatusCode.ClientErrorNotFound]: {
                description: "Couldn't find the event specified",
                schema: EventNotFoundErrorSchema,
            },
        },
    }),
    async (_req, res) => {
        const events = await Models.Event.find({ isStaff: true });
        const filteredEvents = events.map(filterEvent);
        return res.status(StatusCode.SuccessOK).send({ events: filteredEvents });
    },
);

There are several breaking changes from this:
changes:

  • Reverted v2:
    • GET /user/v2-qr/ -> GET /user/qr/
    • GET /shop/v2/ -> GET /shop/
  • Changed user following to be consistent with other following endpoints:
    • GET /user/following/ properly returns followed events as { userId: "123", following: ["event1",...] } instead of { userId: "123", events: ["event1",...] }
  • Changed event following ids to be path parameters instead of body ones:
    • PUT /user/follow/ with { eventId: "event1"} to PUT /user/follow/event1/
    • PUT /user/unfollow/ with { eventId: "event1"} to DELETE /user/unfollow/event1/
  • Fixed point shop buying to include all item properties:
    • POST /shop/item/buy/ with { itemName: "Jacket" } to { name: "Jacket", ..all additional item properties }
  • Fixed the weirdest endpoint, admin rsvp behavior. No changes required to apps, just admin site for this one.
    • GET /admission/rsvp/ no longer returns all rsvps just for staff and only current user for non-staff (why in the first place...) - admin site will need to use new GET /admission/rsvp/staff/ endpoint to get all rsvps
    • other changes that were on unused endpoints

There's still a lot to clean up, but at least now we're consistent. The biggest move going forward is adding more tests, as it was much harder to do these changes without them.

- `GET /user/v2-qr/` -> `GET /user/qr/`
- `GET /user/following/` properly returns followed events as
  `{ userId: "123", following: ["event1",...] }` instead of
  `{ userId: "123", events: ["event1",...] }`
- `PUT /user/follow/` with `{ eventId: "event1"}` to
  `PUT /user/follow/event1/`
- `PUT /user/unfollow/` with `{ eventId: "event1"}` to
  `DELETE /user/unfollow/event1/`
- Improve checkin to not use RouterError
Instead of /notification/batch -> /notification/send just do
/notification/send since we are moving from serverless

Also changes `GET /notifications/`` response type from `[{..notif1}]`
to `{ notifications: [{..notif1}] }` to be consistent
Not just objects, as an array can be a return type
We did it!
Also reverted v2 change: `GET /shop/v2/` -> `GET /shop/`
Cleaned up `POST /shop/item/buy/` with `{ itemName: "Jacket" }` to
`{ name: "Jacket", ..all additional item properties }`
Changes parameters, query, and body to have empty/unknown default types.
This means using them when not defined in spec will result in an error
instead of being typed as unknown.

As such, this also fixes two missed cases in users and notifications
that were caught by this change.
We need to allow both auth with no required roles and no auth so that
swagger always sends auth header if it exists, even if not required.

Mainly for endpoints like `GET /event/` which change based on roles but
are still public.
Useful when JWT not required but nice
This moves events following from the attendee database to the user
database, since we already treat it that way anyways. All users should
be able to follow events, there's no problem with that.
Mainly doing this for the automatic promise error handling:

https://expressjs.com/en/guide/error-handling.html

```
Starting with Express 5, route handlers and middleware that return a
Promise will call next(value) automatically when they reject or throw
an error
```
@aletya
Copy link
Contributor

aletya commented Oct 24, 2024

HUGE PR! I dug through specification.ts, specification-validator.ts, and model.ts in depth and skimmed through the rest. The code really is a ton cleaner and intuitive which will make future changes easier. Appreciate the work to make everything consistent, especially for the req/res validation and documentation! LGTM :)

@Timothy-Gonzalez Timothy-Gonzalez merged commit 4abda2b into main Oct 24, 2024
4 checks passed
@Timothy-Gonzalez Timothy-Gonzalez deleted the implement-api-schema-and-docs branch October 24, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants