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

fix request logging middleware status code reporting #3316

Merged

Conversation

dillonstreator
Copy link
Contributor

@dillonstreator dillonstreator commented Nov 14, 2023

With the logRequests config enabled, the request logging middleware reports 200 status code regardless of non-200 status codes being sent to the client.

res.on('finish', () => {}) should be used to properly hook into response being sent and report the actual status code.

❗ Note this PR also changes the log property requestMethod -> method. @mattwiller called this out in his comment #3196 (comment) on the PR introducing this logging middleware. At this point, it would technically be considered a breaking change so I'd understand if this is not desired and is preferred to be kept as requestMethod.

@dillonstreator dillonstreator requested a review from a team as a code owner November 14, 2023 00:25
Copy link

vercel bot commented Nov 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
medplum-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 14, 2023 1:03am
medplum-www ⬜️ Ignored (Inspect) Visit Preview Nov 14, 2023 1:03am

Copy link

vercel bot commented Nov 14, 2023

Someone is attempting to deploy a commit to the Medplum Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

sweep-ai bot commented Nov 14, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

@mattwiller
Copy link
Member

Thanks @dillonstreator! ✨

@mattwiller mattwiller added this pull request to the merge queue Nov 14, 2023
Merged via the queue into medplum:main with commit be16119 Nov 14, 2023
12 checks passed
codyebberson added a commit that referenced this pull request Nov 14, 2023
Fixes #3269 - use shawl for agent service admin (#3314)
fix request logging middleware status code reporting (#3316)
Docusaurus 3 (#3311)
Redirect to next from signin page if already authenticated (#3294)
Ensure trailing slash on external auth signin page (#3306)
Fix Admin Users doc page title (#3299)
Document setting up Google auth for Medplum App (#3302)
Update domain level idp docs (#3298)
Added separate scripts for 'build' and 'build:all' (#3303)
Dependency upgrades (#3285)
Make @medplum/core a module (#3263)
feat(client/fhircast): add `fhircastGetContext` (#3292)
Add some margin to ResourceArrayInput and make it flex-based (#3295)
Don't show copy button for ID if empty/missing (#3297)
Add link to Tasks guide for charting demo README (#3288)
Show language and text when nested (#3296)
Fixed projects guide link (#3281)
Fix highlighting of default tab (#3293)
Remove option for 1 AZ in AWS setup (#3283)
codyebberson added a commit that referenced this pull request Nov 14, 2023
Fixes #3269 - use shawl for agent service admin (#3314)
fix request logging middleware status code reporting (#3316)
Docusaurus 3 (#3311)
Redirect to next from signin page if already authenticated (#3294)
Ensure trailing slash on external auth signin page (#3306)
Fix Admin Users doc page title (#3299)
Document setting up Google auth for Medplum App (#3302)
Update domain level idp docs (#3298)
Added separate scripts for 'build' and 'build:all' (#3303)
Dependency upgrades (#3285)
Make @medplum/core a module (#3263)
feat(client/fhircast): add `fhircastGetContext` (#3292)
Add some margin to ResourceArrayInput and make it flex-based (#3295)
Don't show copy button for ID if empty/missing (#3297)
Add link to Tasks guide for charting demo README (#3288)
Show language and text when nested (#3296)
Fixed projects guide link (#3281)
Fix highlighting of default tab (#3293)
Remove option for 1 AZ in AWS setup (#3283)
@reshmakh reshmakh added this to the November 30, 2023 milestone Dec 1, 2023
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

3 participants