Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Jun 24, 2022
    • dpsutton's avatar
      Join slack channels with slack-id (#23495) · f25b6115
      dpsutton authored
      * Join slack channels with slack-id
      
      Fixes https://github.com/metabase/metabase/issues/23229
      
      We upload images to a channel and then send messages to the desired
      channel referencing those images from the channel we uploaded. But the
      slack bot must be in the channel to upload images.
      
      We handle this in `slack/upload-image!` where we watch the error message
      and join the channel if we recognize that is our issue. This special
      upload channel is set in the admin section when setting up, typed in by
      a human.
      
      Slack now requires us to use the internal id of the channel when
      joining. IE we used to use "metabase_files" but now we need to use
      something like "C87LQNL0Y23". But we do not have this information and we
      don't want humans to have to look this up.
      
      SOLUTION:
      change our cache. We currently just get a list of channels and users
      ```
      ["#general" "@dan" ...]
      ```
      
      Change this to
      ```
      {:version 2,
       :channels [{:display-name "#random",
                   :name "random",
                   :id "CT2FNGZSRPL",
                   :type "channel"}
                  {:display-name "#general",
                   :name "general",
                   :id "C87LQNL0Y23",
                   :type "channel"}
                  {:display-name "@dan",
                   :type "user",
                   :name "dan",
                   :id "UR65C4ZJVIW"}
                  ...]}
      ```
      
      Now we have slack internal ids present. When we attempt to join the
      slack channel, look for this id and attempt to use that.
      
      This has some knock-on effects. The UI still lists the channels in a
      channel picker when sending pulses. The list sent over the wire still
      mimics the previous shape (a flat list) and the choice is still the
      human readable name.
      
      In the future we should switch over to using the stable ids rather than
      solely channel names. Channel names can be renamed.
      
      I didn't go down this route because of the files channel. It is set at
      setup before we have a channel list. We could do some kind of run time
      migration but it is difficult because it would change the type of
      `slack-files-channel` from string to :json to handle the more complex
      type. Or perhaps we could make another setting to hold the json form and
      set that when we can positively identify things.
      
      In either case, these changes were not required yet to fix our slack
      issue. We just upgrade the information we have about slack channels,
      downgrade it when it hits the wire so the UI needs no changes, and use
      the extra information in the one spot where we need it.
      
      The cache is populated at startup and every four hours after that. So we
      do not need to worry about the old cache shape. If the new code is
      running, its the new cache.
      
      * Send #channel and @user forms over wire
      
      We store `{"channel": "#slack-pulses"}` in the pulse_channel.details
      column so we should keep those types of values around.
      
      We use the bare portion ("slack-pulses") rather than with the hash on it
      so we seem to be mixing usernames and channels. But these sets are
      distinct and you cannot create a channel with the same name as a
      user. Also, channel names are lowercase while channel-ids are uppercase
      so those are also non-overlapping sets.
      
      * Put slack token so slack reports as configured
      
      * Errant tap>
      
      * Alignment and docstring fixes
      
      * Remove slack-cache version information
      
      remove the `:version 2` from the cache. We are always in charge of the
      cache, and we compute it on startup so there's little risk of other data
      shapes being present.
      Unverified
      f25b6115
    • Cal Herries's avatar
      Specify table for id column (#23525) · 2e34bd0d
      Cal Herries authored
      Unverified
      2e34bd0d
  2. Jun 23, 2022
    • adam-james's avatar
      application-colors have new keys that may be added via the getter (#23493) · 4bfd682f
      adam-james authored
      * application-colors have new keys that may be added via the getter
      
      Some colors previously had multiple usage contexts. For example, `accent1` was used both in charts and other parts of
      the summarize UI. Now, those notions are being separated, so `accent1` remains a valid key, and `summarize` is a new
      valid key. To make sure behaviour remains the same for existing whitelabel users who may have set these keys, when
      such a key exists, it is 'split' by the getter.
      
      For example, if the existing application-colors json contains `accent1`, the getter will add a new key `summarize`
      with the same value as `accent1`, but only if `accent1` already exists, otherwise it does nothing. This is also true
      for keys `brand`, which adds `accent0`, and `accent7`, which adds `filter`
      
      * Make application colors getter make change only once
      
      * Premium feature flag for test
      Unverified
      4bfd682f
    • Cal Herries's avatar
      Stop caching /api/geojson/:key requests (#23474) · 02f26e07
      Cal Herries authored
      * Stop caching /api/geojson/:key requests
      
      * Remove extra require
      Unverified
      02f26e07
    • Braden Shepherdson's avatar
      Foundation for v2 serialization and deserialization (#23204) · 2eb89b4d
      Braden Shepherdson authored
      This supports serialization of only Collections and Settings so far, but
      it demonstrates the design of the new serialization system.
      
      `metabase.models.serialization.base` defines the multimethods, which
      are to be implemented by all the exported models eventually.
      The actual serialization code that drives the larger process is in
      `metabase_enterprise.serialization.v2.extract` and `.merge`, since
      serialization is an enterprise feature.
      
      The design calls for two matching phases on each side:
      - Serialization is extract + store;
      - Deserialization is ingest + load.
      
      Extract and load deal with vanilla Clojure maps with a `serdes/meta` key
      giving common details; they deliberately know nothing about files.
      
      Store and ingest deal with the storage medium and the process of
      listing and reading a stored export.
      
      Laziness is retained: the `load` process ingests full details on demand,
      so only the metadata of the importing database needs to fit in memory.
      Unverified
      2eb89b4d
    • dpsutton's avatar
      Enterprise settings (#23441) · 9c4e7389
      dpsutton authored
      * Allow for disabling settings
      
      Disabled settings will return their default value (else nil if no
      default is set). This allows us to have enterprise override settings and
      use them from regular OSS code without classloaders, extra vars,
      remembering to check if the feature is enabled, etc.
      
      Motivating examples are the appearance settings. We allow
      `application-font` setting to change the font of the application. This
      is an enterprise feature, but anyone can post to
      `api/setting/application-font` and set a new value or startup as
      `MB_APPLICATION_FONT=comic-sans java -jar metabase.jar` and have the
      functionality.
      
      Same thing for application colors in static viz. The calling code just
      calls `(settings/application-colors)` and uses them but doesn't check if
      the enterprise settings are enabled. To do this correctly, you have to
      remember to implement the following onerous procedure:
      
      A whole namespace for a setting
      ```clojure
      (ns metabase-enterprise.embedding.utils
        (:require [metabase.models.setting :as setting :refer [defsetting]]
                  [metabase.public-settings :as public-settings]
                  [metabase.public-settings.premium-features :as premium-features]
                  [metabase.util.i18n :refer [deferred-tru]]))
      
      (defsetting notification-link-base-url
        (deferred-tru "By default \"Site Url\" is used in notification links, but can be overridden.")
        :visibility :internal
        :getter (fn []
                  (when (premium-features/hide-embed-branding?)
                    (or (setting/get-value-of-type :string :notification-link-base-url)
                        (public-settings/site-url)))))
      ```
      
      And then in the calling code you have to do the procedure to
      conditionally require it and put it behind a var that can handle it
      being nil:
      
      ```clojure
      ;; we want to load this at the top level so the Setting the namespace defines gets loaded
      (def ^:private site-url*
        (or (u/ignore-exceptions
              (classloader/require 'metabase-enterprise.embedding.utils)
              (resolve 'metabase-enterprise.embedding.utils/notification-link-base-url))
            (constantly nil)))
      
      ;; and then the usage
      (defn- site-url
        "Return the Notification Link Base URL if set by enterprise env var, or Site URL."
        []
        (or (site-url*) (public-settings/site-url)))
      ```
      
      Far nicer to just place the following into the regular public-settings
      namespace:
      
      ```clojure
      (defsetting notification-link-base-url
        (deferred-tru "By default \"Site Url\" is used in notification links, but can be overridden.")
        :visibility :internal
        :enabled?    premium-features/hide-embed-branding?)
      ```
      
      Then no need for a custom namespace to hold this setting, no need to
      have an extra var to point to the setting else a fallback value.
      
      Note that this feature is not required on every enterprise feature we
      have. We a namespace `metabase-enterprise.sso.integrations.sso-settings`
      that has 24 settings in it, all of which are enterprise features. But
      these features are used in our enterprise sso offerings and are directly
      referenced from the enterprise features. No need for the extra var to
      point to them and the flag checks happen in other parts.
      
      * Mark the UI/UX customization settings as requiring whitelabeling
      
      Mark the following settings as requiring
      premium-settings/enable-whitelabeling? (aka token check)
      
      - application-name
      - loading-message (override of "doing science")
      - show-metabot (override of showing our friendly metabot)
      - application-colors
      - application-font
      - application-logo-url
      - application-favicon-url
      
      Updates the helper functions for colors to use the setting rather than
      feeling entitled to use a lower level `setting/get-value-of-type`. We
      need the higher level api so it takes into account if its enabled or
      not.
      
      * Move notification-link-base-url into regular settings with enabled?
      
      * Cleanup ns
      Unverified
      9c4e7389
    • Mahatthana (Kelvin) Nomsawadi's avatar
      Hide password + user form fields when logged in via JWT and SAML (#23476) · 7f246b7e
      Mahatthana (Kelvin) Nomsawadi authored
      
      * Hide password + user form fields when logged in via JWT and SAML
      
      * ngoc - update the maybe-add-sso_soruce
      
      * Address review: removing deprecated Cypress functions
      
      Co-authored-by: default avatarNgoc Khuat <qn.khuat@gmail.com>
      Unverified
      7f246b7e
    • Ngoc Khuat's avatar
      Make schema for parameters and parameter_mappings on card more strict (#23456) · 22d77dbe
      Ngoc Khuat authored
      * make schema for parameters and parameter_mappings more strict
      
      * no duplicate id for parameterlist in mbql schema
      
      * reverse a change in mbql schema
      
      * allow blank name and slug for parameter
      
      * add sectionId
      
      * trying to use mbql.s/Parameter instead
      
      * reverse change to use mbql.s/Parameter, there are too many failing tests
      Unverified
      22d77dbe
  3. Jun 22, 2022
  4. Jun 21, 2022
  5. Jun 20, 2022
    • metamben's avatar
      Fix compilation of temporal arithmetic in between filters (#23292) · 001055df
      metamben authored
      Fix compilation of temporal arithmetic for BigQuery and Mongo 5+
      
      * Mongo 4 doesn't support $dateAdd so the generated filters result in an exception.
      * Support adding field to interval too (time intervals were not allowed in the first place of an addition)
      * Support temporal arithmetic with more than two operands for Mongo
      Unverified
      001055df
  6. Jun 18, 2022
  7. Jun 17, 2022
  8. Jun 16, 2022
    • Cal Herries's avatar
      Remove extra whitespace in defendpoint (#23350) · 90cb13cb
      Cal Herries authored
      
      * Remove extra whitespace in defendpoint
      
      * [ci nocache]
      
      Co-authored-by: default avatarLuis Paolini <paoliniluis@gmail.com>
      Unverified
      90cb13cb
    • dpsutton's avatar
      Handle quarters in native queries (#23368) · 3530241c
      dpsutton authored
      MBQL goes through a different path. This is only for native
      queries. Also the diff is huge. Ignoring whitespace shows a very modest
      diff:
      
      ```diff
          "month"   {:unit-range month-range
                     :to-period  t/months}
      +   "quarter" {:unit-range relative-quarter-range
      +              :to-period  (comp t/months (partial * 3))}
          "year"    {:unit-range year-range
                     :to-period  t/years}})
      
      ...
      
         "past2months~"   {:end "2016-06-30", :start "2016-04-01"}
         "past13months"   {:end "2016-05-31", :start "2015-05-01"}
      +  "past2quarters"  {:end "2016-03-31", :start "2015-10-01"}
      +  "past2quarters~" {:end "2016-06-30", :start "2015-10-01"}
         "past1years"     {:end "2015-12-31", :start "2015-01-01"}
         "past1years~"    {:end "2016-12-31", :start "2015-01-01"}
      ```
      
      Helpful Testing Strategies
      --------------------------
      
      Sample DB
      =========
      
      `select id, created_at from orders where {{created_at}}`
      and set a field filter of type "Date Filter"
      
      Custom Table
      ============
      
      Create a table that has entries in the middle of each quarter.
      
      ```sql
      esting=# create table quarters (id serial primary key not null, description text, t timestamptz);
      CREATE TABLE
      testing=# insert into quarters (description, t) values ('Q1 2022', '2022-02-01'), ('Q2 2022', '2022-05-01'), ('Q3 2022', '2022-08-01'), ('Q4 2022', '2022-11-01');
      INSERT 0 4
      testing=# select * from quarters;
       id | description |           t
      ----+-------------+------------------------
        1 | Q1 2022     | 2022-02-01 00:00:00-06
        2 | Q2 2022     | 2022-05-01 00:00:00-05
        3 | Q3 2022     | 2022-08-01 00:00:00-05
        4 | Q4 2022     | 2022-11-01 00:00:00-05
      (4 rows)
      ```
      
      Before this change
      ------------------
      
      > Cannot invoke "clojure.lang.IFn.invoke(Object)" because "to_period" is null
      
      (note, if you cannot reproduce this its because you haven't gotten
      https://github.com/metabase/metabase/pull/23346 in your local version
      which ensured errors always appear as errors on the frontend. java 11
      has no message for NPE so the Frontend missed it was an error and
      displayed no results as if the query had succeeded)
      
      This was the case for the following scenarios:
      - "last1quarters"
      - "last1quarters~"
      - "thisquarter"
      - "next1quarters"
      - "next1quarters~"
      
      where the ~ means to include the current quarter.
      
      After this change
      -----------------
      
      Running the queries against the custom table I made (current time is Jun
      15, Q2)
      
      - "last1quarters": "Q1 2022" "February 1, 2022, 6:00 AM"
      - "last1quarters~": "Q1 2022" "February 1, 2022, 6:00 AM" | "Q2 2022" "May 1, 2022, 5:00 AM"
      - "thisquarter": "Q2 2022" "May 1, 2022, 5:00 AM"
      - "next1quarters" "Q3 2022" "August 1, 2022, 5:00 AM"
      - "next1quarters~": "Q2 2022" "May 1, 2022, 5:00 AM" | "Q3 2022" "August 1, 2022, 5:00 AM"
      
      And of course added tests into the matrix for the date parsing.
      Unverified
      3530241c
    • jkeys089's avatar
      improved support for Google Cloud SQL (#19302) · e8368d1d
      jkeys089 authored
      
      * improved support for Google Cloud SQL
      
      * upgrade `postgres-socket-factory` and add license overrides
      
      * fix license check
      
      Co-authored-by: default avatarCam Saul <1455846+camsaul@users.noreply.github.com>
      Unverified
      e8368d1d
    • Howon Lee's avatar
      JSON query in postgres now walks identifier tree if one encountered (fixes #22967) (#23278) · 9e8cdef2
      Howon Lee authored
      Pursuant to #22967.
      
      Previously the notion of the identifier in json-query in postgres assumed a singular identifier at the top-level of the Identifier record. This is not always the case in complicated binning scenarios - the identifier could also just comprise an entire tree of Identifier record maps with the actual Identifier we're looking to turn into the JSON query in some leaf somewhere.
      
      Therefore, the json-query's determination of what the identifier is now walks that identifier record tree and replaces the identifiers that have JSON field semantics with the reification of ToSql from json-query.
      
      Previous attempt at solution hacked together something at the json-query level, changing the reification, but that only worked for one level down, instead of just walking the Identifier tree and therefore getting stuff at arbitrary tree depth. And we do get nontrivial tree depth in the bucketing in #22967.
      Unverified
      9e8cdef2
  9. Jun 15, 2022
    • dpsutton's avatar
      Ensure :error is present for frontend (#23346) · 79965714
      dpsutton authored
      * Ensure :error is present for frontend
      
      The frontend checks for a string at "error" to determine if there's an
      error or not. Any error that lacks a message (`(.getMessage e) = nil`)
      will report as a successful query with no results. This was hard to
      reproduce at first because java 14+ added a nice error message to NPEs
      in [JEP-358](https://openjdk.org/jeps/358).
      
      ```java
      ;; java 11
      jshell> HashSet x = null;
      x ==> null
      
      jshell> x.contains(3);
      |  Exception java.lang.NullPointerException
      |        at (#2:1)
      
      jshell>
      
      ;; java 17
      jshell> HashSet x = null;
      x ==> null
      
      jshell> x.contains(3);
      |  Exception java.lang.NullPointerException: Cannot invoke "java.util.HashSet.contains(Object)" because "REPL.$JShell$11.x" is null
      |        at (#4:1)
      ```
      
      ```clojure
      ;; java 17
      catch-exceptions=> (let [x nil] (x 3))
      Execution error (NullPointerException) at metabase.query-processor.middleware.catch-exceptions/eval130385 (REPL:41).
      Cannot invoke "clojure.lang.IFn.invoke(Object)" because "x" is null
      
      ;; java 11
      
      catch-exceptions=> (let [x nil] (x 3))
      Execution error (NullPointerException) at metabase.driver.sql.parameters.substitution/eval118508 (REPL:17).
      null
      ```
      
      Here are the responses to the FE edited to the relevant bits:
      
      ```javascript
      // objects are edited for clarity/brevity:
      // from java-11
      {
        "error_type": "qp",
        "status": "failed",
        "class": "class java.lang.NullPointerException",
        "stacktrace": [...],
        "error": null,   // <---- the FE ignores all of the other data and only sees this
      }
      
      // vs java-17
      {
        "error_type": "qp",
        "status": "failed",
        "class": "class java.lang.NullPointerException",
        "stacktrace": [...],
        "error": "Cannot invoke \"clojure.lang.IFn.invoke(Object)\" because \"to_period\" is null",,
      }
      ```
      
      Also, note that we were still logging
      
      > ERROR middleware.catch-exceptions :: Error processing query: null
      
      because we were looking for `(:error format-exception)` instead of
      `(:error formatted-exception)`. `format-exception` is a multimethod and
      lookup on non-associative things will happily return nil.
      
      * Tests
      Unverified
      79965714
  10. Jun 14, 2022
  11. Jun 13, 2022
    • adam-james's avatar
      Allow 'null' First and Last names for Users (#23154) · 14312b9e
      adam-james authored
      
      * Migration to allow 'null' first and last names for users
      
      This is part of improving how we handle user names, related to SSO, but we also have an opportunity to make login via
      email simpler as well -> just loosen the requirement for names.
      
      * When only last name is given, :common_name should not have a space
      
      * WIP making optional first/last names safe in the backend.
      
      Trying to fix up some assumptions about first/last name keys in backend implementations.
      
      * Only add `:common_name` key when non-nil
      
      * Adjust setup tests to pass if use first/last name are nil
      
      * Fix up setup tests to work with null first/last names.
      
      * User API endpoints altered to handle nil names
      
      * Remove name validation test since they can be nil now
      
      * Alter JWT/SAML tests for nil user names
      
      * Remove "unknown" default in SAML
      
      * Add tests to make sure users are updating appropriately
      
      * use good naming convention in local function
      
      Co-authored-by: default avatarCam Saul <1455846+camsaul@users.noreply.github.com>
      
      * Simplify truthy check in test
      
      Co-authored-by: default avatarCam Saul <1455846+camsaul@users.noreply.github.com>
      
      * Simplify truthy check in test
      
      Co-authored-by: default avatarCam Saul <1455846+camsaul@users.noreply.github.com>
      
      * Indentation fix
      
      * Fix up syntax issue from github merge.
      
      * Fix missed function name changes in test
      
      * Clean up how sets are built up in the PUT user/:id endpoint
      
      * Better implementation for update-user-name
      
      * Avoid relying on 'vals' since order of keys is not guaranteed
      
      * Fixing little things, clarification in a comment
      
      * I'm tired... missed an obvious thing here. oops
      
      * indentation and clarify some testing
      
      * Change post-select implementation slightly
      
      * expected status is now an optional arg no dynamic var binding needed
      
      * 'f' is not optional, so we take it with first, then args are rest
      
      * simplify destructuring
      
      Co-authored-by: default avatarCam Saul <1455846+camsaul@users.noreply.github.com>
      Unverified
      14312b9e
    • Jeff Bruemmer's avatar
      docs - frontmatter (#23272) · b337f510
      Jeff Bruemmer authored
      Unverified
      b337f510
    • Braden Shepherdson's avatar
      Define identity-hash for fairly robust de-duplication when deserializing (#23145) · d675a480
      Braden Shepherdson authored
      Define identity-hash for fairly robust de-duplication when deserializing
      
      This is a fallback for fully robust de-duplication based on `entity_id`
      fields.
      
      All serialized models should support identity-hash, and there is a test
      to enforce that.
      Unverified
      d675a480
    • Ngoc Khuat's avatar
      Add API to fetch and search card parameters values (#23102) · fa8d88a3
      Ngoc Khuat authored
      * add API to fetch and search card parameters
      
      * remove code to support linked filters
      
      * remove unecessary data in tests
      
      * fix from Noah's comments
      
      * some chain-filter -> param-values
      Unverified
      fa8d88a3
  12. Jun 11, 2022
  13. Jun 10, 2022
    • Noah Moss's avatar
      Allow multi-line strings using `str` in translation macros (#22901) · 92175e0b
      Noah Moss authored
      
      * allow str form as the first argument to deferred-tru
      
      * add tests and update doc strings
      
      * migrate more setting descriptions
      
      * Handle multiline translation sources
      
      ```clojure
      enumerate=> x
      (deferred-tru
       (str
        "Whether an introductory modal should be shown after the next database connection is added. "
        "Defaults to false if any non-default database has already finished syncing for this instance."))
      enumerate=> (form->string-for-translation x)
      "Whether an introductory modal should be shown after the next database connection is added. Defaults to false if any non-default database has already finished syncing for this instance."
      ```
      
      We get the form from `(g/grasp <file> ::translate)` which returns the
      `x`. And then simply pick through it for either a string literal or a
      call to `(str <literal>+)`
      
      Co-authored-by: default avatardan sutton <dan@dpsutton.com>
      Unverified
      92175e0b
    • Howon Lee's avatar
      Use big types when we have big JSON contents (whacks #22732) (#23221) · 4eb558a8
      Howon Lee authored
      Previously we defaulted to integer and float type in all cases for json unfolding type determination, even with bigints and bigdecimals. Now if we encounter a bignum in json unfolding we call it a bignum.
      Unverified
      4eb558a8
  14. Jun 09, 2022
    • dpsutton's avatar
      Fix mysql humanize error (#23263) · 051534fe
      dpsutton authored
      * Correctly handle fall-through case.
      
      Condp is exhaustive and throws if nothing matches
      
      ```
      mysql=> (condp = 2
                1 :nope
                3 :nope)
      Execution error (IllegalArgumentException) at metabase.driver.mysql/eval169387 (REPL:870).
      No matching clause: 2
      mysql=>
      ```
      
      and can take a single default clause at the end with no predicate
      
      ```clojure
      mysql=> (condp = 2
                1 :nope
                :default-catch-all)
      :default-catch-all
      ```
      
      This was attempted with a regex `#".*"` but this regex does not match
      everything!
      
      ```clojure
      mysql=> (driver/humanize-connection-error-message :mysql
                                                        "hello\nthere")
      Execution error (IllegalArgumentException) at metabase.driver.mysql/eval167431$fn (mysql.clj:124).
      No matching clause: hello
      there
      mysql=> (re-matches #".*" "hi\nthere")
      nil
      ```
      
      So just use use the message as the default to be returned which was the
      original intention.
      
      * Fix other databases as well
      Unverified
      051534fe
    • Noah Moss's avatar
      Force slack channel cache refresh when new token is saved (#23253) · 68951307
      Noah Moss authored
      * clear Slack cache last-updated value when resetting cache
      
      * use clear-channel-cache! function on invalid tokens as well
      Unverified
      68951307
  15. Jun 08, 2022
    • Case Nelson's avatar
      Hydrate :persisted on Card PUT (#23239) · 9f41e1d2
      Case Nelson authored
      We want to make sure that the :persisted hydration is returned when
      updating a card's description.
      Unverified
      9f41e1d2
    • dpsutton's avatar
      Save query executions for cached queries (#23228) · 6571d223
      dpsutton authored
      * Save query executions for cached queries
      
      Previously had addressed
      https://github.com/metabase/metabase/pull/16720/ because we were
      including cache hit query executions in the running query execution
      time. IE, queries take 60 seconds to run but the cache hit takes
      100ms. Our average thus would be (60s + 100ms)/2 ~ 30 seconds and the
      query duration falls below the cache threshold so we abandon the cache
      until the average creeps up back up again. Original issue:
      https://github.com/metabase/metabase/issues/15432
      
      So the first fix was a poor one and it skipped recording the query
      execution at all which was way too heavy handed.
      
      ```clojure
      (when-not (:cached acc)
        (save-successful-query-execution! (:cached acc) execution-info @row-count))
      ```
      
      This approach takes the more considered approach: skip updating average
      execution time but record the query-execution.
      
      ```clojure
      ;; conditionally save average execution time
      (when-not (:cache_hit query-execution)
          (query/save-query-and-update-average-execution-time! query query-hash running-time))
      
      ;; unconditionally (modulo having a context) save the query execution
      (if-not context
        (log/warn (trs "Cannot save QueryExecution, missing :context"))
        (db/insert! QueryExecution (dissoc query-execution :json_query)))
      ```
      
      One question
      ------------
      
      I'm unsure about one change here. What are the implications of not
      updating this execution time. There are two operations involved:
      ensuring there is a `Query` (table query) entry for a query and updating
      its average execution time. I'm assuming that since a query is cached it
      has a query table entry. But this code creates the Query if it doesn't
      exist and then updates its average execution time. It's not clear to me
      if we do in fact want to ensure a Query entry exists. But it feels
      harmless both in the case it is missing and also that it most likely
      isn't missing since we have a cached query and when it was cached this
      entry would have been created or updated. But adding a note just in case.
      
      * Fix test
      
      distinguish between the calls to save query execution and the calls to
      update average execution duration. The former should be called
      twice (for the uncached query and the cached query run) while the latter
      should only be called on the first run (aka, uncached results affect
      query duration stats, cached runs do not affect it)
      
      * Checksums are no more
      
      metadata became editable for models and the checksum really didn't add any value, just asserted that it had not been
      changed.
      Unverified
      6571d223
    • dpsutton's avatar
      Create po template file (pot) from clojure (#23181) · ab2c5af3
      dpsutton authored
      * Create po template file (pot) from clojure
      
      Rather than use xgettext we can use grasp to look for translation
      sites. The reason to do this is twofold:
      
      Multiline Translated Strings
      ----------------------------
      
      We can use multiline strings in source to aide in readability. This
      [PR](https://github.com/metabase/metabase/pull/22901)
      was abandoned because we xgettext cannot be expected to combine string
      literals into a single string for translation purposes. But we can
      certainly do that with clojure code.
      
      ```clojure
      (defn- form->string-for-translation
        "Function that turns a form into the translation string. At the moment
        it is just the second arg of the form. Afterwards it will need to
        concat string literals in a `(str \"foo\" \"bar\")` situation. "
        [form]
        (second form))
      
      (defn- analyze-translations
        [roots]
        (map (fn [result]
               (let [{:keys [line _col uri]} (meta result)]
                 {:file (strip-roots uri)
                  :line line
                  :message (form->string-for-translation result)}))
             (g/grasp roots ::translate)))
      ```
      
      `form` is the literal form. So we can easily grab all of the string
      literals out of it and join them here in our script. The seam is already
      written. Then reviving the PR linked earlier would upgrade the macros to
      understand that string literals OR `(str <literal>+)` are acceptable
      clauses.
      
      Translation context
      -------------------
      
      Allowing for context in our strings. The po format allows for context in
      the file format.
      
      ```
      msgctxt "The update is about changing a record, not a timestamp"
      msgid "Failed to notify {0} Database {1} updated"
      msgstr ""
      ```
      
      See [this
      issue](https://github.com/metabase/metabase/issues/22871#issuecomment-1146947441)
      for an example situation. This wouldn't help in this particular instance
      because it is on the Frontend though.
      
      But we could have a format like
      ```clojure
      (trs "We" (comment "This is an abbreviation for Wednesday, not the
      possessive 'We'"))
      ```
      The macro strips out the `comment` form and we can use it when building
      our pot file.
      
      Note there is a difficulty with this though since all source strings
      must be unique. There can be multiple locations for each translated
      string.
      
      ```
      ,#: /metabase/models/field_values.clj:89
      ,#: /metabase/models/params/chain_filter.clj:588
      msgid "Field {0} does not exist."
      msgstr ""
      ```
      The leading commas are present to prevent commit message comments. But
      if one location has a context and the other doesn't, or even worse, if
      they have two different contexts, we have a quandry: we can only express
      one context. Probably easy to solve or warn on, but a consideration.
      
      Caught Errors
      -------------
      
      The script blew up on the following form:
      
      ```clojure
      (-> "Cycle detected resolving dependent visible-if properties for driver {0}: {1}"
          (trs driver cyclic-props))
      ```
      
      No tooling could (easily) handle this properly. Our macro assertions
      don't see the thread. But xgettext never found this translation
      literal. I warn in the pot generation so we can fix this. We could also
      have a test running in CI checking that all translations are strings and
      not symbols.
      
      Fundamental Tool
      ----------------
      
      The sky is the limit because of this fundamental grasp tool:
      
      ```clojure
      enumerate=> (first (g/grasp single-file ::translate))
      (trs "Failed to notify {0} Database {1} updated" driver id)
      enumerate=> (meta *1)
      {:line 35,
       :column 22,
       :uri "file:/Users/dan/projects/work/metabase/src/metabase/driver.clj"}
      ```
      
      We can find all usages of tru/trs and friends and get their entire form
      and location. We can easily do whatever we want after that.
      
      Verifying Translation scripts still work
      ----------------------------------------
      
      You can check a single file is valid with `msgcat <input.pot> -o
      combined.pot`. This will throw if the file is invalid.
      
      The general script still works:
      
      ```
      ❯ ./update-translation-template
      [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs.pprint.js as it exceeds the max of 500KB.
      [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs.core.js as it exceeds the max of 500KB.
      [BABEL] Note: The code generator has deoptimised the styling of /Users/dan/projects/work/metabase/frontend/src/cljs/cljs-runtime/cljs.core.js as it exceeds the max of 500KB.
      Warning: environ value /Users/dan/.sdkman/candidates/java/current for key :java-home has been overwritten with /Users/dan/.sdkman/candidates/java/17.0.1-zulu/zulu-17.jdk/Contents/Home
      SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
      SLF4J: Defaulting to no-operation (NOP) logger implementation
      SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
      Created pot file at  ../../locales/metabase-backend.pot  #<----- new line here
      Warning: environ value /Users/dan/.sdkman/candidates/java/current for key :java-home has been overwritten with /Users/dan/.sdkman/candidates/java/17.0.1-zulu/zulu-17.jdk/Contents/Home
      2022-06-06 15:05:57,626 INFO metabase.util :: Maximum memory available to JVM: 8.0 GB
      Warning: protocol #'java-time.core/Amount is overwriting function abs
      WARNING: abs already refers to: #'clojure.core/abs in namespace: java-time.core, being replaced by: #'java-time.core/abs
      WARNING: abs already refers to: #'clojure.core/abs in namespace: java-time, being replaced by: #'java-time/abs
      2022-06-06 15:06:01,368 WARN db.env :: WARNING: Using Metabase with an H2 application database is not recommended for production deployments. For production deployments, we highly recommend using Postgres, MySQL, or MariaDB instead. If you decide to continue to use H2, please be sure to back up the database file regularly. For more information, see https://metabase.com/docs/latest/operations-guide/migrating-from-h2.html
      2022-06-06 15:06:03,594 INFO util.encryption :: Saved credentials encryption is DISABLED for this Metabase instance. :unlock:
       For more information, see https://metabase.com/docs/latest/operations-guide/encrypting-database-details-at-rest.html
      WARNING: abs already refers to: #'clojure.core/abs in namespace: taoensso.encore, being replaced by: #'taoensso.encore/abs
      WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.math, being replaced by: #'kixi.stats.math/abs
      WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.test, being replaced by: #'kixi.stats.math/abs
      WARNING: abs already refers to: #'clojure.core/abs in namespace: kixi.stats.distribution, being replaced by: #'kixi.stats.math/abs
      msgcat: msgid '{0} metric' is used without plural and with plural.
      msgcat: msgid '{0} table' is used without plural and with plural.
      ```
      
      I'm not sure what the last two lines are about but I suspect they are
      preexisting conditions. their form from the final combined pot
      file (althrough again with leading commas on the filenames to prevent
      them from being omitted from the commit message)
      
      ```
      ,#: frontend/src/metabase/admin/permissions/components/PermissionsConfirm.jsx:32
      ,#: src/metabase/automagic_dashboards/core.clj
      ,#: target/classes/metabase/automagic_dashboards/core.clj
      ,#, fuzzy, javascript-format
      msgid "{0} table"
      msgid_plural "{0} tables"
      msgstr[0] ""
      "#-#-#-#-#  metabase-frontend.pot  #-#-#-#-#\n"
      "#-#-#-#-#  metabase-backend.pot (metabase)  #-#-#-#-#\n"
      msgstr[1] "#-#-#-#-#  metabase-frontend.pot  #-#-#-#-#\n"
      
      ...
      
      ,#: frontend/src/metabase/query_builder/components/view/QuestionDescription.jsx:24
      ,#: src/metabase/automagic_dashboards/core.clj
      ,#: target/classes/metabase/automagic_dashboards/core.clj
      ,#, fuzzy, javascript-format
      msgid "{0} metric"
      msgid_plural "{0} metrics"
      msgstr[0] ""
      "#-#-#-#-#  metabase-frontend.pot  #-#-#-#-#\n"
      "#-#-#-#-#  metabase-backend.pot (metabase)  #-#-#-#-#\n"
      msgstr[1] "#-#-#-#-#  metabase-frontend.pot  #-#-#-#-#\n"
      ```
      
      * Add drivers, one override, remove unused import
      
      import wasn't necessary
      forgot to check the driver sources for i18n
      for some reason grasp doesn't descend into
      
      ```clojure
      (defmacro ^:private deffingerprinter
        [field-type transducer]
        {:pre [(keyword? field-type)]}
        (let [field-type [field-type :Semantic/* :Relation/*]]
          `(defmethod fingerprinter ~field-type
             [field#]
             (with-error-handling
               (with-global-fingerprinter
                 (redux/post-complete
                  ~transducer
                  (fn [fingerprint#]
                    {:type {~(first field-type) fingerprint#}})))
               (trs "Error generating fingerprint for {0}" (sync-util/name-for-logging field#))))))
      ```
      
      I've opened an issue on [grasp](https://github.com/borkdude/grasp/issues/28)
      
      * Use vars rather than name based matching
      Unverified
      ab2c5af3
    • Ngoc Khuat's avatar
      add :updated-at-timestamped? property and use it on some models (#23220) · 0b573e50
      Ngoc Khuat authored
      * add :updated-at-timestamped? property and use it on some models
      Unverified
      0b573e50
    • dpsutton's avatar
      Handle time types in render code (#23198) · bc1e7b54
      dpsutton authored
      Unverified
      bc1e7b54
    • metamben's avatar
      Support SSL with client auth for Mongo (#22977) · cef4c19b
      metamben authored
      We have already had support for server authentication based on custom
      certificates. This change adds support for authenticating the client
      based on custom client key and certificate.
      Unverified
      cef4c19b
  16. Jun 07, 2022
    • Case Nelson's avatar
      Include :features whether or not db is initialized (#23128) · f0a1406e
      Case Nelson authored
      * Include :features whether or not db is initialized
      
      On restart, drivers may not be initialized but admins still need to know
      what features are available. Always include features on db post-select.
      
      * Driver can be null when not selected
      
      * Remove intitialized check for :details as well
      
      Both driver.u/features and driver/normalize-db-details eventually call
      `dispatch-on-initialized-driver` which will call
      `initialize-driver-if-needed`, removing the need for the check.
      
      * Unregistered drivers can make it in here, but they can be abstract so driver/available is not apropriate
      Unverified
      f0a1406e
    • Howon Lee's avatar
      Do trivial identity for passthrough of symbols for nested field column instead... · ea4825a3
      Howon Lee authored
      Do trivial identity for passthrough of symbols for nested field column instead of default processing (#23136)
      
      Should whack both #23026 and #23027.
      
      Previous (default) behavior of the reducible query that gets the limited contents of the JSON to break out the nested field columns is to lowercase identifiers. This is root cause of #23026 and #23027.
      
      But we want the proper case of those identifiers in order to be modifying the query correctly when we query the JSON. So we set the reducible query to just pass through the identifiers instead of default behavior.
      Unverified
      ea4825a3
    • Luiz Arakaki's avatar
  17. Jun 06, 2022
Loading