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

Remove res.status calculation as it is set later #19214

Merged

Conversation

u12206050
Copy link
Contributor

res.status is anyways set in either flow later in this function, so no need to loop through errors just for this.

Only edge case is when there are no errors, which would anyways just mean 500 as you can see.

@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2023

🦋 Changeset detected

Latest commit: ecb425e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@u12206050
Copy link
Contributor Author

Actually, it is better to say that status is overridden later in the flow, so this removed logic, is actually just useless.

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

There's no guarantee that errors[0].status is 500, so this will break the http error status code in case an error is thrown with a non-500 error code (like ForbiddenException) right?

@u12206050
Copy link
Contributor Author

There's no guarantee that errors[0].status is 500, so this will break the http error status code in case an error is thrown with a non-500 error code (like ForbiddenException) right?

No, but then errors.length is at least 1 so then the logic that follows will execute which if it isn't a DirectusError will set the status accordingly.

@rijkvanzanten
Copy link
Member

Ahh right right, I wasn't looking further than the diff that github presented ^^

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

The original intended behavior was to use the err.status as the status code if it existed, or fall back to 500 if multiple errors of different types are thrown or if a non-directus error is thrown. That wasn't working as intended before either, but lets make that work as part of this PR. Small tweak incoming!

@rijkvanzanten rijkvanzanten merged commit ad1207a into directus:main Jul 21, 2023
5 of 6 checks passed
@github-actions github-actions bot added this to the Next Release milestone Jul 21, 2023
sethkaufee pushed a commit to sethkaufee/directus that referenced this pull request Jul 24, 2023
* Remove res.status calculation as it is set later

* Ensure 500 is thrown on multiple errors of different types

* Add changeset

---------

Co-authored-by: rijkvanzanten <rijkvanzanten@me.com>
br-rafaelbarros pushed a commit to personal-forks/directus-source that referenced this pull request Nov 7, 2023
* Remove res.status calculation as it is set later

* Ensure 500 is thrown on multiple errors of different types

* Add changeset

---------

Co-authored-by: rijkvanzanten <rijkvanzanten@me.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants