Select Git revision
Search by author
- Any Author
- authors
-
Arti Sandeep Dhareshwar adharesh
-
Benjamin Aaron Fleming
bflemin2
-
Brett Edward Caplan bcaplan2
-
Cedric William Prentice
cprentic
-
Chinhnam Dinh Nguyen
cn2149
-
Clara Madeleine Rigaux crigaux
-
Dale Morey dale
-
Daniel Wysling wysling
-
David Eisner
deisner
-
Daya Sagar dsagar
-
Devon Andrew Lai dlai12
-
Emily Li emilyli5
-
Eric Charles McKinney emckinne
-
Erica Lee Honadel ehonade1
-
Jason Alexander Steinberg
jsteinb4
-
Jim Zahniser zahniser
-
Karthik Valliappan Thiagarajan
kvthiaga
-
Kendrick Maurizio Montecino
kmtecino
-
Lynne Homann Cure lhcure
-
Megan Lok-Yi Nee mnee
This project is mirrored from https://github.com/metabase/metabase.
Pull mirroring updated .
- Aug 05, 2022
-
-
dpsutton authored
* Fix in-memory logger Our Admin > Troubleshooting > Logs page broke, just showing a spinner and never showing the logs. Don't quite understand why this fixes it. Javadocs are https://logging.apache.org/log4j/2.x/log4j-api/apidocs/org/apache/logging/log4j/LogManager.html#getContext-boolean- ```clojure logger=> (log/warn "test") nil logger=> (count @messages*) 0 ;; no in-memory logs so page is empty ;; change `(LogManager/getContext true)` in the momoized ns-logger fn ;; and then observe: logger=> (log/warn "test") nil logger=> (count @messages*) 4 ``` Some explorations that might shine some light: ```clojure logger=> (into {} (.getAppenders (.getLogger (LogManager/getContext false) (str *ns*)))) {} logger=> (into {} (.getAppenders (.getLogger (LogManager/getContext true) (str *ns*)))) {"metabase-appender" #object[metabase.logger.proxy$org.apache.logging.log4j.core.appender.AbstractAppender$ff19274a "0x4d680247" "metabase-appender"]} ``` So something is not hooked up quite right. * Add tests for metabase.logger * Ensure `api/util/logs` returns logs * tests * Check for presence of `MetabaseLoggerFactory` rather than whole str When in a namespace with a record, `Foo resolves to ns.Foo. But outside it resolves to ns/Foo. When running tests from the command line *ns* is user so it gets more complicated. * Kinda playing whackamole™ with `(LogManager/getContext true)` * Remove custom memoizing logger History: 39.2 we set `Multi-Release: true` in our manifest file and query speed drops like a stone. Jeff was able to track this to our logger calls in tight loops. We revert the multi-release and keep seeing the warning on startup > WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance. Benchmarking on 39.2 (bench (log/trace "hi")) -> 15383 ns So we freaked out and set up a memoizing logger factory (bench (log/trace "hi")) -> 141 ns What a win. But we never noticed that the default *logger-factory* being picked up was slf4j ( `(log.impl/name log/*logger-factory*)` -> "org.slf4j" ). On 39.2 if you set the factory to the log4j2 version you get back to a great number: `(bench (log/trace "hi"))` -> 25 ns And thus ensuring that our logger is the default log4j2 version is even faster than our memoizing logger-factory. Memoizing factory: 141 ns slf4j factory: 2269 ns log4j2 factory: 31 ns What does `(LogManager/getContext false)` mean versus using `true`? We only need and want a single context. But log4j2 by default uses a context selector called `ClassLoaderContextSelector`. We could put all of this behind us if we used a context selector type `BasicContextSelector` but this is surprisingly hard to do: you have to set a system property. And since all of this stuff gets initialized in static initializers, we don't have an opportunity to do this programmatically. The way around this is to require people to pass this system property on startup which is not acceptable. So getContext true checks for a threadlocal context in a specific static variable and falls back to a Default context. getContext false looks at classloaders and ends up at a different context. BUT: the log.impl version uses a closure over getContext false instead of getting it each time. And I suspect that when it does this there is only one so it is the default and continues to use this one. In our LoggerFactory implementation we were looking up the context each time. This still seems to work and everything is playing nice in our application classloader but its totally possible that our drivers are not hitting this. I'll have to investigate this. Verification: - build the uberjar locally (`bin/build`) - copy to some temp directory and also copy criterium.jar ```shell MB_JETTY_PORT=4000 java "$(socket-repl 4001)" -cp locally-built.jar:criterium.jar metabase.core ``` ```clojure /tmp/locally-built via
-
- Aug 04, 2022
- Aug 03, 2022
-
-
Howon Lee authored
* rewrite here... * no bq * thinking * conjunction of alias thing... * make sure we dont wanna see it * nit * multi-arity func * fiddly bit on test * add bit to see order by works right * no default breakout true
-
dpsutton authored
* Remove deprecated friend library - friend has two functions we used: bcrypt and bcrypt-verify. Easy to lift them into our own namespace with attribution - uses simple interop on org.mindrot.jbcrypt.BCrypt to achieve these - also brings in other stuff we don't need ``` com.cemerick/friend 0.2.3 X org.mindrot/jbcrypt 0.3m :use-top <- all we care about X org.clojure/core.cache 0.6.3 :superseded X org.clojure/data.priority-map 0.0.2 :parent-omitted . org.openid4java/openid4java-nodeps 0.9.6 X commons-logging/commons-logging 1.1.1 :older-version . net.jcip/jcip-annotations 1.0 . com.google.inject/guice 2.0 . aopalliance/aopalliance 1.0 ``` And we already declare a dependency on 0.4 of this lib ``` org.mindrot/jbcrypt 0.4 ``` This means we can remove openid4, google.inject/guice, aopalliance, etc and just keep using the same `BCrypt` java class we have been using this whole time. Behavior and classfiles are identical. So very low risk Want to call out a use of ```clojure (when-not api/*is-superuser?* (api/checkp (u.password/bcrypt-verify (str (:password_salt user) old_password) (:password user)) "old_password" (tru "Invalid password"))) ``` This has the same signature of an existing function in `u.password/verify-password`: ```clojure (defn verify-password "Verify if a given unhashed password + salt matches the supplied hashed-password. Returns `true` if matched, `false` otherwise." ^Boolean [password salt hashed-password] ;; we wrap the friend/bcrypt-verify with this function specifically to avoid unintended exceptions getting out (boolean (u/ignore-exceptions (bcrypt-verify (str salt password) hashed-password)))) ``` I did not replace it in this PR so that the diff is essentially `creds/<fn>` -> `u.password/<fn>` and very easy to structually see what is going on. But totally makes sense to clean up the usages of these in another pass * sort ns * simple tests
-
- Aug 02, 2022
-
-
dpsutton authored
* Handle flakiness with geojson java.net.UnknownHostException errors In CI seems like we are getting errant errors: ```clojure geojson.clj:62 It validates URLs and files appropriately http://0xc0000200 expected: (valid? geojson) actual: #error { :cause "Invalid IP address literal: 0xc0000200" :via [{:type clojure.lang.ExceptionInfo :message "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to a file on the classpath. URLs referring to hosts that supply internal hosting metadata are prohibited." :data {:status-code 400, :url "http://0xc0000200"} :at [metabase.api.geojson$valid_url_QMARK_ invokeStatic "geojson.clj" 62]} {:type java.net.UnknownHostException :message "0xc0000200" :at [java.net.InetAddress getAllByName "InetAddress.java" 1340]} {:type java.lang.IllegalArgumentException :message "Invalid IP address literal: 0xc0000200" :at [sun.net.util.IPAddressUtil validateNumericFormatV4 "IPAddressUtil.java" 150]}] ``` Not clear if this change has a hope of fixing it: if it doesn't resolve once its possible it is cached somewhere in the network stack, or it won't resolve if you ask again. But gonna give it a shot. Set the property `"networkaddress.cache.negative.ttl"` to `"0"` > networkaddress.cache.negative.ttl (default: 10) > Indicates the caching policy for un-successful name lookups from the name service. The value is specified as an integer to indicate the number of seconds to cache the failure for un-successful lookups. > A value of 0 indicates "never cache". A value of -1 indicates "cache forever". From https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/InetAddress.html in the hopes that we can try multiple times. Restores the original value after the test completes so we don't inadvertently change behavior elsewhere. If we get an error of java.net.UnknownHostException we try again if we have attempts remaining. If we get a boolean it means the ip resolution worked so we can rely on the response (checking if it resolves locally or not) * add a delay * comment out test
-
Howon Lee authored
Enforce bigint for all int fields derived from JSON instead of doing more complete refactor (#24494) Pursuant to #22732. Should basically eliminate it with prejudice by saying that any int should fit bigints in our point of view in postgres.
-
- Aug 01, 2022
-
-
Ngoc Khuat authored
* correct format datetime property for snowplow * some docs and comments
-
- Jul 29, 2022
-
-
Noah Moss authored
-
- Jul 28, 2022
-
-
Noah Moss authored
-
dpsutton authored
* Try multi-release true again in our manifest Problem statement: Luiz packs our partner jars (exasol, starburst, etc.) into our jar so they can be "first class" and in cloud. But with the 44 cycle we've run into some issues: ```shell /tmp/j via
Cam Saul <1455846+camsaul@users.noreply.github.com>
-
Howon Lee authored
Pursuant to #23635. Previous behavior was to blank out the nested field columns if there were too many for our arbitrary limit (of 100). However, this silent behavior was very user-unfriendly, and people were perplexed that the feature just seemed to turn off out of nowhere. New, better behavior is to log that there are too many and cut it off at 100, but still have some if there are 100.
-
- Jul 27, 2022
-
-
Ngoc Khuat authored
* revert #23658 * keep the migration to add native_query_snippet.template_tag, add a new migration to drop it * Remove snippet parameter support in fully parametrized check Co-authored-by:Tamás Benkő <tamas@metabase.com>
-
dpsutton authored
* Ensure uploaded secrets are stable Fixes: https://github.com/metabase/metabase/issues/23034 Background: Uploaded secrets are stored as bytes in our application db since cloud doesn't have a filesystem. To make db connections we stuff them into temporary files and use those files. We also are constantly watching for db detail changes so we can recompose the connection pool. Each time you call `db->pooled-connection-spec` we check if the hash of the connection spec has changed and recompose the pool if it has. Problem: These uploaded files have temporary files and we make new temp files each time we call `db->pooled-connection-spec`. So the hashes always appear different: ```clojure connection=> (= x y) true connection=> (take 2 (clojure.data/diff (connection-details->spec :postgres (:details x)) (connection-details->spec :postgres (:details y)))) ({:sslkey #object[java.io.File 0x141b0f09 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_1388256635324085910.tmp"], :sslrootcert #object[java.io.File 0x6f443fac "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_9248342447139746747.tmp"], :sslcert #object[java.io.File 0xbb13300 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_17076432929457451876.tmp"]} {:sslkey #object[java.io.File 0x6fbb3b7b "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_18336254363340056265.tmp"], :sslrootcert #object[java.io.File 0x6ba4c390 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_11775804023700307206.tmp"], :sslcert #object[java.io.File 0x320184a0 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_10098480793225259237.tmp"]}) ``` And this is quite a problem: each time we get a db connection we are making a new file, putting the contents of the secret in it, and then considering the pool stale, recomposing it, starting our query. And if you are on a dashboard, each card will kill the pool of the previously running cards. This behavior does not happen with the local-file path because the secret is actually the filepath and we load that. So the file returned is always the same. It's only for the uploaded bits that we dump into a temp file (each time). Solution: Let's memoize the temp file created by the secret. We cannot use the secret as the key though because the secret can (always?) includes a byte array: ```clojure connection-test=> (hash {:x (.getBytes "hi")}) 1771366777 connection-test=> (hash {:x (.getBytes "hi")}) -709002180 ``` So we need to come up with a stable key. I'm using `value->string` here, falling back to `(gensym)` because `value->string` doesn't always return a value due to its cond. ```clojure (defn value->string "Returns the value of the given `secret` as a String. `secret` can be a Secret model object, or a secret-map (i.e. return value from `db-details-prop->secret-map`)." {:added "0.42.0"} ^String [{:keys [value] :as _secret}] (cond (string? value) value (bytes? value) (String. ^bytes value StandardCharsets/UTF_8))) ``` Why did this bug come up recently? [pull/21604](https://github.com/metabase/metabase/pull/21604) gives some light. That changed `(hash details)` -> `(hash (connection-details->spec driver details))` with the message > also made some tweaks so the SQL JDBC driver connection pool cache is > invalidated when the (unpooled) JDBC spec returned by > connection-details->spec changes, rather than when the details map > itself changes. This means that changes to other things outside of > connection details that affect the JDBC connection parameters, for > example the report-timezone or start-of-week Settings will now properly > result in the connection pool cache being flushed So we want to continue to hash the db spec but ensure that the spec is stable. * typehint the memoized var with ^java.io.File * Switch memoization key from String to vector of bytes Copying comment from Github: When you upload a sequence of bytes as a secret, we want to put them in a file once and only once and always reuse that temporary file. We will eventually hash the whole connection spec but I don't care about collisions there. It's only given the same sequence of bytes, you should always get back the exact same temporary file that has been created. So i'm making a function `f: Secret -> file` that given the same Secret always returns the exact same file. This was not the case before this. Each uploaded secret would return a new temporary file with the contents of the secret each time you got its value. So you would end up with 35 temporary files each with the same key in it. An easy way to get this guarantee is to memoize the function. But the secret itself isn't a good key to memoize against because it contains a byte array. If the memoization key is the byte-array itself, this will fail because arrays have reference identity: ```clojure user=> (= (.getBytes "hi") (.getBytes "hi")) false ``` So each time we load the same secret from the database we get a new byte array, ask for its temp file and get a different temp file each time. This means that memoization cannot be driven off of the byte array. But one way to gain this back quickly is just stuff those bytes into a string, because strings compare on value not identity. This is what is currently in the PR (before this change). I was banking on the assumption that Strings are just opaque sequences of bytes that will compare byte by byte, regardless of whether those bytes make sense. But you've pointed out a good point that maybe that is flirting with undefined behavior. If we use the hash of the contents of the byte array as the memoization key with (j.u.Arrays/hashCode array), then we open ourselves up the (albeit rare) case that two distinct secret values hash to the same value. This sounds really bad. Two distinct secrets (think two ssh keys) but both would map to only one file containing a single ssh key. An easy way to have the value semantics we want for the memoization is just to call (vec array) on the byte array and use this sequence of bytes as the memoization key. Clojure vectors compare by value not reference. So two secrets would return the same file if and only if the sequence of bytes are identical, in which case we would expect the files to be identical. This gives me the same guarantee that I was wanting from the String behavior I used initially without entwining this with charsets, utf8, etc.
-
Howon Lee authored
Pursuant to #24214. Previously MySQL JSON spunout fields didn't work when the individual fields had heterogeneous contents because type information was missing, unlike where type information was provided in Postgres JSON fields. Now they are provided, along with a refactor to not use reify which was used in Postgres JSON fields because the type operator was really annoying otherwise to add (MySQL type cast is a function qua function).
-
- Jul 26, 2022
-
-
metamben authored
This change makes sure that false as the default value in a case statement is not ignored.
-
Braden Shepherdson authored
-
Cam Saul authored
* Remove the `Dependency` model code * Appease the namespace linter again.
-
- Jul 25, 2022
-
-
adam-james authored
* Add a check to PUT /user/:id to disallow name edits if an SSO user * Clean up After SAML SSO tests The `:sso_source` key is set for the Rasta user in some SAML tests, but is expeted to be nil in subsequent tests, so we clean up in the SAML test ns. * Add a test to ensure SSO user names can't be changed via API * Missed a change I had made while adjusting tests * valid-name-update? take 1 name, to allow better error messages Let's the user know that first or last name is the cause of a problem, rather than just 'names'. * Remove unneeded thread macro * Use partial= * slight change to local fn for reusability
-
dpsutton authored
* Ignore fields in joins when hoisting joined fields Need to understand two sides of mbql to understand this. ```clojure (defn nest-expressions "Pushes the `:source-table`/`:source-query`, `:expressions`, and `:joins` in the top-level of the query into a `:source-query` and updates `:expression` references and `:field` clauses with `:join-alias`es accordingly. See tests for examples. This is used by the SQL QP to make sure expressions happen in a subselect." ...) ``` Make a query against the orders table joined to the people table and select order id, order total, and the person's email: ```sql SELECT "PUBLIC"."orders"."id" AS "ID", "PUBLIC"."orders"."total" AS "TOTAL", "People - User"."email" AS "People - User__EMAIL" FROM "PUBLIC"."orders" LEFT JOIN "PUBLIC"."people" "People - User" ON "PUBLIC"."orders"."user_id" = "People - User"."id" LIMIT 1048575 ``` Now add a custom column called adjective, which is 'expensive' when total > 100 else 'cheap' ```sql SELECT "source"."id" AS "ID", "source"."total" AS "TOTAL", "source"."adjective" AS "adjective", "source"."people - user__email" AS "People - User__EMAIL" FROM (SELECT "PUBLIC"."orders"."id" AS "ID", "PUBLIC"."orders"."user_id" AS "USER_ID", "PUBLIC"."orders"."product_id" AS "PRODUCT_ID", "PUBLIC"."orders"."subtotal" AS "SUBTOTAL", "PUBLIC"."orders"."tax" AS "TAX", "PUBLIC"."orders"."total" AS "TOTAL", "PUBLIC"."orders"."discount" AS "DISCOUNT", "PUBLIC"."orders"."created_at" AS "CREATED_AT", "PUBLIC"."orders"."quantity" AS "QUANTITY", CASE WHEN "PUBLIC"."orders"."total" > 50 THEN 'expensive' ELSE 'cheap' end AS "adjective", "People - User"."email" AS "People - User__EMAIL", "People - User"."id" AS "People - User__ID" FROM "PUBLIC"."orders" LEFT JOIN "PUBLIC"."people" "People - User" ON "PUBLIC"."orders"."user_id" = "People - User"."id") "source" LIMIT 1048575 ``` We put the "work" in a nested query and then just update the outer select to select `source.total` instead of `orders.total`. But the way this figured out which joins to hoist up was a bit broken. It walked all over the inner query, finding fields it was interested in. However, it also got fields it shouldn't have, by descending into join information that should be opaque at this level. In the repro for this, we make a card Q1, selecting product_id and count, but with an implicit join to products and filtering where category = doohickey. ```clojure ;; card Q1 {:type :query, :query {:source-table 4, #_ reviews :filter [:= [:field 26 {:source-field 33}] "Doohickey"], #_ products.category :aggregation [[:count]], :breakout [[:field 33 nil]], #_ reviews.product_id :limit 2}, :database 1} ``` A second question Q2 queries the Orders table and joins to Q1 on orders.product_id = q1.product_id, and adds a custom column `+ 1 1`. ``` ;; card Q2, based on Q1 {:source-table 2, :expressions {"CC" [:+ 1 1]}, :fields [[:field 9 nil] [:field 3 nil] [:field 5 nil] [:field 6 nil] [:field 8 nil] [:field 7 nil] [:field 1 nil] [:field 4 {:temporal-unit :default}] [:field 2 nil] [:expression "CC" {:metabase.query-processor.util.add-alias-info/desired-alias "CC", :metabase.query-processor.util.add-alias-info/position 9}] [:field 33 nil] [:field "count" {:base-type :type/BigInteger}]], :joins [{:alias "Question 4918", :strategy :left-join, :fields [[:field 33 nil] [:field "count" {:base-type :type/BigInteger}]], :condition [:= [:field 5 nil] [:field 33 nil]], :source-card-id 4918, }]} ``` and this should yield the sql: ```sql SELECT "source"."ID" AS "ID", "source"."PRODUCT_ID" AS "PRODUCT_ID", "source"."TOTAL" AS "TOTAL", "source"."CC" AS "CC", "source"."Question 4918__PRODUCT_ID" AS "Question 4918__PRODUCT_ID", "source"."Question 4918__count" AS "Question 4918__count" FROM ( SELECT "PUBLIC"."ORDERS"."ID" AS "ID", "PUBLIC"."ORDERS"."USER_ID" AS "USER_ID", "PUBLIC"."ORDERS"."PRODUCT_ID" AS "PRODUCT_ID", "PUBLIC"."ORDERS"."SUBTOTAL" AS "SUBTOTAL", "PUBLIC"."ORDERS"."TAX" AS "TAX", "PUBLIC"."ORDERS"."TOTAL" AS "TOTAL", "PUBLIC"."ORDERS"."DISCOUNT" AS "DISCOUNT", "PUBLIC"."ORDERS"."CREATED_AT" AS "CREATED_AT", "PUBLIC"."ORDERS"."QUANTITY" AS "QUANTITY", (1 + 1) AS "CC", "Question 4918"."PRODUCT_ID" AS "Question 4918__PRODUCT_ID", "Question 4918"."count" AS "Question 4918__count" FROM "PUBLIC"."ORDERS" LEFT JOIN ( SELECT "PUBLIC"."REVIEWS"."PRODUCT_ID" AS "PRODUCT_ID", count(*) AS "count" FROM "PUBLIC"."REVIEWS" LEFT JOIN "PUBLIC"."PRODUCTS" "PRODUCTS__via__PRODUCT_ID" ON "PUBLIC"."REVIEWS"."PRODUCT_ID" = "PRODUCTS__via__PRODUCT_ID"."ID" WHERE "PRODUCTS__via__PRODUCT_ID"."CATEGORY" = 'Doohickey' GROUP BY "PUBLIC"."REVIEWS"."PRODUCT_ID" ORDER BY "PUBLIC"."REVIEWS"."PRODUCT_ID" ASC) "Question 4918" ON "PUBLIC"."ORDERS"."PRODUCT_ID" = "Question 4918"."PRODUCT_ID") "source" LIMIT 1048575 ``` But when it was looking through to see which fields to hoist top level, it was also finding `"PRODUCTS__via__PRODUCT_ID"."ID"` and `"PRODUCTS__via__PRODUCT_ID"."CATEGORY"` as fields it should hoist up because it searched the whole tree underneath it which includes join conditions and such. But of course that doesn't matter, the only thing is really analyzing what fields come out of a query, and those fields do not come out of the nested query ```sql SELECT "PUBLIC"."REVIEWS"."PRODUCT_ID" AS "PRODUCT_ID", count(*) AS "count" FROM "PUBLIC"."REVIEWS" LEFT JOIN "PUBLIC"."PRODUCTS" "PRODUCTS__via__PRODUCT_ID" ON "PUBLIC"."REVIEWS"."PRODUCT_ID" = "PRODUCTS__via__PRODUCT_ID"."ID" WHERE "PRODUCTS__via__PRODUCT_ID"."CATEGORY" = 'Doohickey' GROUP BY "PUBLIC"."REVIEWS"."PRODUCT_ID" ORDER BY "PUBLIC"."REVIEWS"."PRODUCT_ID" ASC) "Question 4918" ``` that only returns product_id and count. And this matches the error it was (helpfully) throwing: ```clojure investigation=> (qp/compile nested-query) Execution error (ExceptionInfo) at metabase.query-processor.util.add-alias-info/field-source-table-alias (add_alias_info.clj:182). Cannot determine the source table or query for Field clause [:field 26 #_ products.category {:source-field 33, #_reviews.product_id :join-alias "PRODUCTS__via__PRODUCT_ID", :metabase.query-processor.util.add-alias-info/source-table "PRODUCTS__via__PRODUCT_ID", :metabase.query-processor.util.add-alias-info/source-alias "CATEGORY"}] ``` And here's the query it was analyzing when determining which fields it needed to hoist (looking for ones with `:join-alias` (which is also in the nest_query_test.clj file): ```clojure ;; removed some cruft and extra field information for brevity {:source-table 2, :expressions {"CC" [:+ 1 1]}, :fields [[:field 33 {:join-alias "Question 4918",}] [:field "count" {:join-alias "Question 4918"}]] :joins [{:alias "Question 4918", :strategy :left-join, :fields [[:field 33 {:join-alias "Question 4918"}] [:field "count" {:join-alias "Question 4918"}]] :condition [:= [:field 5 nil] [:field 33 {:join-alias "Question 4918",}]], :source-card-id 4918, :source-query {:source-table 4, ;; nested query has filter values with join-alias that should not ;; be selected :filter [:= [:field 26 {:join-alias "PRODUCTS__via__PRODUCT_ID"}] [:value "Doohickey" {}]], :aggregation [[:aggregation-options [:count] {:name "count"}]], :breakout [[:field 33 nil]], :limit 2, :order-by [[:asc [:field 33 nil]]], ;; nested query has an implicit join with conditions that should ;; not be selected :joins [{:alias "PRODUCTS__via__PRODUCT_ID", :strategy :left-join, :condition [:= [:field 33 nil] [:field 30 {:join-alias "PRODUCTS__via__PRODUCT_ID"}]] :source-table 1, :fk-field-id 33}]}, :source-metadata [{:field_ref [:field 33 nil]} {:field_ref [:aggregation 0]}]}]} ``` * Add round-trip test through qp This test is essentially the repro from the ticket https://github.com/metabase/metabase/issues/20809 ```clojure debug-qp=> (to-mbql-shorthand (:dataset_query (metabase.models/Card 4919))) (mt/mbql-query orders {:joins [{:source-table "card__4918", :alias "Question 4918", :condition [:= $product_id [:field %reviews.product_id {:join-alias "Question 4918"}]], :fields :all}], :expressions {"CC" [:+ 1 1]}}) debug-qp=> (to-mbql-shorthand (:dataset_query (metabase.models/Card 4918))) (mt/mbql-query reviews {:breakout [$product_id], :aggregation [[:count]], :filter [:= $product_id->products.category "Doohickey"]}) ``` Thanks to @cam for providing such a lovely tool
-
- Jul 23, 2022
-
-
metamben authored
-
- Jul 22, 2022
-
-
metamben authored
-
Braden Shepherdson authored
`PulseChannelRecipients` are a `:recipients` field on `PulseChannels`. This requires some careful handling when inserting or updating a `PulseChannel` to upsert the recipients.
-
- Jul 21, 2022
-
-
Noah Moss authored
-
Braden Shepherdson authored
-
Cal Herries authored
* Update search query to not match search string tokens with dataset_query column if the question is not native * Add test * Rewrite test * Rename query-card -> mbql-card * Use the correct dataset_query map
-
Cal Herries authored
* Correct `score-and-result` to count the weight of the text score when there is no text match * Replace nil-punning of scores with explicit checking for zero scores at each step * Update scoring functions that can return nil to return zero instead * Address archived search with no search string * Update e2e test * Update e2e test with new ordering of results * Fix dataset search with blank query string
-
Braden Shepherdson authored
-
- Jul 20, 2022
-
-
Noah Moss authored
* basic logic for including params in dashboard subscriptions * BE date formatting * redo approach to date formatting locale * refactor and use the same formatting code for parameters listed in subscription header * fix tests and lint * fix more tests * fix final (?) test and fix namespace lint error * a couple extra test cases * fix tests * new cypress test * address braden and ngoc's comments * fix tests * guard against nil text in substitute_tags
-
Ngoc Khuat authored
* fixing tests and make chain-filter dashboard endpoints returns the flag * remove using anonymous function in update * Update dashboard endpoints and `FieldValuesWidget` to handle `has_more_values` scenario (#24022) * update dashboard action/reducer * Update FieldValuesWidget * Update isSearchable to accept map of args * Add unit tests for various FVW utils * Split isSearchable logic a little Co-authored-by:Dalton <daltojohnso@users.noreply.github.com>
-
Braden Shepherdson authored
-
Cam Saul authored
* Remove `u/key-by` * Fix typo * SORT THE NAMESPACES
-
- Jul 18, 2022
-
-
Braden Shepherdson authored
Cam suggested most of these changes in another PR, and I'm tackling them here.
-
Case Nelson authored
Fixes #23938 metabase.driver.ddl.postgres should be named metabase.driver.postgres.ddl metabase.driver.ddl.mysql should be named metabase.driver.mysql.ddl metabase.driver.ddl.sql should be named metabase.driver.sql.ddl Renamed aliases to match new structure
-
Noah Moss authored
-
Ngoc Khuat authored
* add snowplow tracking for new records on TaskHistory * more tests and have a better way to figure out db_id, db_engine
-
- Jul 15, 2022
-
-
metamben authored
-
dpsutton authored
* Check email test errors for javax.mail.AuthenticationFailedException Authing with Office365 led to a less-than-great user experience. If you type in the wrong username or password, we would display an unhelpful message: > Sorry, something went wrong. Please try again. Error::Exception reading response.Read timed out. This is an error from an inner nested exception which looks like: ```clojure (javax.mail.AuthenticationFailedException. "" ;; Office365 returns auth exception with no message so we only saw "Read timed out" prior (javax.mail.MessagingException. "Exception reading response" (java.net.SocketTimeoutException. "Read timed out"))) ``` We didn't get any message from the AuthenticationFailedException so our humanization strategy only went on the underlying "Read timed out". Now we can check against the error class and use that information. * Little bit of clarity better function name (`check` -> `match-error`), better arg name ( `regex|exception-class` -> `regex-or-exception-class`), full argument names `m` -> `message` and `es` -> `exceptions`
-
dpsutton authored
The frontend receives obfuscated values for settings marked sensitive: ```clojure (defsetting email-smtp-password (deferred-tru "SMTP password.") :sensitive? true) ``` and over the wire: ```javascript { "key": "email-smtp-password", "value": "**********ig", "is_env_setting": false, "env_name": "MB_EMAIL_SMTP_PASSWORD", "description": "SMTP password.", "default": null }, ``` So the frontend form never has a valid password in it. When you edit email settings, we check if those are valid, and if so, commit the settings. But with the wrong password email settings are almost always wrong. You have to re-type the email password just to change other settings, like the friendly name, reply-to email address, etc. So we recognize that we've received the obfuscated value, swap in the real value for the email connection test, and then obfuscate the password in the response. If we do not receive an obfuscated value, just leave it alone and return the value anyways (they typed it in, so seems safe to send it back). Note that there is a function in models.setting called `obfuscated-value?` that checks strings against a regex of `#"^\*{10}.{2}$"`. This is used to never set settings to an obfuscated value. But its a bit less sensitive than our purposes need. We have a real value and an obfuscated value so we can check if the obfuscated value is based on the real value. (obfuscate-value "password") -> "**********rd" . Whereas we might recognize "**********AA" as the obfuscated value if we reused that helper function. NOTE this could be weird if anyone's password changes from "password" to "**********rd" but the chances of that happening are miniscule. Had thought to have the FE not send a value at all if it is unchanged but this had some far-reaching implications that we don't want to tackle so close to the release. There's an associated issue for LDAP settings that is largely the same as this: https://github.com/metabase/metabase/issues/16708 But it is not clear to me if these are the only two places or if there could be others. Until that research is done we can just accept this patch as is and come up with a systematic approach in the future. But it does seem only three settings are sensitive: ```clojure setting=> (->> (vals @registered-settings) (filter :sensitive?) (map :name)) (:saml-keystore-password :email-smtp-password :ldap-password) ``` The direct setting apis won't write setting values which appear as obfuscated, but we have other endpoints that take collections of settings as a unit (for instance, the endpoint /api/email that takes all of the email settings here to test if they work together).
-
