-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: auth typings and reduced type ambiguity #4562
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
Merged
+1,047
−21
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,8 @@ export interface RequestAuth< | |
| * set to false. | ||
| */ | ||
| isAuthorized: boolean; | ||
| /** true if the request has been authenticated via the `server.inject()` `auth` option, otherwise `undefined`. */ | ||
| isInjected?: boolean | undefined; | ||
| /** the route authentication mode. */ | ||
| mode: AuthMode; | ||
| /** the name of the strategy used. */ | ||
|
|
@@ -250,7 +252,7 @@ export interface RequestLog { | |
| } | ||
|
|
||
| export interface RequestQuery { | ||
| [key: string]: any; | ||
| [key: string]: string | string[] | undefined; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -269,9 +271,9 @@ export interface InternalRequestDefaults { | |
|
|
||
| Payload: stream.Readable | Buffer | string | object; | ||
| Query: RequestQuery; | ||
| Params: Record<string, any>; | ||
| Params: Record<string, string>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the actual type is the result of the validation step. |
||
| Pres: Record<string, any>; | ||
| Headers: Record<string, any>; | ||
| Headers: Record<string, string | string[] | undefined>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the actual type is the result of the validation step. |
||
| RequestApp: RequestApplicationState; | ||
|
|
||
| AuthUser: UserCredentials; | ||
|
|
@@ -303,11 +305,7 @@ export type ReqRef = Partial<Record<keyof ReqRefDefaults, unknown>>; | |
| /** | ||
| * Utilities for merging request refs and other things | ||
| */ | ||
| export type MergeType<T, U> = { | ||
| [K in keyof T]: K extends keyof U | ||
| ? U[K] | ||
| : T[K]; | ||
| }; | ||
| export type MergeType<T, U> = Omit<T, keyof U> & U; | ||
|
|
||
| export type MergeRefs<T extends ReqRef> = MergeType<ReqRefDefaults, T>; | ||
|
|
||
|
|
@@ -441,7 +439,7 @@ export interface Request<Refs extends ReqRef = ReqRefDefaults> extends Podium { | |
| /** | ||
| * Same as pre but represented as the response object created by the pre method. | ||
| */ | ||
| readonly preResponses: Record<string, any>; | ||
| readonly preResponses: Record<string, unknown>; | ||
|
|
||
| /** | ||
| * By default the object outputted from node's URL parse() method. | ||
|
|
@@ -474,7 +472,7 @@ export interface Request<Refs extends ReqRef = ReqRefDefaults> extends Podium { | |
| /** | ||
| * An object containing parsed HTTP state information (cookies) where each key is the cookie name and value is the matching cookie content after processing using any registered cookie definition. | ||
| */ | ||
| readonly state: Record<string, any>; | ||
| readonly state: Record<string, unknown>; | ||
|
|
||
| /** | ||
| * The parsed request URI. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is going on here? This is a bad choice. After validation the value can be anything. Parsing to a
numberor aDateare valid choices. This should probably just beunknown.Uh oh!
There was an error while loading. Please reload this page.
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.
The thing is: If you don't validate, it will most certainly be
string | string[] | undefined. This is the actual possibility of runtime data types Hapi would produce.If you do validate, it's whatever you've set with Joi, which is a custom definition, and depends on you not using
schema.strict(), which would makestring | string[] | undefinedtrue again. At that point you might as well include a customServerRouteif you're depending on the types to be correct:unknown | unknown[] | undefinedwould imply: reality depends on the user's Joi usage and Joi configuration for ALL routes— Joi is the source of runtime truth. This seems like the wrong assumption.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 understand your point and it definitely has merits, but ultimately I think it is flawed.
Header, query, and param validation and transformation is core to using Hapi, so it doesn't make sense to have a default type that only safely applies when it is not used. It does not have to be Joi, but some kind of validation and possible transformation is very much expected.
Whether the default should be
unknownoranycan be debated, but given Hapi tends to prefer safety and strictness I would probably go withunknown, so any parameter usage would have to be explicit.Btw, for query params the types are even more uncertain, since it also depends on the
parserserver option. With eg. the suggestedqsmodule it can also contain plain objects, which is now impossible to type, as it is not a subset ofstring | string[] | undefined.…fails with this error: