Conversation
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
nice tool to test the api "validations" |
tdethier
left a comment
tdethier
left a comment
There was a problem hiding this comment.
general comments in any order:
- this is a great starting point, thanks for the huge work 👍
- i like that the API is aware of all valid params and can refuse unknown ones instead of ignoring them
- i like that the API can suggest valid options in case of spelling mistakes for instance
- i don't like that adding a new field somewhere is much harder - the same field has to be added in:
- a serializer (which is ok)
- the whist list of fields (would it be possible to not make them and generate the white list based off serializer attributes instead?)
- in
batch_xxxmethods to actually fetch data - in
batch_xxx_for_querysetmethods to fetch data for exports - in
only_columns_for_field_tree(i'm not sure why) - maybe in other places as well? this sounds very complicated to maintain
- i don't like how csv/xlsx exports are handled: most of the code is more or less duplicated, it's gonna be really hard to maintain + it reuses the same old "get_row" logic with inner functions like old endpoints
- we should have more tests
- the API test file (
test_org_units.py) should be split in test_views/filters/pagination/... like we usually do - i didn't check if there were features/fields/... that exist in the current API but are missing in this new version
- we should handle the security alerts that github raised, they make sense
- we should get inputs from other team members and decide on how we go from there - personally, i like the added features, but i think the maintenance cost is too high compared to their added value (at least in the current version); getting rid of some new features, or implementing them differently might be better? i'm not sure
| @@ -0,0 +1,99 @@ | |||
| """Parser for the v3 `fields=` query parameter sub-selector grammar. | |||
There was a problem hiding this comment.
we should add unit tests for all helpers in this file
| CORE_EXTRA_ALLOWED_PARAMS: FrozenSet[str] = frozenset({"order", "format", "page", "page_size", "with_count", "fields"}) | ||
|
|
||
|
|
||
| class BaseV3FilterSet(django_filters.FilterSet): |
There was a problem hiding this comment.
i would also add unit tests to check the behavior of this class, with filters that inherit from it and define extra_allowed_params or not
| return # To not perform the csrf check previously happening | ||
|
|
||
|
|
||
| class CsrfExemptSessionAuthenticationScheme(OpenApiAuthenticationExtension): |
There was a problem hiding this comment.
not sure why we have this, was it needed for swagger?
There was a problem hiding this comment.
it's to be able to test the schema generation :
iaso/tests/api/v3/test_openapi_schema.py::V3SchemaDocumentationTestCase::test_v3_endpoints_generate_a_clean_openapi_schema
| def __init__(self, *args, geometry_field: str, org_unit_model, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.geometry_field = geometry_field | ||
| self.org_unit_model = org_unit_model |
There was a problem hiding this comment.
not sure why you need to pass the OrgUnit model here, if you're checking geom and simplified_geom, you already know that it's this model and you don't need to pass it? otherwise it sounds like we have multiple models that store geo data, with the same attribute names, but i don't think we do
| self.assertEqual(row["name"], "Theed District") | ||
| self.assertEqual(row["ancestors[0].name"], "Naboo") # root | ||
| self.assertEqual(row["ancestors[1].name"], "Theed") # immediate parent |
There was a problem hiding this comment.
here as well, i would assert with values from OrgUnit objects from the setup instead of hardcoding values with comments
There was a problem hiding this comment.
to be honest I fixed it but I don't like that... it finally feels your "test" is too smart and don't detect anything (like playing with mock, and you don't even realize that you test your mock and not the implementation)
| self.assertIn("parqet", data["error"]) | ||
| self.assertIn("parquet", data["detail"]) | ||
|
|
||
| def test_format_csv(self): |
There was a problem hiding this comment.
for all csv tests: there's a self.assertCsvFileResponse() utility you can use
| content = b"".join(response.streaming_content).decode("utf-8") | ||
| self.assertIn("Theed", content) | ||
|
|
||
| def test_format_xlsx(self): |
There was a problem hiding this comment.
and for all xlsx tests, there's a self.assertXlsxFileResponse() utility you can use
| main_query = next( | ||
| q["sql"] for q in ctx.captured_queries if q["sql"].strip().startswith('SELECT "iaso_orgunit".') | ||
| ) | ||
| # `LEFT OUTER JOIN` is what `select_related` would add; the account-scoping subquery | ||
| # (`filter_for_account`) legitimately uses `INNER JOIN` in its own nested SELECT, which is fine. | ||
| self.assertNotIn("LEFT OUTER JOIN", main_query) |
There was a problem hiding this comment.
are you sure that the first query with 'SELECT "iaso_orgunit".' will be the right one? are they always in the right order? can't there be another query on this table before the "big" query with select/prefetch related?
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assert_parquet_content_type(response) | ||
| with open("/tmp/v3_orgunits_test.parquet", "wb") as f: | ||
| write_response_to_file(response, f) |
There was a problem hiding this comment.
no assert after writing the file? what's the point of writing the file then?
This branch has not been deployed
3 participants
What problem is this PR solving?
proposition for a new orgunit api
Related JIRA tickets
IA-XXX, WC2-XXX, POLIO-XXX, POLIOPM-XXX SLEEP-XXX, SNT-XXX, CONSOLE-XXX
Changes
mainly addition (new v3 name space)
main changes
/api/v3/orgunits/?ancestor_id=<country_id>&location__outside_org_unit=<country_id>&fields=id,name,latitude,longitude,ancestors(id,name)/api/v3/orgunits?location__outside_bbox=-8.60,4.19,-2.49,10.74&fields=id,name,latitude,longitude,ancestors(id,name)How to test
Explain how to test your PR.
If a specific config is required explain it here: dataset, account, profile, etc.
Print screen / video
Upload here print screens or videos showing the changes.
Notes
in the things I noticed we don't have indexes for search by name.
Doc
Tell us where the doc can be found (docs folder, wiki, in the code...).