DataBiosphere / data-explorer

BSD 3-Clause "New" or "Revised" License
10 stars 6 forks source link

SQL query is now returned by Facets GET API. EXTRA_FACET_INFO is removed from app config. #378

Closed wnojopra closed 4 years ago

wnojopra commented 4 years ago

Fixes #377

Summary of changes:

1) Code that constructs the SQL query was moved out of export_url_controller.py and into its own query_util.py file. 2) The facets_get API in facets_controller.py was updated to call get_sql_query and returns the sql_query in its response. 3) Client side javascript was updated to store the sql_query in app state. Currently, its only use is to be sent back to the server when calling ExportAPIPost. It will be used more in the near future as we implement new UI features that require this sql query. 4) export_url_controller.py no longer calls get_sql_query, instead the sql_query is passed from the client.

You may notice auto-generated code changes in ui/src/api/src/model/.js and api/data_explorer/models/.py. This is due to my changes in api/app.yaml which introduces the sql_query parameter into the FacetsResponse and exportUrlRequest.

wnojopra commented 4 years ago

Nice sleuthing. Making sql_query state in react SG. But if you're going to make it state, does sql_query state update when user clicks around the UI? (It should.)

Yes it does. Clicking around the UI makes a GET facet request. We set the sql_query state in the callback.

Why put sql query in React state, instead of just making the API server sql query computation request-scoped? Latter seems like a better solution. You have everything you need to know to generate sql query in url.

That was also my initial hunch, so good question. Before this PR, The facets_controller cached extra facet data from Elasticsearch into EXTRA_FACET_INFO. The sql query generation code looked at EXTRA_FACET_INFO to determine if a field is time series, and also to get the bucket intervals. Now that we realized this EXTRA_FACET_INFO isn't scoped per request/user-session, I have decided to remove it, and the sql generation code is now missing this information. We could instead update the sql generation code to fetch this data from elasticsearch, but it currently has zero dependencies on elasticsearch and its nice to keep it that way.