Skip to content

Conversation

@thunderbird7413
Copy link
Contributor

Summary

This PR fixes multiple cases where API views were throwing 500 Internal Server Errors due to unhandled native Python exceptions.
These failures were previously worked around in Issue #8 using assertRaises. This change addresses the root cause by adding proper exception handling in the views and updating the test suite accordingly.

What was fixed

API View Error Handling

  • Added basic exception handling to prevent crashes on invalid input
  • Views now return 400 Bad Request instead of 500 errors for:

Malformed tags
Non-numeric coordinates
Invalid / malformed JSON

Files updated

mainapp/api.py

Wrapped logic in lookup_tag, search_range, and search_full
Catches ValueError and json.JSONDecodeError
Returns HttpResponseBadRequest (400)

Routing Improvements

  • Relaxed URL regex for search_range so invalid parameters reach the view instead of returning 404.
  • Reordered URL patterns to avoid conflicts (search_title before search_range).

Files updated

mainapp/urls.py

Test Suite Updates

  • Updated API tests to assert 400 responses instead of raised exceptions.
  • Removed reliance on assertRaises (Issue Add Preliminary Test Coverage #8 workaround).
  • Mocked external GLB validation to prevent OS-specific failures.
  • Fixed filesystem and path assertions to work reliably on Windows.

Files updated

  • test_lookup.py
  • test_model.py
  • test_forms.py
  • test_nightly.py
  • mixins.py

Verification

  • python manage.py test mainapp.tests.api.test_lookup
  • Full test suite: 118 tests passing
  • No unhandled exceptions
image

Copy link
Collaborator

@AyushDharDubey AyushDharDubey left a comment

Choose a reason for hiding this comment

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

Apart from these, as a general practice, try to segregate different changes in meaningful independent commits (for eg, .gitignore change, settings.py change, test addition/updates, should have been separate commits)

Also, It would be nice to see fix for:

# def test_revise_non_existent_model_id(self):
# response = self.client.get(reverse('revise', args=[99999999]))
# self.assertEqual(response.status_code, 404)

Copy link
Collaborator

@AyushDharDubey AyushDharDubey left a comment

Choose a reason for hiding this comment

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

Apart from these, there are commented out pieces of code from the previous iteration which needs to be removed

@thunderbird7413 thunderbird7413 force-pushed the fix/handle-500-errors-in-views branch 2 times, most recently from f5fd142 to cb4b308 Compare December 25, 2025 07:54
Copy link
Collaborator

@AyushDharDubey AyushDharDubey left a comment

Choose a reason for hiding this comment

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

Just a few tiny nit picking and then we are ready to merge.
Also, this PR has been bloated with commits along with the review iterations, would you be able to squash the commits into 2 or 3 with meaningful commit messages and force push.

@AyushDharDubey
Copy link
Collaborator

Also, some of the tests could be extended to test the output more thoroughly. In particular, it is always good to have tests to ensure that something does not happen

Originally proposed by @lonvia in #8 (review)

In case you would you like to enhance the test suite further, this one would need to go in a separate PR.

- Handle malformed JSON safely
- Improve error handling in API views
- Relax and reorder URL regex patterns
- Update API tests for search_full
- Organize code into correct files
- Remove redundant logic
- Restore mixins.py
- Restore .gitignore
- Stabilize branch after refactor
@thunderbird7413 thunderbird7413 force-pushed the fix/handle-500-errors-in-views branch from 36883a0 to ef5b883 Compare December 29, 2025 15:18
@AyushDharDubey AyushDharDubey merged commit 429a549 into fossgis:main Dec 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants