DataJunction / dj

A metrics platform.
http://datajunction.io
MIT License
35 stars 15 forks source link

Better input validation on get_sql_for_metrics(). #1227

Closed agorajek closed 1 day ago

agorajek commented 5 days ago

Summary

We did not properly check the input to /sql api. This PR does a better job, example:

  1. When metrics arg has a non-existing node:
    • before: 'NoneType' object has no attribute 'current'
    • after: datajunction_server.errors.DJNodeNotFound: A node with namefoo.bar.bazdoes not exist.
  2. When metrics argument has a non-metric node:
    • before: {"message":"","errors":[{"code":204,"message":"","debug":null,"context":""}],"warnings":[]}
    • after: datajunction_server.errors.DJInvalidInputException: All nodes must be of metric type, but some are not: system.logs.cube (cube).

Test Plan

Locally.

Deployment Plan

auto

netlify[bot] commented 5 days ago

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
Latest commit f5200425727760d483e8273b9efdf94472e6346c
Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/673fda37084dbf00088ce6d4
agorajek commented 4 days ago

Is that error 'NoneType' object has no attribute 'current' coming from here? https://github.com/DataJunction/dj/blob/main/datajunction-server/datajunction_server/api/helpers.py#L365-L380

Nope. It was coming from here: https://github.com/DataJunction/dj/blob/main/datajunction-server/datajunction_server/database/queryrequest.py#L313

samredai commented 4 days ago

Is that error 'NoneType' object has no attribute 'current' coming from here? https://github.com/DataJunction/dj/blob/main/datajunction-server/datajunction_server/api/helpers.py#L365-L380

Nope. It was coming from here: https://github.com/DataJunction/dj/blob/main/datajunction-server/datajunction_server/database/queryrequest.py#L313

I see. I don't want to hold this PR up but I'm worried about having to perform the query for the metric nodes twice--once to make sure they exist, and then again when we're generating the sql. I suspect there's a way to just make the second time we call it robust enough to raise the node not found error like you did here.