Updated stratifier behavior #129
Conversation
dogversioning
commented
Oct 16, 2024
dogversioning
commented
- Fixes stratifier to prevent returning all data
- Adds a counts section to stratifier to match new vintage of chart data API
dogversioning
force-pushed
the
mg/stratifier_update
branch
from
b2951c3
to
408539d
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| @@ -61,29 +62,33 @@ def _build_query(query_params: dict, filters: list, path_params: dict) -> str: | |||
| select_str = f"{query_params['stratifier']}, {select_str}" | |||
| group_str = f"{query_params['stratifier']}, {group_str}" | |||
| columns.remove(query_params["stratifier"]) | |||
| strat_str = f'AND {query_params["stratifier"]} IS NOT NULL ' | |||
There was a problem hiding this comment.
I'm not seeing any quoting of these query params? We should probably be escaping them, then adding double quotes (or verifying that they match [a-zA-Z]+), yeah? That's what the ruff warning S608 is about, which is quieted below, yeah.
Does not need to block this PR, but feels important.
There was a problem hiding this comment.
yeah i was just thinking about how gross this approach to SQL construction was - I may make a ticket for this rather than deal with it as part of the PR
| if "filter" in query_params and filters == []: | ||
| filters = [query_params["filter"]] |
There was a problem hiding this comment.
Why the filters == [] check (which could be not filters I think)? Do we want to ignore a filter query entirely if multiValueQueryStringParameters is set?
There was a problem hiding this comment.
in experimenting with this - I can't explictly say in cloudformation 'this parameter can be an array', so i'm not sure if it's going to be in queryStringParameters or multiValueQueryStringParameters in the AWS inbound event to the lambda. so I need to check both.
There was a problem hiding this comment.
I guess I'm not 100% sure of the expected usage here, but maybe combine the two arrays? It just felt odd to me that you'd ignore the filter query param in the case you get both.
There was a problem hiding this comment.
if we got both (which I don't think should happen), queryStringParameters would have to be a subset of multiValueQueryStringParameters
