Skip to content
Snippets Groups Projects
Unverified Commit 191b8165 authored by Mark Bastian's avatar Mark Bastian Committed by GitHub
Browse files

Indexed Entity X-Rays (#33656)


# Primary objective: From an indexed entity, located the items referencing that entity and make a dashboard for that entity. This will make several dashboards, one for each entity, so the strategy is to combine these all into a single dashboard with tabs. If there are no linked entities as simple dashboard indicating that nothing interesting is linked. If there is a single linked entity we create a single dashboard (no tab) but with the same linked entity title.

A few of the main nses that were affected and other bullet point changes:

* metabase.api.automagic-dashboards - The vast majority of the work to create the unified dashboard is found here.
* metabase.automagic-dashboards.core - Docs, cleanup, and destructuring for clarity
* metabase.api.search - Adding fix to realize search results
* metabase.automagic-dashboards.filters - Fix to build-fk-map so that parameters show up on model x-rays
* metabase.automagic-dashboards.populate - Fix typo
* metabase.api.search-test - Unit tests for search fix
* Brought over tab saving of transient x-ray dashboard code to save-transient-dashboard!

---------

## Overall summary

The primary entry point to these changes can be found in `metabase.api.automagic-dashboards` at:

```clojure
(api/defendpoint GET "/model_index/:model-index-id/primary_key/:pk-id" ...
```

A simple reproduction of dashboard creation from indexed entities is shown here:

```clojure
(let [model-index-id 54 ;; This and pk-id will be specific to some indexed entity in your system
      pk-id          1]
  (binding [api/*current-user-permissions-set* (delay #{"/"})
            api/*current-user-id*              1]
    (let [model-index       (t2/select-one ModelIndex :id model-index-id)
          model             (t2/select-one Card (:model_id model-index))
          model-index-value (t2/select-one ModelIndexValue
                                           :model_index_id model-index-id
                                           :model_pk pk-id)
          linked            (#'api.magic/linked-entities
                              {:model             model
                               :model-index       model-index
                               :model-index-value model-index-value})
          dashboard         (#'api.magic/create-linked-dashboard
                              {:model             model
                               :linked-tables     linked
                               :model-index       model-index
                               :model-index-value model-index-value})]
      dashboard)))
```

---------

## Fixed the query filter in `metabase.api.automagic-dashboards` so that `create-linked-dashboard` doe not produce bad queries.

We're no longer making joins back to the model's underlying
table. Recognizing that we were joining on linked table's fk to the
model's underlying table's pk and then searching where pk = <pk-value>,
we can just filter where fk = <pk-value>, omit the join.

So these tests just grab all of the linked tables and assert that one of
those filters is found.

eg, suppose a model based on products. the linked tables are orders and
reviews. And rather than the queries:

```sql
select o.*
from orders o
left join products p
on p.id = o.product_id
where p.id = <pk-value>
```

we can just do

```sql
select *
from orders
where product_id = <pk-value>
```

And similar for reviews. And for each query in the dashboard we're
looking for one of the following:

```clojure
, #{[:= $orders.product_id 1] [:= $reviews.product_id 1]}
```

---------

## Handle expression refs in indexed-models

Custom columns look like the following:

```clojure
{:expressions {"full-name" [:concat <first-name> <last-name>]}
```

To index values, we need a sequence of primary key and the associated
text value. So we select them from a nested query on the model. But the
model's way to refer to an expression and queries _on_ the model are
different. To the model, it's an expression. To queries based on the
model, it's just another field.

And have field refs in the result_metadata `[:expression
"full-name"]`. But when selecting from a nested query, they are just
string based fields: `[:field "full-name" {:base-type :type/Text}]`.

old style query we issued when fetching values:

```clojure
{:database 122
 :type :query
 :query {:source-table "card__1715"
         :breakout [[:field 488 {:base-type :type/Integer}]
                    [:expression "full name"]] ;; <-- expression
         :limit 5001}}
```

new style after this change:

```clojure
{:database 122
 :type :query
 :query {:source-table "card__1715"
         :breakout [[:field 488 {:base-type :type/Integer}]
                    [:field "full name" {:base-type :type/Text}]]
         :limit 5001}}
```

---------

## Normalize field references

The schema was expecting valid mbql field refs (aka vectors and
keywords) but was getting a list and string (`[:field 23 nil]`
vs ("field" 23 nil)`). So normalize the field ref so we can handle the
stuff over the wire

this nice little bit of normalization lived in models.interface and
comped two functions in mbql.normalize. A few places reused it so moved
it to the correct spot.

* Better error message in `fix-expression-refs`

handles [:field ...] and [:expression ...] clauses. Seems unlikely that
aggregations will flow through here as that's not a great way to label a
pk. But i'll add support for that in a follow up

* Move `normalize-field-ref` below definition of `normalize-tokens`

`normalize-tokens` is `declare`d earlier, but we aren't using the var as
a var, but in a def we derefence it. But that's nil as it hadn't been
defined yet. Caused lots of failures downstream, including in cljs land

```clojure
user=> (declare x)
user=> (def y x) ;; <- the use in a def gets its current value
user=> (def x 3) ;; <- and it's not reactive and backfilling
user=> y
```

* Don't capture `declare`d vars in a def

need late binding. when the function is called, those vars will have a
binding. But if we use a `def`, it grabs their current value which is an
unbound var.

---------

Co-authored-by: default avatardan sutton <dan@dpsutton.com>
Co-authored-by: default avatarEmmad Usmani <emmadusmani@berkeley.edu>
parent 53aeec80
No related branches found
No related tags found
No related merge requests found
Showing
with 564 additions and 102 deletions
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment