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

Handle validation, errors, and business logic in the API consistently #623

Closed
kgodey opened this issue Sep 2, 2021 · 7 comments · Fixed by #1016
Closed

Handle validation, errors, and business logic in the API consistently #623

kgodey opened this issue Sep 2, 2021 · 7 comments · Fixed by #1016
Assignees
Labels
affects: dx Related to developer experience affects: technical debt Improves the state of the codebase ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: documentation Improvements or additions to documentation

Comments

@kgodey
Copy link
Contributor

kgodey commented Sep 2, 2021

Problem

Currently, we have validation, error handling, and business logic spread across the db module, models, viewsets, and serializers to make the API work. This is confusing to work with.

Proposed solution

  • Organize the code with a clear separation of concerns.
  • Document how the code is organized for reference.

Additional context

Related to:

@kgodey kgodey added work: documentation Improvements or additions to documentation type: enhancement New feature or request affects: dx Related to developer experience affects: technical debt Improves the state of the codebase work: backend Related to Python, Django, and simple SQL ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue labels Sep 2, 2021
@kgodey kgodey added this to the 06. 2021-09 Stability milestone Sep 2, 2021
@kgodey kgodey self-assigned this Sep 2, 2021
@kgodey kgodey removed their assignment Sep 28, 2021
@kgodey kgodey modified the milestones: [06] 2021-10 Stability, [08A] 2021-10 improvements Sep 28, 2021
@silentninja
Copy link
Contributor

silentninja commented Nov 16, 2021

Few suggestions to handle errors.

  1. Extending ApiException - This could allow us to raise exceptions other than validation errors
  2. Using context managers - This is more Java-esque, but should suit our usecase well though as we have similar error handling in viewsets and would help in capturing non API errors from other modules and converting them in an appropriate API response

@mathemancer
Copy link
Contributor

I like the context manager setup, but I don't see how it's exclusive with the ApiException idea.

@silentninja
Copy link
Contributor

silentninja commented Nov 17, 2021

ApiException is limited to Django rest and cannot be used outside the service layer, while context manager is useful for capturing Custom errors imported from other modules and converting it into a proper API error with custom status codes

@dmos62
Copy link
Contributor

dmos62 commented Nov 17, 2021

I feel good about using context managers for error handling.

@kgodey
Copy link
Contributor Author

kgodey commented Nov 17, 2021

Now that we have a larger backend team, I think it would be good for whoever takes this issue to write up a technical spec and get it reviewed before proceeding with the refactor.

@mathemancer
Copy link
Contributor

One thing to keep in mind when speccing this out is that assuming multiple context managers that we'd want to potentially combine to catch different groups of exceptions (for example, we could have one exclusive to the Django part of the code that just deals in the APIException children), we'll need to think a bit about how to make them compose associatively and (hopefully) symmetrically. We don't want to have different exceptions and messages depending on the order in which context-management decorators are applied.

  • We could try to ensure that there's never any overlap between two, but I don't think that's feasible (also, we may well want to catch a parent exception with a more general message if a child exception doesn't match, and these could be in separate wrappers).
  • We could also require all such composition be done inside another decorator, and only allow applying one exception handling decorator to a given function, so we avoid ad-hoc combinations that could produce different results.

@kgodey
Copy link
Contributor Author

kgodey commented Dec 14, 2021

@silentninja I assigned this to you based on the conversation in #883.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: dx Related to developer experience affects: technical debt Improves the state of the codebase ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL work: documentation Improvements or additions to documentation
Projects
No open projects
4 participants