-
Notifications
You must be signed in to change notification settings - Fork 14
✨ [maykinmedia/commonground-api-common#142] Use exception handler registry from commonground-api-common #723
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #723 +/- ##
==========================================
- Coverage 84.98% 84.92% -0.06%
==========================================
Files 141 141
Lines 2843 2832 -11
Branches 224 222 -2
==========================================
- Hits 2416 2405 -11
Misses 375 375
Partials 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f19beff to
9f8bbc0
Compare
| response = self.client.get(self.url, {"type": objecttype_url}) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, since the response is consistent with other projects, with invalid_params, etc., you can use this method to get the errors from vng_api_common.tests import get_validation_errors.
You can apply this for all tests as well.
src/objects/tests/v2/test_filters.py
Outdated
| invalid_params = {p["name"]: p["reason"] for p in data["invalid_params"]} | ||
|
|
||
| self.assertEqual( | ||
| response.json()["type"], | ||
| [ | ||
| f"Select a valid object type. {objecttype_url} is not one of the available choices." | ||
| ], | ||
| invalid_params["type"], | ||
| f"Select a valid object type. {objecttype_url} is not one of the available choices.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| invalid_params = {p["name"]: p["reason"] for p in data["invalid_params"]} | |
| self.assertEqual( | |
| response.json()["type"], | |
| [ | |
| f"Select a valid object type. {objecttype_url} is not one of the available choices." | |
| ], | |
| invalid_params["type"], | |
| f"Select a valid object type. {objecttype_url} is not one of the available choices.", | |
| self.assertEqual( | |
| get_validation_errors(response, "type"), | |
| { | |
| "name": "type", | |
| "code": "invalid_choice", | |
| "reason": f"Select a valid object type. {objecttype_url} is not one of the available choices.", | |
| }, | |
| ) |
danielmursa-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good 👍
requirements/base.in
Outdated
| zgw-consumers[setup-configuration] | ||
| mozilla-django-oidc-db[setup-configuration] | ||
| commonground-api-common[oas] | ||
| commonground-api-common[oas] @ git+https://github.com/maykinmedia/commonground-api-common.git@feature/142-exception-handler-mapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's upgrade this to 2.11.0
| commonground-api-common[oas] @ git+https://github.com/maykinmedia/commonground-api-common.git@feature/142-exception-handler-mapping | |
| commonground-api-common[oas] |
5ee4377 to
0dd574b
Compare
1e83b0b to
6de9948
Compare
…istry from commonground-api-common
6de9948 to
d42b42e
Compare
stevenbal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remarks about tests, looks good otherwise
src/objects/tests/utils.py
Outdated
| def get_invalid_param(response, field): | ||
| data = response.json() | ||
|
|
||
| for param in data.get("invalid_params", []): | ||
| if param["name"] == field: | ||
| return param | ||
|
|
||
| raise AssertionError(f"No invalid_param found for field '{field}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can make use of from vng_api_common.tests import get_validation_errors for this I think
| def get_invalid_param(response, field): | |
| data = response.json() | |
| for param in data.get("invalid_params", []): | |
| if param["name"] == field: | |
| return param | |
| raise AssertionError(f"No invalid_param found for field '{field}'") |
| from sentry_sdk.transport import Transport | ||
|
|
||
| from ..views import exception_handler | ||
| from vng_api_common.exception_handling import exception_handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is already tested in the library: https://github.com/maykinmedia/commonground-api-common/blob/83160a164ce42af546de7ac9f0f6088ad677ba90/tests/test_exception_handler.py#L225C9-L225C42, let's remove this entire test file
src/objects/tests/v2/test_filters.py
Outdated
| "status": 500, | ||
| "detail": "This search operation is not supported by the underlying data store.", | ||
| }, | ||
| response.data["detail"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the assertion on the entire response dict, you can pop the UUID from the dict because that one varies per run
| self.assertEqual(data["code"], "invalid") | ||
| self.assertIn("invalid_params", data) | ||
|
|
||
| ordering_error = next( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can also make use of get_validation_errors
| self.assertEqual(data["code"], "invalid") | ||
| self.assertIn("invalid_params", data) | ||
|
|
||
| type_error = next( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can also make use of get_validation_errors
| self.assertEqual(data["code"], "invalid") | ||
| self.assertIn("invalid_params", data) | ||
|
|
||
| type_error = next( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can also make use of get_validation_errors
…ler registry from commonground-api-common
…on handler registry from commonground-api-common
|
| Branch | feature/142-custom-exception-handlers |
| Testbed | ubuntu-24.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| performance_test/tests/test_objects_list.py::test_objects_api_list_filter_by_object_type | 📈 view plot 🚷 view threshold | 120.51 ms(-2.76%)Baseline: 123.93 ms | 130.13 ms (92.61%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_filter_one_result | 📈 view plot 🚷 view threshold | 21.97 ms(-4.84%)Baseline: 23.09 ms | 24.24 ms (90.63%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_large_page_size_page_1 | 📈 view plot 🚷 view threshold | 309.98 ms(-0.38%)Baseline: 311.16 ms | 326.72 ms (94.88%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_large_page_size_page_5 | 📈 view plot 🚷 view threshold | 303.03 ms(-2.33%)Baseline: 310.25 ms | 325.76 ms (93.02%) |
| performance_test/tests/test_objects_list.py::test_objects_api_list_small_page_size_page_20 | 📈 view plot 🚷 view threshold | 130.31 ms(-2.68%)Baseline: 133.89 ms | 140.59 ms (92.69%) |
…exception handler registry from commonground-api-common
… Use exception handler registry from commonground-api-common
…mmon#142] Use exception handler registry from commonground-api-common
…-api-common#142] Use exception handler registry from commonground-api-common
|
|
||
| data = response.json() | ||
|
|
||
| invalid_params = {p["name"]: p for p in data["invalid_params"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| invalid_params = {p["name"]: p for p in data["invalid_params"]} | |
| error = get_validation_errors(response, "type") |
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| data = response.json() | ||
| invalid_params = {p["name"]: p["reason"] for p in data["invalid_params"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OlhaZahoruiko I think you can use this method everywhere to get the error dict
@stevenbal should we also add the key for validations that are not provided? like here
objects-api/src/objects/api/validators.py
Line 93 in ff5f242
| raise serializers.ValidationError(message, code=code) |
| invalid_params = {p["name"]: p["reason"] for p in data["invalid_params"]} | |
| error = get_validation_errors(response, "") |
Fixes maykinmedia/commonground-api-common#142
Changes
[Describe the changes here]