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: correct error for missing context field #3647

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Apr 28, 2023

When adding a strategy using a context field that did not exist, we threw an unknown error.

This changes to throw NotFoundError so that our users can better know what they did wrong.

When adding a strategy using a context field that did not exist, we
threw an unknown error. This changes to throw NotFoundError so that our
users can better know what they did wrong.
@vercel
Copy link

vercel bot commented Apr 28, 2023

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

2 Ignored Deployments
Name Status Preview Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Apr 28, 2023 9:13am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Apr 28, 2023 9:13am

@chriswk chriswk enabled auto-merge (squash) April 28, 2023 09:18
Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Interesting. I'd usually expect NotFoundErrors to be 404s. But if what can't be found is a nested part of a request object, shouldn't it be a 400 because the data is bad? If so, how do we differentiate between "route doesn't exist" and "your data is bad because it points to something that doesn't exist"?

src/lib/db/context-field-store.ts Show resolved Hide resolved
@chriswk chriswk merged commit f1db90d into main Apr 28, 2023
8 checks passed
@chriswk chriswk deleted the task/strategyWithNonExistingContextField branch April 28, 2023 09:21
@chriswk
Copy link
Contributor Author

chriswk commented Apr 28, 2023

We discussed the same thing, and came to the conclusion that the 404 is still a better error message than 500 UnknownError.

The next step is having a strategy for the validation of constraints that changes the 404 to 400.

chriswk pushed a commit that referenced this pull request Apr 28, 2023
When adding a strategy using a context field that did not exist, we
threw an unknown error.

This changes to throw NotFoundError so that our users can better know
what they did wrong.
Copy link
Contributor

@andreas-unleash andreas-unleash left a comment

Choose a reason for hiding this comment

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

LGTM!

thomasheartman pushed a commit that referenced this pull request Apr 28, 2023
When adding a strategy using a context field that did not exist, we
threw an unknown error.

This changes to throw NotFoundError so that our users can better know
what they did wrong.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants