Fix Not Null Violation to return detailed error response

[silentninja]

Fixes #1051

Extracts details from Postgres diagnostics and returns a detailed error response that consists of the the column name causing the exception along with other details

// Sample error response
[
  {
    'code': 4204,
    'message': 'null value in column "Case Number" of relation "NASA Record Create" violates not-null constraint',
    'field': None,
    'detail': {
      'row_parameters': {
        'Center': 'NASA Example Space Center',
        'Status': 'Application',
        'Case Number': None,
        'Patent Number': '01234',
        'Application SN': '01/000,001',
        'Title': 'Example Patent Name',
        'Patent Expiration Date': ''
      },
      'row_detail': 'Failing row contains (1394, NASA Example Space Center, Application, null, 01234, 01/000,001, Example Patent Name, ).',
      'column': 'Case Number'
    }
  }
]

Screenshots

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[seancolsen]

I'll review this.

[seancolsen]

• Can we return the column id instead of the column name?
• If I try to add a new record to table that has two NOT NULL columns, I only see the error for the first column. I'd like the response to give me enough info to display error messages within both cells. I think it would be best to handle this case by returning two separate error objects within the top-level array of the JSON response body. But I'm open to other approaches too.

[kgodey]

Some general comments about the format, I haven't looked at the code.

• Why do we need row_parameters in the response? Also, why is it called row?, We call it record everywhere else.
• We need to be able to return errors for multiple columns, a single column key isn't going to work.
• We should be using IDs to identify columns to be consistent with the rest of the API. We should also be using PKs to identify records.

[seancolsen]

Why do we need row_parameters in the response?

I agree with @kgodey on this too. I don't see the use in that param.

[seancolsen]

@kgodey

We need to be able to return errors for multiple columns, a single column key isn't going to work.

What do you think about my suggestion above?

I think it would be best to handle this case by returning two separate error objects within the top-level array of the JSON response body

[silentninja]

• Why do we need row_parameters in the response? Also, why is it called row?, We call it record everywhere else.

I will rename row to record. As for row_paramters, I thought it would be helpful information when debugging as it makes sure the information received by the database is correct. I can remove it if it seems unnecessary.

• We need to be able to return errors for multiple columns, a single column key isn't going to work.

This is the actual behaviour of the database, it returns just the first column responsible for the not null violation.

[seancolsen]

Reactions

• @pavish I'd like to loop you into this convo too. The goal of this PR is to help the front end get enough info to display per-cell error messages. @silentninja said that it's hard for us to get info from Postgres about multiple constraint violations at once.

  This is the actual behaviour of the database, it returns just the first column responsible for the not null violation.

Thoughts about the larger picture

Here are some situations where the API should respond with error info if an attempt to POST to records results in an error from Postgres:

• (a) NULL value for a NOT NULL column (as Provide detailed error info for POST to records API when in violation of a not-null constraint #1051 describes)
• (b) value in violation of a unique constraint
• (c) value that does not fit within the column's specified length
• (d) FK constraint violation
• ...yada yada

For some of these situations (e.g. (a) and (c)), the front end already has sufficient info to perform client-side validation. Others (e.g. (b), (d)), it's not feasible for the front end to perform the validation.

@pavish and I briefly discussed using client-side validation to satisfy #775 but I argued that we should opt for server-side validation to reduce complexity.

Moving forward

• From the perspective of designing an intuitive API, the challenge of getting multiple errors from Postgres seems problematic.

• However, from the more pragmatic perspective of our existing front end, we might be abe to cope with this limitation somewhat easily if we use client-side validation. Let's see if others agree with my train of thought (especially @pavish)...

• As far as I can tell, the front end currently has two situations where it makes POST requests to the records API:

  • When updating the value of an existing cell

    Here, we only send one field at a time, because it's only one cell. So the API will never need to respond with multiple errors. Even in situations like (b) and (d) above, we'd still be okay because I don't imagine we'd ever need to display two errors for the same cell.

    Further (and only relevant to situation (a) above), if I recall, our specs for setting a single cell to NULL (not yet implemented) require client-side validation anyway.

  • When adding a new record

    Here we always send an empty object as the request ({}).

    I think we should only need to worry about errors from situation (a) above. Right @kgodey @silentninja? In theory, a table could have a default value set for a column which also contains a unique constraint. That would generate a (b) type error. But that seems really unlikely. Maybe there's a situation I'm not considering though.

    If I'm not seeing some special failure scenario where the attempt to add a new empty row violates a constraint which only the server can validate, then we'll need to put some more thought into this!

If my train of thought is correct here, then we could rely on client-side validation to solve #775 (although at the cost of increasing complexity on the front end). That would change the priority of #1051 from "blocking important work" to "nice to have someday".

[kgodey]

I will rename row to record. As for row_paramters, I thought it would be helpful information when debugging as it makes sure the information received by the database is correct. I can remove it if it seems unnecessary.

I think we should remove it. Let's keep errors minimal and only have the information that's needed.

I think it would be best to handle this case by returning two separate error objects within the top-level array of the JSON response body.

Since the database can't return multiple errors, I think the current format is fine. @silentninja I do think we should rename the column field to column_id for clarity.

--

Generally, I agree with the approach of the frontend valdiating what it can with the information that it is. For one thing, it's the fastest feedback loop for the user.

I think we should only need to worry about errors from situation (a) above.

I do think (b) errors are possible, but it should be unlikely since those columns would have to have been created outside of Mathesar. I think in that case, it's fine for the server validation to be incomplete, we can just show a frontend error for the first cell with an error.

[seancolsen]

I feel good about merging this PR once @kgodey's concerns are addressed.

I have some more thoughts regarding implementation of #775 that I will raise somewhere else.

[pavish]

@seancolsen I'll add some quick thoughts here.

• I would prefer performing frontend validations wherever possible, for a better UX for the user. We can validate (a) on the frontend before sending the request.
• For (b), I do think it's possible to show targeted error messages. When there's an error for the unique constraint due to the default value, if the server could respond with the name of the constraint that failed, we can use that to highlight all associated columns.

[seancolsen]

if the server could respond with the name of the constraint that failed, we can use that to highlight all associated columns.

This is a great idea, and addresses a concern that's been rattling around in my head about how we'll deal with the case of a row that violates a multi-column unique constraint.

For this PR, I think it makes sense to stick with column_id as a way to identify the column (since, strictly speaking, there's no actual "constraint" for the column). But we'll need to consider other approaches once we take up the validation of unique constraints. I've opened #1059 for that.

[codecov-commenter]

Codecov Report

Merging #1053 (2d67ba7) into master (f58106f) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1053      +/-   ##
==========================================
+ Coverage   92.56%   92.70%   +0.13%     
==========================================
  Files         108      108              
  Lines        3848     3852       +4     
==========================================
+ Hits         3562     3571       +9     
+ Misses        286      281       -5     

Flag	Coverage Δ
pytest-backend	92.70% <100.00%> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
mathesar/api/db/viewsets/records.py	100.00% <100.00%> (+1.69%)	⬆️
...r/api/exceptions/database_exceptions/exceptions.py	97.10% <100.00%> (+6.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f58106f...2d67ba7. Read the comment docs.

[kgodey]

Looks good to me.

[kgodey]

I'll leave it to @seancolsen to approve and merge.