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

Error 500 from records API when in violation of a not-null constraint #859

Closed
Tracked by #889
seancolsen opened this issue Dec 1, 2021 · 5 comments · Fixed by #1017
Closed
Tracked by #889

Error 500 from records API when in violation of a not-null constraint #859

seancolsen opened this issue Dec 1, 2021 · 5 comments · Fixed by #1017
Assignees
Labels
ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Dec 1, 2021

Reproduce

  1. Use the front end
  2. Open a table where at least one column has a NOT NULL constraint set
  3. Click the + icon at the bottom left of the table to begin adding a new record.
  4. Observe a request to http://localhost:8000/api/v0/tables/24/records/ that
  5. Observe HTTP response code of 500. The response needs to conform to our error structure, to-be-decided in Define common error structure #560. In particular, for Improve UX when adding new records to a table with a NOT NULL column #775 I need a structured machine-readable way to know which column(s) were in violation of the constraint.

Related to #858

@kgodey
Copy link
Contributor

kgodey commented Dec 3, 2021

Moving this to the "Working with Tables" milestone since it blocks #775.

@seancolsen seancolsen removed the needs: unblocking Blocked by other work label Dec 12, 2021
@seancolsen
Copy link
Contributor Author

Marking this is unblocked since #560 is complete.

@dmos62
Copy link
Contributor

dmos62 commented Dec 14, 2021

Unless we split this into two issues, this is still blocked. The two issues would be:

  1. handle the input, so that we would return an HTTP 4xx instead of 500;
  2. adhere the API response to the new error spec.

Task 2 is still blocked pending error reporting implementation/concretization tracked in #883.

Edit: oh, I see @silentninja is alread on this.

@seancolsen
Copy link
Contributor Author

seancolsen commented Dec 14, 2021

@dmos62 As I see it, #883 is a large issue that requires many small tasks, one of which would be closing this ticket. To put that another way, to close #883 we'll need to make sure all our endpoints, and all the HTTP methods, and all error scenarios adhere to to the new structure. That's potentially a lot of small independent tasks. This ticket is tracking one error scenario for one HTTP method for one endpoint. And unlike a lot of the other small tasks within #883, this one is blocking a front end ticket, so I think it's worth keeping this ticket separate from #883 and prioritizing it over all the other small tasks which #883 encompasses. No need to close #883 before starting work on this ticket.

silentninja added a commit that referenced this issue Dec 20, 2021
Return proper error response according to the api spec when a row insert violates null value constraint
@silentninja silentninja linked a pull request Dec 20, 2021 that will close this issue
7 tasks
@silentninja silentninja linked a pull request Jan 25, 2022 that will close this issue
7 tasks
silentninja added a commit that referenced this issue Feb 1, 2022
Fixes #859 Return proper error response on not null violation
@seancolsen
Copy link
Contributor Author

#1017 didn't fully address the concerns raised in this ticket.

Specifically, in step 5:

I need a structured machine-readable way to know which column(s) were in violation of the constraint.

As such, I've opened #1051 to track progress on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants