metabase / toucan

A classy high-level Clojure library for defining application models and retrieving them from a DB
Eclipse Public License 1.0
570 stars 49 forks source link

Model resolution registry #91

Closed dpsutton closed 2 years ago

dpsutton commented 2 years ago

Changes:

add a model registry from model symbol to set of namespaces that defines it. Use this registry to look up when model is used as a symbol, eg (db/select 'Foo :id 1).

Suggested breaking change

Before this change, defining multiple models with the same name and querying by model symbol would always resolve to the model defined in the "conventional" namespace as constructed with model/root-namespace. If there were multiple models defined in "unconventional" locations, it would never find either of them. The registry is checked after the inferred namespace to preserve this behavior. But it might be better to change this behavior. Rather than allowing multiple model definitions like this, perhaps we could check the inferred location and also the registered locations and ensure we do not have an ambiguous model reference.

There is probably a small chance of this happening but having models findable in non-standard locations might increase the odds of it a little bit.

ie, if you define a normal model User in the conventional location ((defmodel User :production_user_table)) and a test file defines (defmodel User :test_user_table), usages of 'User as a symbol would resolve to the production user table. It might be better to throw in this case, even if this is a remote scenario.

dpsutton commented 2 years ago

If we're willing to break a bit to error when there are multiple models and a query uses a model symbol,

(defn- resolve-model-from-symbol
  [symb]
  (letfn [(sym->model-var [ns' symb']
            (try
              (requiring-resolve (symbol (str ns') (str symb')))
              (catch java.io.FileNotFoundException _e nil)))]
    (let [inferred-ns         (model-symb->ns symb)
          registered-nss      (get @models/model-sym->namespace-sym symb)
          [model-var & ambig] (->> (conj registered-nss inferred-ns)
                                   (keep #(sym->model-var % symb)))]
      (cond
        (not model-var)
        (throw (ex-info (format "Could not find model for: %s" symb)
                        {:symbol                symb
                         :configured-namespace  inferred-ns
                         :registered-namespaces registered-nss}))
        (seq ambig)
        (throw (ex-info (format "Found multiple models for: %s" symb)
                        {:symbol symb
                         :configured-namespace inferred-ns
                         :registered-namespaces registered-nss
                         :vars (conj ambig model-var)}))
        :else
        (deref model-var)))))
camsaul commented 2 years ago

I think the expectation is that models are unique anyway so erroring if there are duplicates would be fine if you wanted to open a follow-on PR to do that... but I'll merge this as-is and cut a new release