[Bugfix:API] Fix DatetimeFieldOverflow error handling

[sahilsuman933]

Fixes #3331

This change incorporates an additional error from psycopg2 within the try-catch exception block in DatetimeFieldOverflow to handle out-of-range date values. Due to this, we encountered an UnboundLocalError, which shouldn't have been the case; instead, it should have been an error in the format of InvalidDateFormatAPIException.

Screenshots

Error Message:

After implementing this error handling case:

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the develop 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.

[Anish9901]

Hey, @sahilsuman933 please add a test case for this.

[sahilsuman933]

There is no need for an additional test case because there is already a test case that should cover this issue. You can verify this by checking the test case 99/99/9999. Previously, it resulted in a 500 Internal Server Error, but after my fix, it no longer throws that HTTP error.

Test Case:

SRC: /mathesar/tests/api/test_record_api.py

Before my fix:

After my fix:

// @Anish9901

[Anish9901]

There is no need for an additional test case because there is already a test case that should cover this issue. You can verify this by checking the test case 99/99/9999. Previously, it resulted in a 500 Internal Server Error, but after my fix, it no longer throws that HTTP error.

Was this the case while running the pytest as well? As from the video in the issue description it seems like the exception was getting thown the 2nd time someone tried to add an invalid date.

[sahilsuman933]

Was this the case while running the pytest as well?

No, I didn't encounter any issues while running the pytest. When I manually tested it with the table data used by pytest, it ran without any problems. Surprisingly, it didn't even raise an error for the value 99/99/9999, which should have triggered an "invalid_date_format" issue.

Instead of returning a count of 0, it should throw an error for an invalid date. However, this was not the case with the test database used by pytest.

I am using patents table data:

Now, when I tried using it with another test data, specifically the "invalid_reference_base_table.csv" table, it started throwing an "InvalidDateFormat" error.

As from the video in the issue description it seems like the exception was getting thown the 2nd time someone tried to add an invalid date.

If you carefully observe the video, you'll notice that the date format should be in MM-DD-YYYY, and the person was attempting to enter a value greater than 12, which caused the "DatetimeFieldOverflow" error. This is because there are only 12 months in a year.

[Anish9901]

No, I didn't encounter any issues while running the pytest. When I manually tested it with the table data used by pytest, it ran without any problems. Surprisingly, it didn't even raise an error for the value 99/99/9999, which should have triggered an "invalid_date_format" issue.

This is because the type of the column "Patent Expiration Date" in your dataset is TEXT instead of DATE.

If you carefully observe the video, you'll notice that the date format should be in MM-DD-YYYY, and the person was attempting to enter a value greater than 12, which caused the "DatetimeFieldOverflow" error. This is because there are only 12 months in a year.

Ah, that makes sense.

[sahilsuman933]

This is because the type of the column "Patent Expiration Date" in your dataset is TEXT instead of DATE.

True. The issue was due to the data being of text type, preventing pytest detection. Please advise on how to proceed further. Should I make another PR related to it to address this issue or do you want me to do something else?

Apart from that I have implemented the review changes that you requested.

[Anish9901]

I noticed a couple more things since my last review:

• We have a separate exception class for DatetimeFieldOverlflow namely InvalidDateAPIException.
• I was able to reproduce the issue locally using our pytest test suite, and you should be adding a test case addressing this issue, the test case mentioned in your screenshot refer to record update instead of record search you can see the difference by noticing the patch and get requests.
  Look at test_record_search for reference.

[Anish9901]

Please split these to raise InvalidDateFormatAPIException and InvalidDateAPIException respectively.

[Anish9901]

MathesarAPIException would be more appropriate here, check the following for reference.

mathesar/mathesar/api/serializers/records.py

Lines 62 to 63 in d375575

	else:
	raise database_api_exceptions.MathesarAPIException(e, status_code=status.HTTP_400_BAD_REQUEST)

[sahilsuman933]

I have addressed all these changes that you mentioned in my latest commit.

[sahilsuman933]

@Anish9901 The test case is failing and it is because the Patent Expiration Date is being treated as text rather than DATE as we discussed earlier. So, should I address this particular issue in the same pull request or should I create a new pull request for it.

[Anish9901]

@Anish9901 The test case is failing and it is because the Patent Expiration Date is being treated as text rather than DATE as we discussed earlier. So, should I address this particular issue in the same pull request or should I create a new pull request for it.

That is expected, you'd need to change the column type in the test itself.

mathesar/mathesar/tests/api/test_record_api.py

Lines 1414 to 1415 in d375575

	table.alter_column(column_attnum, {'type': 'date'})
	data = {f"{column_id_with_date_type}": "99/99/9999"}

[sahilsuman933]

@Anish9901 Now the tests related to invalid dates are passing. Thank you so much for your guidance :)

[Anish9901]

Alright, looks good now @sahilsuman933. Thanks for your contribution!