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

Added API error codes and implemented 404 on file endpoints #350

Merged
merged 2 commits into from
May 14, 2024

Conversation

wpfl-dbt
Copy link
Collaborator

@wpfl-dbt wpfl-dbt commented May 14, 2024

Context

Our endpoints should provide meaningful errors. This PR adds 404, and implements it on relevant file endpoints.

On the changes to the E2E tests, the existing make test-integration wasn't working, and the existing solution would never extend to E2E tests involving the LLM, as we can't put API keys in .env.integration. A blended approach of pytest using the existing .env, but overwriting a few key vars, makes more sense.

Changes proposed in this pull request

  • Adds pydantic error models inspired loosely by the RFC 9457 error specification, opening to door to following it properly in future
  • Adds 404 error codes to file endpoints in core-api
  • Adds unit tests to pick up these error codes
  • Converts unit tests to using http.HTTPStatus instead of hard-coded error numbers
  • Edits local E2E tests so they run and don't destroy your .env

Guidance to review

  • Check you're happy with the RFC-lite approach
  • Ensure you're happy with my specific implementation of the error codes -- have I included the right fields? Is my copy what you'd expect? Would you be happy extending these objects?
  • Check you're happy with my changes to the E2E tests. Are these variables okay in pyproject.toml rather than .env.integration?

Relevant links

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

Copy link
Contributor

@brunns brunns left a comment

Choose a reason for hiding this comment

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

Nitpicks.

core_api/src/routes/file.py Outdated Show resolved Hide resolved
core_api/src/routes/file.py Outdated Show resolved Hide resolved
core_api/src/routes/file.py Outdated Show resolved Hide resolved
core_api/tests/routes/test_file.py Outdated Show resolved Hide resolved
@wpfl-dbt wpfl-dbt marked this pull request as ready for review May 14, 2024 08:48
@wpfl-dbt wpfl-dbt requested a review from brunns May 14, 2024 08:54
Copy link
Contributor

@brunns brunns left a comment

Choose a reason for hiding this comment

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

You might want to check the Makefile check with @gecBurton. Aside from that, LGTM.

Makefile Outdated Show resolved Hide resolved
@wpfl-dbt wpfl-dbt requested a review from gecBurton May 14, 2024 09:21
Copy link
Collaborator

@gecBurton gecBurton left a comment

Choose a reason for hiding this comment

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

lovely stuff!

@wpfl-dbt wpfl-dbt merged commit 71d0d84 into main May 14, 2024
14 checks passed
@wpfl-dbt wpfl-dbt deleted the feature/file404 branch May 14, 2024 11:23
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.

3 participants