This project is mirrored from https://github.com/metabase/metabase.
Pull mirroring updated .
- Jun 08, 2022
-
-
Luiz Arakaki authored
-
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.
-
Ryan Laurie authored
-
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.
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 -
Ryan Laurie authored
* update modal tab names
-
Ngoc Khuat authored
* add :updated-at-timestamped? property and use it on some models
-
dpsutton authored
-
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.
-
Nemanja Glumac authored
-
Nemanja Glumac authored
-
- Jun 07, 2022
-
-
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
-
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.
-
Alexander Lesnenko authored
-
Ryan Laurie authored
-
Nemanja Glumac authored
-
dpsutton authored
-
Luiz Arakaki authored
-
Alexander Polyankin authored
-
Diogo Mendes authored
* Adding conditional to GHA E2E Test * Adding missing ` * Running if as suggested * Update matrix * Correcting logic * Removing typo * Removing duplicated run * Updating to the new launch name * Update with master
-
Alexander Polyankin authored
-
Alexander Polyankin authored
-
Ikko Ashimine authored
unqiue -> unique
-
Nemanja Glumac authored
-
Nemanja Glumac authored
-
Benoit Vinay authored
* Mode moved into its own package * Misc code clena up * Comment added back in * modes set to TS * getMode method WIP * getMode clean up * ObjectLiteral type added * Fix types for Mode * ObjectLiteral replaced where necessary * FilterPopover commit issue Please enter the commit message for your changes. Lines starting * ObjectLiteral replaced with TS Record * ObjectLiteral removed
-
- Jun 06, 2022
-
-
Ariya Hidayat authored
-
Nemanja Glumac authored
-
Nemanja Glumac authored
-
metamben authored
Fixes #22626.
-
Natalie authored
-
Noah Moss authored
-
Dalton authored
* Move date formatting code to utils file * Add parameter formatting util * Replace component format fn usage * Add exception for parameters mapped to sql variables * Remove now unused format functions
-
Nemanja Glumac authored
-
Alexander Polyankin authored
-
dpsutton authored
CVE info: Package: com.google.oauth-client:google-oauth-client Installed Version: 1.31.5 Vulnerability CVE-2021-22573 Severity: HIGH Fixed Version: 1.33.3 ``` . metabase/bigquery-cloud-sdk /Users/dan/projects/work/metabase/modules/drivers/bigquery-cloud-sdk . com.google.cloud/google-cloud-bigquery 1.135.4 . [truncated] . com.google.oauth-client/google-oauth-client 1.31.5 . metabase/googleanalytics /Users/dan/projects/work/metabase/modules/drivers/googleanalytics . com.google.apis/google-api-services-analytics v3-rev20190807-1.32.1 . com.google.api-client/google-api-client 1.32.1 . com.google.oauth-client/google-oauth-client 1.31.5 ``` I looked into bumping com.google.apis/google-api-services-analytics-v3-rev20190807-1.32.1 but as far as I can tell from https://search.maven.org/artifact/com.google.apis/google-api-services-analytics this is the most recent version so we have to just target the transitive dep. For bigquery, it seems we are pretty far behind. 1.135.4 was released in July 2021, the current version is 2.13.1 released in June. https://mvnrepository.com/artifact/com.google.cloud/google-cloud-bigquery I'm hesitant to bump this for a CVE but we need to prioritize this upgrade. After this PR: ``` clj -Stree -A:drivers . metabase/bigquery-cloud-sdk /Users/dan/projects/work/metabase/modules/drivers/bigquery-cloud-sdk . com.google.cloud/google-cloud-bigquery 1.135.4 . [truncated] X com.google.oauth-client/google-oauth-client 1.31.5 :older-version . com.google.oauth-client/google-oauth-client 1.33.3 . metabase/googleanalytics /Users/dan/projects/work/metabase/modules/drivers/googleanalytics . com.google.apis/google-api-services-analytics v3-rev20190807-1.32.1 . com.google.api-client/google-api-client 1.32.1 X com.google.oauth-client/google-oauth-client 1.31.5 :older-version ``` With the `X` meaning not included and 1.33.3 being top level included so using that version.
-
Gustavo Saiani authored
-
Case Nelson authored
* Handle deletable state when enabling persistence When enabling persistence all models are supposed to be set to creating unless they have been explicitly turned "off". Before, we were only looking for new models without attached PersistedInfos when enabling, but missed deletable PersistedInfos that needed to be re-enabled. It's the correct thing to do during the refresh as you wouldn't want refresh to re-enable but it's incorrect when enabling. * Remove unecessary thread-last
-
Nemanja Glumac authored
-
- Jun 03, 2022
-
-
Ryan Laurie authored
* add generic range slider input
-
Ryan Laurie authored
* add boolean filtering inline
-