GTC-2659: "include_attribute" query param does not behave as expected #130
Conversation
Attributes enum from nasa_viirs was not appropriate for the generic vector tile endpoint
No longer used. SupportedAttributes enum is now used.
…hey are valuable again.
gtempus
force-pushed
the
gtc-2663--agg-rules-for-validation
branch
from
151c3fa to
8490bf0
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #130 +/- ##
==========================================
- Coverage 79.49% 78.74% -0.76%
==========================================
Files 53 53
Lines 1780 1778 -2
==========================================
- Hits 1415 1400 -15
- Misses 365 378 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We moved fields into the `field_metadata` table in the GTC-1347 Metadata epic: https://gfw.atlassian.net/browse/GTC-1347 Keeping it in the query only causes confusion.
gtempus
force-pushed
the
gtc-2663--agg-rules-for-validation
branch
from
44db630 to
563b71a
Compare
gtempus
force-pushed
the
gtc-2663--agg-rules-for-validation
branch
from
1db2c5a to
d143171
Compare
gtempus
changed the title
| elif len(rows) == 0: | ||
| logger.warning("Zero rows. There are no tile caches registered with the API") | ||
| else: | ||
| logger.warning(f"Found {len(rows)} rows") | ||
| # tile_caches = rows |
There was a problem hiding this comment.
I'll admit having no tile caches is a rare situation (likely only in dev) but, in the interest of being thorough, let's clean up this bit which I vaguely recall we added while pairing. :)
The problem is that we check if rows is None (which it looks like it might be if there are no rows: Gino returns None, SQA would return an empty iterable IIRC), but then go on to do a for "for row in rows". If rows is None this will break because None is not iterable. So I think I would reduce this selection to logging a warning and then exiting early. For example:
if rows is None:
logger.warning("There are no tile caches registered with the API")
return []
for row in rows:
...
There was a problem hiding this comment.
Good catch, @dmannarino! 💪
I hadn't looked at that code since we paired on it.
I did refactor it. However, the function returns a dict. What I wanted to ensure is that rows are iterable as you suggest. Then the list comprehension never runs and the function returns the tile cache dict as before.
What do you think? My changes are in this commit: 8071e29
There was a problem hiding this comment.
Mmm, that looks good! I forgot the return value was a dictionary.
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Nasa Viirs Fire Alerts Tile Endpoint
Two main issues surfaced when tasked with supporting FLAG-914
The
include_attributequery param lists all of the possible attributes for the latest version of the dataset at the time the server was started. The uptime of the server should not be a factor in what data is available. I belive this design was implemented to populate the developer docs with proper attributes in the form of an enumeration. However, it also turned out to be misleading for clients of this endpoint and leading to the second issue.This was the biggest point of confusion for clients. When the dynamic tile is created, it's possible that several of the points can represent more than one detected fire. Each point has an attribute:
count, and it's a value from 1 to n.When multiple points are involved, aggregation of any included attributes must be calculated. For example,
latitutudeandlongitude(if included in theinclude_attributesURL as a query param) need to be averaged.There was only a subset of all possible attributes with these types of aggregation rules and they were hard-coded into a
dictin the source.Even though an attribute was advertised as available to include (see issue one, above) it would cause the API to return a
500 Server Errorwhen a client tried to pass it as a value to theinclude_attributequery param.Issue Number: GTC-2659
What is the new behavior?
The advertised attributes to include are now only the ones in which aggregation rules exist. Those
SupportedAttributesare an enhanced enum that stores the aggregation code with the enumeration so there can be no error when submitting a request withinclude_attribute.For future requests to add another attribute, an enum will be added to the collection with a corresponding aggregation rule in one place in the code. No other changes will be necessary.
Also, attributes for the version are validated per request (requests are still cached) instead of when the server is stood up. Therefore, should older versions be requested, error responses can be much more helpful to the developer.
Does this introduce a breaking change?
Testing
There is an updated
nasa_viirs_fire_alertsdataset instaging. Here is an example request that uses severalinclude_attributeparams:https://staging-tiles.globalforestwatch.org/nasa_viirs_fire_alerts/v20231227/dynamic/0/0/0.pbf?start_date=2023-12-01&end_date=2023-12-31&high_confidence_only=false&force_date_range=True&include_attribute=alert__date&include_attribute=alert__time_utc&include_attribute=bright_ti4__K&include_attribute=frp__MW&include_attribute=latitude&include_attribute=longitude&include_attribute=confidence__cat&include_attribute=umd_tree_cover_density_2000__threshold&include_attribute=umd_tree_cover_density__threshold
Other Notes
I will follow up this PR with more developer experience (DX) improvements to documentation and I'll also add appropriate unit tests. The existing tests are far too dependent on the database and other technologies and need to be decoupled.