From d55ea82343255f1a6e4fd7c9145f8380f10289b2 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Thu, 4 Nov 2021 12:32:26 -0500
Subject: [PATCH] Handle api/tiles requests for native queries (#18810)
* Handle api/tiles requests for native queries
Ordinarily the api takes the source query and adds in a filter clause
for a region:
```clojure
{:database 19
:query {:source-table 88
:fields [[:field 562 nil]
[:field 574 nil]
[:field 576 nil]]
:limit 2000}
:type :query}
{:database 19
:query {:source-table 88
:fields [[:field 562 nil]
[:field 574 nil]
[:field 576 nil]]
:limit 2000
:filter [:inside
[:field 576 nil]
[:field 574 nil]
-40.97989944013222
-179.99999564141046
-66.51326189011354
-134.99999673105785]}
:type :query
:async? false}
```
But when native, this would break, for three different reasons:
- native queries don't necessarily (or possibly) know their field
ids. So the api request it would construct would have `undefined` in the
slot where the field-id would normally go. This would cause the route to
404 as it would fail to match the expected numeric part of the url
- just passing in the field name isn't suffiencient, because mbql cannot
natively use an mbql filter clause into a native snippet. We do this to
a limited extent with filters and substitution but this requires a
marker like `{{filter}}` placeholder for us to add the filter text.
- when using `[:field name ...]` type field references we need the
base-type of a field in order to construct a query.
So the solution:
- allow the route to take field ids or names
- if native, rewrite the query to be a nested query using the native
query as the source query. ie, "select name, lat, long from table" ->
"select source.* from (select name, lat, long from table) source" but in
mbql so we can add our fliter clause
- if a string name is passed in, annotate it with a base-type of
:type/Float since we are dealing with lats and longs.
A concern was that we need to include the source-metadata. I'm omitting
this because we are essentially "select * from nested" so we just take
what we want, and the route already expects the index of the lat and
long columns and then selects only those from the query results.
As it stands we end up with a working query but a log
```
2021-11-02 14:37:25,470 DEBUG middleware.log :: GET /api/tiles/2/1/1/latitude/longitude/1/2/ 200 47.3 ms (5 DB calls) App DB connections: 2/13 Jetty threads: 9/50 (2 idle, 0 queued) (128 total active threads) Queries in flight: 0 (0 queued); postgres DB 19 connections: 3/6 (0 threads blocked)
2021-11-02 14:37:25,474 WARN middleware.add-implicit-clauses :: Warning: cannot determine fields for an explicit `source-query` unless you also include `source-metadata`.
2021-11-02 14:37:25,490 WARN middleware.add-implicit-clauses :: Warning: cannot determine fields for an explicit `source-query` unless you also include `source-metadata`.
2021-11-02 14:37:25,501 DEBUG middleware.log :: GET /api/tiles/2/0/2/latitude/longitude/1/2/ 200 39.4 ms (5 DB calls) App DB connections: 1/13 Jetty threads: 8/50 (2 idle, 0 queued) (128 total active threads) Queries in flight: 0 (0 queued); postgres DB 19 connections: 2/6 (0 threads blocked)
```
It might be possible to suppress these logs when we are in a query
context of `:map-tiles`. Or we might be able to require the frontend to
send along the metadata to include in the query. I didn't do this yet
since it seems to work fine and it would just make the urls quite long
if there are lots of columns (and the metadata can get quite verbose).
```clojure
;; source query of the card
{:type :native
:native {:query "select name, latitude, longitude from zomato limit 2000;"
:template-tags {}}
:database 19}
;; nest it into a source query
{:database 19
:type :query
:query {:source-query {:template-tags {}
:native "select name, latitude, longitude from zomato limit 2000;"}}}
;; add the `:inside` filter for the tile
{:database 19
:type :query
:query {:source-query {:template-tags {}
:native "select name, latitude, longitude from zomato limit 2000;"}
:filter [:inside
[:field
"latitude"
{:base-type :type/Float}]
[:field
"longitude"
{:base-type :type/Float}]
0.0
89.99999782070523
-66.51326189011354
179.99999564141046]}
:async? false}
```
* Add docstring and make function private
* url encode column names to api/tiles
---
.../components/LeafletTilePinMap.jsx | 4 +-
src/metabase/api/tiles.clj | 64 +++++++++++++------
test/metabase/api/tiles_test.clj | 32 +++++++---
3 files changed, 68 insertions(+), 32 deletions(-)
diff --git a/frontend/src/metabase/visualizations/components/LeafletTilePinMap.jsx b/frontend/src/metabase/visualizations/components/LeafletTilePinMap.jsx
index 052da6ddd04..a3cc88de8f2 100644
--- a/frontend/src/metabase/visualizations/components/LeafletTilePinMap.jsx
+++ b/frontend/src/metabase/visualizations/components/LeafletTilePinMap.jsx
@@ -48,9 +48,9 @@ export default class LeafletTilePinMap extends LeafletMap {
"/" +
coord.y +
"/" +
- latitudeField.id +
+ (latitudeField.id || encodeURIComponent(latitudeField.name)) +
"/" +
- longitudeField.id +
+ (longitudeField.id || encodeURIComponent(longitudeField.name)) +
"/" +
latitudeIndex +
"/" +
diff --git a/src/metabase/api/tiles.clj b/src/metabase/api/tiles.clj
index a206a51a304..dd6785026c7 100644
--- a/src/metabase/api/tiles.clj
+++ b/src/metabase/api/tiles.clj
@@ -1,6 +1,7 @@
(ns metabase.api.tiles
"`/api/tiles` endpoints."
(:require [cheshire.core :as json]
+ [clojure.set :as set]
[compojure.core :refer [GET]]
[metabase.api.common :as api]
[metabase.mbql.normalize :as normalize]
@@ -8,7 +9,8 @@
[metabase.query-processor :as qp]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
- [metabase.util.schema :as su])
+ [metabase.util.schema :as su]
+ [schema.core :as s])
(:import java.awt.Color
java.awt.image.BufferedImage
[java.io ByteArrayInputStream ByteArrayOutputStream]
@@ -47,13 +49,14 @@
{:lat lat, :lon lon}))
(defn- query-with-inside-filter
- "Add an `INSIDE` filter to the given query to restrict results to a bounding box"
- [details lat-field-id lon-field-id x y zoom]
+ "Add an `INSIDE` filter to the given query to restrict results to a bounding box. The fields passed in can be either
+ integer field ids or string field names. When a field name, the `base-type` will be set to `:type/Float`."
+ [details lat-field lon-field x y zoom]
(let [top-left (x+y+zoom->lat-lon x y zoom)
bottom-right (x+y+zoom->lat-lon (inc x) (inc y) zoom)
inside-filter [:inside
- [:field lat-field-id nil]
- [:field lon-field-id nil]
+ [:field lat-field (when (string? lat-field) {:base-type :type/Float})]
+ [:field lon-field (when (string? lon-field) {:base-type :type/Float})]
(top-left :lat)
(top-left :lon)
(bottom-right :lat)
@@ -109,40 +112,59 @@
(u/ignore-exceptions
(.close output-stream))))))
+(defn- native->source-query
+ "Adjust native queries to be an mbql from a source query so we can add the filter clause."
+ [query]
+ (if (contains? query :native)
+ (let [native (set/rename-keys (:native query) {:query :native})]
+ {:database (:database query)
+ :type :query
+ :query {:source-query native}})
+ query))
;;; ---------------------------------------------------- ENDPOINT ----------------------------------------------------
+(defn- int-or-string
+ "Parse a string into an integer if it can be otherwise return the string. Intended to determine whether something is a
+ field id or a field name."
+ [x]
+ (try (Integer/parseInt x)
+ (catch NumberFormatException _ x)))
+
;; TODO - this can be reworked to be `defendpoint-async` instead
;;
;; TODO - this should reduce results from the QP in a streaming fashion instead of requiring them all to be in memory
;; at the same time
-(api/defendpoint GET "/:zoom/:x/:y/:lat-field-id/:lon-field-id/:lat-col-idx/:lon-col-idx/"
+(api/defendpoint GET "/:zoom/:x/:y/:lat-field/:lon-field/:lat-col-idx/:lon-col-idx/"
"This endpoints provides an image with the appropriate pins rendered given a MBQL `query` (passed as a GET query
string param). We evaluate the query and find the set of lat/lon pairs which are relevant and then render the
appropriate ones. It's expected that to render a full map view several calls will be made to this endpoint in
parallel."
- [zoom x y lat-field-id lon-field-id lat-col-idx lon-col-idx query]
- {zoom su/IntString
- x su/IntString
- y su/IntString
- lat-field-id su/IntGreaterThanZero
- lon-field-id su/IntGreaterThanZero
- lat-col-idx su/IntString
- lon-col-idx su/IntString
- query su/JSONString}
- (let [zoom (Integer/parseInt zoom)
- x (Integer/parseInt x)
- y (Integer/parseInt y)
- lat-col-idx (Integer/parseInt lat-col-idx)
- lon-col-idx (Integer/parseInt lon-col-idx)
+ [zoom x y lat-field lon-field lat-col-idx lon-col-idx query]
+ {zoom su/IntString
+ x su/IntString
+ y su/IntString
+ lat-field s/Str
+ lon-field s/Str
+ lat-col-idx su/IntString
+ lon-col-idx su/IntString
+ query su/JSONString}
+ (let [zoom (Integer/parseInt zoom)
+ x (Integer/parseInt x)
+ y (Integer/parseInt y)
+ lat-col-idx (Integer/parseInt lat-col-idx)
+ lon-col-idx (Integer/parseInt lon-col-idx)
query
(normalize/normalize (json/parse-string query keyword))
updated-query
(-> query
- (update :query query-with-inside-filter lat-field-id lon-field-id x y zoom)
+ native->source-query
+ (update :query query-with-inside-filter
+ (int-or-string lat-field)
+ (int-or-string lon-field) x y zoom)
(assoc :async? false))
{:keys [status], {:keys [rows]} :data, :as result}
diff --git a/test/metabase/api/tiles_test.clj b/test/metabase/api/tiles_test.clj
index 55d08669123..c7619722552 100644
--- a/test/metabase/api/tiles_test.clj
+++ b/test/metabase/api/tiles_test.clj
@@ -2,6 +2,7 @@
"Tests for `/api/tiles` endpoints."
(:require [cheshire.core :as json]
[clojure.test :refer :all]
+ [metabase.query-processor :as qp]
[metabase.test :as mt]
[schema.core :as s]))
@@ -10,15 +11,28 @@
(drop 1 (take 4 s))))
(deftest basic-test
- (testing "GET /api/tiles/:zoom/:x/:y/:lat-field-id/:lon-field-id/:lat-col-idx/:lon-col-idx/"
- (is (png? (mt/user-http-request
- :rasta :get 200 (format "tiles/1/1/1/%d/%d/1/1/"
- (mt/id :venues :latitude)
- (mt/id :venues :longitude))
- :query (json/generate-string
- {:database (mt/id)
- :type :query
- :query {:source-table (mt/id :venues)}}))))))
+ (let [venues-query {:database (mt/id)
+ :type :query
+ :query {:source-table (mt/id :venues)
+ :fields [[:field (mt/id :venues :name) nil]
+ [:field (mt/id :venues :latitude) nil]
+ [:field (mt/id :venues :longitude) nil]]}}]
+ (testing "GET /api/tiles/:zoom/:x/:y/:lat-field-id/:lon-field-id/:lat-col-idx/:lon-col-idx/"
+ (is (png? (mt/user-http-request
+ :rasta :get 200 (format "tiles/1/1/1/%d/%d/1/2/"
+ (mt/id :venues :latitude)
+ (mt/id :venues :longitude))
+ :query (json/generate-string venues-query)))))
+ (testing "Works on native queries"
+ (let [native-query {:query (:query (qp/query->native venues-query))
+ :template-tags {}}]
+ (is (png? (mt/user-http-request
+ :rasta :get 200 (format "tiles/1/1/1/%s/%s/1/2/"
+ "LATITUDE" "LONGITUDE")
+ :query (json/generate-string
+ {:database (mt/id)
+ :type :native
+ :native native-query}))))))))
(deftest failure-test
(testing "if the query fails, don't attempt to generate a map without any points -- the endpoint should return a 400"
--
GitLab