-
Notifications
You must be signed in to change notification settings - Fork 74
(EAI-1159) Implement middleware in new contentRouter
#816
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
base: search_content_route
Are you sure you want to change the base?
Conversation
… add test for limit
…SearchFilter types
…t.ts, remove zod checks where unnecessary in MongoDbSearchResultsStore
…ds function outside of file to mock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing TS-trivia comment
Has access to the Express.js request and response plus the values | ||
from the {@link Response.locals} object. | ||
*/ | ||
export type AddCustomDataFunc<Req extends Request = Request, Res extends Response = Response> = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this truly generic or is this falling back to the default type parameters of Request?
For example, try
AddCustomDataFunc<Request<{test: true}>>
You get:
Type 'Request<{ test: true; }, any, any, ParsedQs, Record<string, any>>' does not satisfy the constraint 'Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>'.
Because Req extends Request
is actually Req extends Request<core.ParamsDictionary, ...>
.
If customizing Request isn't really going to be used, you can skip the generics here and just use the concrete type (i.e. Request
).
Otherwise you can pass the type parameters: Req extends Request<unknown, ...>
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I really went way farther into the weeds on this one than I needed. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
METAPROGRAMMING IS A TEMPTING POISON APPLE
-Chris Bush
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good. couple small things before approval
}: MakeSearchContentRouteParams) { | ||
return async ( | ||
req: ExpressRequest<SearchContentRequest["params"]>, | ||
req: ExpressRequest<ParamsDictionary>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? seems more generic than what you had before.
also this makes me realize that wen must do an input validation. see next comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see! The main issue was that all of our base Express Request types (created from this) have params as an optional property, but Express Requests always have at least an empty object. I've taken away optional
from this zod type and now can keep the above type to the more specific!
}: MakeSearchContentRouteParams) { | ||
return async ( | ||
req: ExpressRequest<SearchContentRequest["params"]>, | ||
req: ExpressRequest<ParamsDictionary>, | ||
res: ExpressResponse<SearchContentResponseBody, SearchContentRouterLocals> | ||
) => { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should validate input with SearchContentRequestBody.safeParse(req.body)
. and if safeParse encounters an error, you should throw it accordingly. Andrew implemented this pattern in the new Responses API route which you can use as reference https://github.com/mongodb/chatbot/blob/responses_api/packages/mongodb-chatbot-server/src/routes/responses/createResponse.ts#L189-L196
} catch (error) { | ||
throw makeRequestError({ | ||
message: "Error parsing custom data from the request", | ||
stack: (error as Error).stack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to include stack
here? also i don't like this as
, what if the thing being thrown doesn't have stack
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! Removing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference one safe way to do this is:
stack: (error as Partial<Error>)?.stack
You can be almost certain that the thrown object is an Error
, but just in case it isn't it won't further blow up your app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually... just the question mark is needed... if error is anything but an Error, it ?.stack
will just return undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we feel about needing to log the stack trace? In most of our error logs we don't - but we have added it in 5 other places.
import { isValidIp } from "../routes/conversations/utils"; | ||
|
||
export function requireValidIpAddress(): ConversationsMiddleware { | ||
export function requireValidIpAddress< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to chris' comment, if the generics aren't ever being used here (which they dont seem to be), lets remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the usage of the generics to all the invocations of these middlewares! Otherwise, the function definitions would need an unknown for the locals values.
Jira: https://jira.mongodb.org/browse/EAI-1159
Changes
requireValidIpAddress
andrequireRequestOrigin
/processors
contentRouter
andsearchContent
route