Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Nov 17, 2021
    • Jeff Evans's avatar
      Secrets :closed_lock_with_key: PR 6 - Update Oracle properties (#18320) · 0480c2cc
      Jeff Evans authored
      
      * Add SSL keystore and truststore secret properties to Oracle driver YAML files
      
      
      Update Oracle driver to set keystore and truststore connection options from secret values
      
      Adding new `select-keys-sequentially` helper function for dealing with assertions on transformed conn props
      
      Add new function to the secret namespace to return a lighter weight "secret map", and remove that functionality from the `handle-db-details-secret-prop!`, so that it can be reused from connection testing
      
      Modifying CircleCI to set the corresponding truststore secret conn prop vars instead of JVM-level truststore settings, as the test had been doing previously
      
      Expand Oracle test connection details to incorporate all SSL related properties, and also make it a function instead of delayed def (so changes can be picked up later)
      
      Misc. fixes in Oracle driver YAML files
      
      Update `exception-classes-not-to-retry` to include SSLHandshakeException
      
      Fix misc issues in secret and database model code
      
      Update CircleCI config to use correct secret based env var keys and values
      
      Add *database-name-override* dynamic var in `metabase.test.data.interface` to allow for DB name overriding
      
      Fix up ssl connectivity test so all parts pass successfully
      
      Get rid of crazy with-redefs to swap in the existing test-data's schema, and instead, just dropping the DB if it was created under a different schema (similar to what H2 does)
      
      Co-authored-by: default avatarGustavo Saiani <gustavo@poe.ma>
      Unverified
      0480c2cc
  2. Nov 15, 2021
  3. Nov 12, 2021
  4. Nov 11, 2021
    • dpsutton's avatar
      Map tiles improvements (#18912) · 1c70882b
      dpsutton authored
      * Speed up interger or string recognition
      
      jeff helpfully pointed this out. I did some repl benchmarks
      
      ```clojure
      (comment
        (doseq [x ["3" "bob"]]
          (dotimes [_ 3] (time (dotimes [_ 10000] (try (Integer/parseInt x)
                                                        (catch NumberFormatException _ x))))))
        (doseq [x ["3" "bob"]]
          (dotimes [_ 3] (time (dotimes [_ 10000] (if (re-matches #"\d+" x)
                                                    (Integer/parseInt x)
                                                    x))))))
      
      tiles-test=> (doseq [x ["3" "bob"]]
                     (dotimes [_ 3] (time (dotimes [_ 10000] (try (Integer/parseInt x)
                                                                  (catch NumberFormatException _ x))))))
      "Elapsed time: 5.72825 msecs"
      "Elapsed time: 1.531042 msecs"
      "Elapsed time: 0.797041 msecs"
      "Elapsed time: 32.120875 msecs"
      "Elapsed time: 27.848375 msecs"
      "Elapsed time: 23.820542 msecs"
      nil
      tiles-test=> (doseq [x ["3" "bob"]]
                     (dotimes [_ 3] (time (dotimes [_ 10000] (if (re-matches #"\d+" x)
                                                               (Integer/parseInt x)
                                                               x)))))
      "Elapsed time: 5.356417 msecs"
      "Elapsed time: 1.97225 msecs"
      "Elapsed time: 1.868708 msecs"
      "Elapsed time: 1.214666 msecs"
      "Elapsed time: 1.185625 msecs"
      "Elapsed time: 1.19025 msecs"
      nil
      tiles-test=>
      ```
      
      * Only select lat/lon fields in api/tiles
      
      We required the index of lat and long columns, ran the whole query, and
      then just grabbed those columns in a `(for [row rows] [(nth row
      lat-idx)..])`. But of course we can just update the fields we want to
      select in the query.
      
      MBQL and native queries flow through this codepath depending on the
      source of the query of the card being mapped. For mbql queries, it is
      straightforward, add a filter on the lat long and replace the fields
      with just lat and long fields. For native, we nest the native query as a
      nested query and then proceed on the resulting mbql query. The only
      difference is requiring the type annotation for the field type.
      
      mbql field: [:field 32 nil] vs [:field "latitude" {:base-type :type/Float}]
      
      This addresses memory concerns. The query is not limited and we don't
      need to select an entire row for these purposes. Dropping lots of text
      fields, id fields, etc could save quite a bit of memory when resolving
      the entire result set in memory.
      
      * Add limit to number of coordinates per tile
      
      these queries were unbounded in getting coordinates for each tile. But
      in a visualization, can there really be more information provided? We
      were leaving them unbounded and getting back millions of rows (reported
      on #4844). So we were adding unnecessary detail at the price of OOM
      errors.
      
      Note tests have been changed to show that the limit of the original mbql
      query is clobbered and the limit of the original native query is only
      present in the nested query whereas the outer query selects its own
      limit.
      
      As always, should this number be exposed as a setting? Configurable in
      the UI? Leaving as a hardcoded and documented number for the moment.
      
      * docstring
      
      * Remove col indices from api/tiles
      
      since we just select the actual columns we want, we no longer need the
      index in the results of those cols. they are always the first and second
      columns
      
      * Trim final slash on url
      Unverified
      1c70882b
    • dpsutton's avatar
      Nest qbnewb under /modal to allow for other modals (#18926) · 11d9bd55
      dpsutton authored
      * Nest qbnewb under /modal to allow for other modals
      
      move the api from api/user/:id/qbnewb -> api/user/:id/modal/qbnewb to
      allow for the upcoming new dismissable modal
      
      * Add additional datasetnewb route
      
      staying with the same naming scheme as qbnewb. the db field is
      is_datasetnewb, the route just takes datasetnewb.
      
      I'm not wild about the default is true, set to false, and the naming
      scheme. But will be easier to migrate and easier for FE to use if we
      continue the defaults/logic etc.
      
      * Remark on db change in migration file
      Unverified
      11d9bd55
  5. Nov 10, 2021
  6. Nov 09, 2021
    • Jeff Evans's avatar
      Secrets :closed_lock_with_key: PR 5 - Translate secret connection-properties into existing types... · 13e9414b
      Jeff Evans authored
      Secrets :closed_lock_with_key: PR 5 - Translate secret connection-properties into existing types understood by frontend (#18236)
      
      * Translate secret connection-properties into existing types understood by frontend
      
      For all :type :secret properties, before returning over the wire (i.e. from `available-drivers-info`), translate the connection-property into one or more new client side properties, using the existing types, to encapsulate the behavior of that individual secret "unit".  Depending on the :secret-kind and hosting environment, this could mean there are multiple additional properties injected (one for the file upload versus local path option field, one for the value itself on upload, and one for the file path)
      
      Adding utility function to do the reverse (i.e. translating those client side properties back to the server secret property definition)
      
      Adding tests for both (including updating secret-test-driver to cover all of these)
      
      Add support for the treat-before-posting connection property config
      
      * Fix bug in `delete-orphaned-secrets!`
      Unverified
      13e9414b
  7. Nov 08, 2021
  8. Nov 05, 2021
  9. Nov 04, 2021
    • Jeff Evans's avatar
      Don't qualify breakout field name for BigQuery when source is not from a table (#18772) · a48e6a4b
      Jeff Evans authored
      Add a new dynamic var - *source-query* - to sqp.qp to bind the current source-query, and bind that in the same place that *table-alias* is currently bound
      
      Update bigquery-cloud-sqp qp implementation to check the sqp.qp/*source-query*, and only qualify the source query alias if the type of source query is a table (which is apparently required; see #15074)
      
      Add test for #18742
      Unverified
      a48e6a4b
    • dpsutton's avatar
      Handle api/tiles requests for native queries (#18810) · d55ea823
      dpsutton authored
      * Handle api/tiles requests for native queries
      
      Ordinarily the api takes the source query and adds in a filter clause
      for a region:
      
      ```clojure
      {:database 19
       :query {:source-table 88
               :fields [[:field 562 nil]
                        [:field 574 nil]
                        [:field 576 nil]]
               :limit 2000}
       :type :query}
      
      {:database 19
       :query {:source-table 88
               :fields [[:field 562 nil]
                        [:field 574 nil]
                        [:field 576 nil]]
               :limit 2000
               :filter [:inside
                        [:field 576 nil]
                        [:field 574 nil]
                        -40.97989944013222
                        -179.99999564141046
                        -66.51326189011354
                        -134.99999673105785]}
       :type :query
       :async? false}
      ```
      
      But when native, this would break, for three different reasons:
      - native queries don't necessarily (or possibly) know their field
      ids. So the api request it would construct would have `undefined` in the
      slot where the field-id would normally go. This would cause the route to
      404 as it would fail to match the expected numeric part of the url
      - just passing in the field name isn't suffiencient, because mbql cannot
      natively use an mbql filter clause into a native snippet. We do this to
      a limited extent with filters and substitution but this requires a
      marker like `{{filter}}` placeholder for us to add the filter text.
      - when using `[:field name ...]` type field references we need the
      base-type of a field in order to construct a query.
      
      So the solution:
      - allow the route to take field ids or names
      - if native, rewrite the query to be a nested query using the native
      query as the source query. ie, "select name, lat, long from table" ->
      "select source.* from (select name, lat, long from table) source" but in
      mbql so we can add our fliter clause
      - if a string name is passed in, annotate it with a base-type of
      :type/Float since we are dealing with lats and longs.
      
      A concern was that we need to include the source-metadata. I'm omitting
      this because we are essentially "select * from nested" so we just take
      what we want, and the route already expects the index of the lat and
      long columns and then selects only those from the query results.
      
      As it stands we end up with a working query but a log
      
      ```
      2021-11-02 14:37:25,470 DEBUG middleware.log :: GET /api/tiles/2/1/1/latitude/longitude/1/2/ 200 47.3 ms (5 DB calls) App DB connections: 2/13 Jetty threads: 9/50 (2 idle, 0 queued) (128 total active threads) Queries in flight: 0 (0 queued); postgres DB 19 connections: 3/6 (0 threads blocked)
      2021-11-02 14:37:25,474 WARN middleware.add-implicit-clauses :: Warning: cannot determine fields for an explicit `source-query` unless you also include `source-metadata`.
      2021-11-02 14:37:25,490 WARN middleware.add-implicit-clauses :: Warning: cannot determine fields for an explicit `source-query` unless you also include `source-metadata`.
      2021-11-02 14:37:25,501 DEBUG middleware.log :: GET /api/tiles/2/0/2/latitude/longitude/1/2/ 200 39.4 ms (5 DB calls) App DB connections: 1/13 Jetty threads: 8/50 (2 idle, 0 queued) (128 total active threads) Queries in flight: 0 (0 queued); postgres DB 19 connections: 2/6 (0 threads blocked)
      ```
      
      It might be possible to suppress these logs when we are in a query
      context of `:map-tiles`. Or we might be able to require the frontend to
      send along the metadata to include in the query. I didn't do this yet
      since it seems to work fine and it would just make the urls quite long
      if there are lots of columns (and the metadata can get quite verbose).
      
      ```clojure
      ;; source query of the card
      {:type :native
       :native {:query "select name, latitude, longitude from zomato limit 2000;"
                :template-tags {}}
       :database 19}
      
      ;; nest it into a source query
      {:database 19
       :type :query
       :query {:source-query {:template-tags {}
                              :native "select name, latitude, longitude from zomato limit 2000;"}}}
      
      ;; add the `:inside` filter for the tile
      
      {:database 19
       :type :query
       :query {:source-query {:template-tags {}
                              :native "select name, latitude, longitude from zomato limit 2000;"}
               :filter [:inside
                        [:field
                         "latitude"
                         {:base-type :type/Float}]
                        [:field
                         "longitude"
                         {:base-type :type/Float}]
                        0.0
                        89.99999782070523
                        -66.51326189011354
                        179.99999564141046]}
       :async? false}
      ```
      
      * Add docstring and make function private
      
      * url encode column names to api/tiles
      Unverified
      d55ea823
    • dpsutton's avatar
      Annotate collection tree with datasets and cards (#18833) · cba58286
      dpsutton authored
      * Annotate collection tree with datasets and cards
      
      In order to know which collections have items of interest, they are
      annotated with two new keys: `:here` and `:below`. These keys will have
      a collection (in clojure a set, in js an array) which could contain
      dataset and card, depending if they have those items "here" or in
      collections below this node.
      
      We include information about which subtrees are interesting so we know
      to show or highlight those. We include information about which nodes
      have interesting things versus which contain nodes which have things so
      we can possibly implement drilldown in an intelligent manner.
      
      ie
      
      ```
      .
      ├── root
      ```
      
      could expand automatically to
      
      ```
      .
      ├── root
      │   ├── collection
      │   │   ├── collection that actually has items
      ```
      
      without requiring someone to click on the intermediate collection which
      has no items itself, just a collection with items.
      
      http://localhost:3000/api/collection/tree?tree=true
      
      ```js
      {
          "name": "super",
          "id": 154,
          "location": "/",
          "below": ["dataset"]  ;; indicates collections below have datasets
          "children": [{
              "name": "parent",
              "id": 155,
              "location": "/154/",
              "below": ["dataset"]  ;; indicates the same
              "children": [{
                  "children": [],
                  "slug": "contains",
                  "name": "contains",
                  "here": ["dataset"],  ;; this node has datasets
                  "id": 157,
                  "location": "/154/155/",
              }],
      
          }, {
              "children": [],
              "name": "sibling",
              "id": 156,
              "location": "/154/",
          }],
      }
      ```
      
      * docstring
      
      * Alignment for readability
      Unverified
      cba58286
  10. Nov 03, 2021
    • Cam Saul's avatar
      Add NOT NULL constraint to Card.database_id; attempt to set database_id if possible (#18472) · 83de3e86
      Cam Saul authored
      * Drop long-unused Table.entity_name column
      
      * Add NOT NULL constraint to Card.database_id
      
      * Test fix :wrench:
      
      * Test fix :wrench:
      
      * Fix migration
      
      * Add note about H2 shell to deps.edn
      
      * Add new SQL migration to attempt to set database_id when unset
      
      * Remove data migration that is now done in Liquibase land
      
      * Oops, '$.database', not '$.database_id'
      
      * Parse ints as signed rather than unsigned just to be safe (they *might* be -1337 if they're really broken)
      
      * Don't run for H2
      
      * Update comment
      
      * Fix migration indentation
      
      * Clean namespace
      
      * Use new migration number.
      
      * Use the new migration numbers
      
      * Adopt new migration numbering scheme
      
      * Fix comments
      
      * Fix MySQL + MariaDB insanity
      
      * Fix ID range validation
      
      * Actually 382 is the last legacy ID
      
      * Improved validation and tests
      
      * Adopt the new-new migration ID format.
      
      * Test fixes :wrench:
      
      * Fix merge
      
      * Simplify precondition
      Unverified
      83de3e86
  11. Nov 01, 2021
    • dpsutton's avatar
      Dataset search (#18715) · daed46d5
      dpsutton authored
      * Move the multimethods over to strings not classes
      
      datasets are Cards so don't have a good way to intercept. And this kinda
      took all of the keywords, turned them into classes, and then didn't
      actually use those classes-- they were just opaque keys in the
      hierarchy. No bueno.
      
      * Include datasets in search results
      
      * Docstring for the linter. Thank you clj-kondo
      
      * Fix namespace decls
      
      * Update tests
      
      * Clean up ns and make test more resilient
      
      these tests that test global collections can be sensitive to test order
      and the CI dbs that continually exist rather than be
      recreated. Sometimes "table" shows up in these available models. Kinda
      don't care so just check for subset
      
      * Rename for confusion
      Unverified
      daed46d5
    • dpsutton's avatar
      tree view for datasets (#18704) · 8947f42a
      dpsutton authored
      * tree view for datasets
      
      * Update docstring
      Unverified
      8947f42a
  12. Oct 29, 2021
  13. Oct 28, 2021
    • Jeff Evans's avatar
      Prevent secret records from being orphaned (#18714) · fa7a89d7
      Jeff Evans authored
      Delete Secret instances from the app DB, when the corresponding Database instance is deleted
      
      Updating existing test (and renamed it to `secret-db-details-integration-test`) to confirm this behavior
      Unverified
      fa7a89d7
    • dpsutton's avatar
      Update the keys we consider to unverify a card (#18734) · d33d83f4
      dpsutton authored
      * Update the keys we consider to unverify a card
      
      Previously when almost anything changed we would unverify a card. But
      this was way too aggresive. We really only want to unverify when a query
      changes. Far simpler to specify the keys we do want to consider rather
      than the keys to ignore.
      
      Was prompted because when we marked a question as a dataset it would
      unverify because the `:display` was changed and also `:dataset`. Neither
      of these matter for verification purposes
      
      * Update testing context string [ci skip]
      
      * Empty commit to trigger CI
      
      had skipped ci since it was a docstring change but that prevents merging
      sad face :(
      Unverified
      d33d83f4
  14. Oct 27, 2021
    • Jeff Evans's avatar
      Handle long running sync in connection pool hash check (#18664) · e0765149
      Jeff Evans authored
      * Handle long running sync in connection pool hash check
      
      Modify the logic in `db->pooled-connection-spec` to check for the latest `DatabaseInstance` from the app DB in case of a hash mismatch, before deciding that the pool needs to be invalidated (based on changing hash of db details)
      
      This is to handle the case where a long running sync keeps passing in a stale version of the `DatabaseInstance` over a period of many minutes/hours, long after the app DB has received an updated version that was saved through the UI
      
      In this case, the connection pool code will get the latest `DatabaseInstance` from the app DB, on a hash mismatch, then compare that against the in-memory has for the given DB ID.  And if it still doesn't match, then the invalidation kicks in
      
      Updating the test accordingly
      Unverified
      e0765149
    • Dalton's avatar
      move parameter ui utils to separate file (#18690) · 275e6112
      Dalton authored
      * move parameter ui utils to separate file
      
      * move utils for building parameters from cards to own utils file (#18691)
      Unverified
      275e6112
  15. Oct 26, 2021
  16. Oct 22, 2021
  17. Oct 21, 2021
    • Jeff Evans's avatar
      Secrets :closed_lock_with_key: PR 3 - Integrate secret handling with db-details (#17677) · 497f433d
      Jeff Evans authored
      Integrate secret handling with db-details
      
      Add helper function to database model namespace to "swap out" secret values with inserted IDs
      
      Calling this from the pre-update fn
      
      Add pre-insert fn for database model to perform the same
      
      Adding test to database model test ns
      
      Add creator_id to secret model to capture the user who created this secret instance/version
      
      Add creator-id and created-at sub properties for each secret in db details
      Unverified
      497f433d
    • Jeff Evans's avatar
      Secrets :closed_lock_with_key: PR 2 - Add secret model (#17649) · 0d7786a1
      Jeff Evans authored
      Add secret model
      
      Add Liquibase migration for secret table
      
      Implement bare bones model namespace for secret
      
      Add simple test to ensure secret values are stored and retrieved under an encryption key (or not)
      
      Add :creator_id to secret model to capture the user who created this secret instance/version
      
      Support updating secret values as part of encryption key rotation
      
      Add test assertions for secret values in the `rotate-encryption-key!-test`
      Unverified
      0d7786a1
  18. Oct 20, 2021
    • Jeff Evans's avatar
      Fix calculation in `ttl-hierarchy` (#18495) · 2dc34cd6
      Jeff Evans authored
      Fix calculation in `ttl-hierarchy`
      
      The `ttl-hierarchy` function falls back to `query-magic-ttl` if no stored (card, dashboard, database) cache settings are available.  But that function returns its TTL value in milliseconds, while the model level, granular TTLs are stored in hours.  Convert those to seconds before returning, so that `ttl-hierarchy` is always returning in seconds
      
      Add backend test for this scenario (no stored model ttls, but an average query execution time is available, and hence the `query-magic-ttl` should be used)
      
      Unskip Cypress repro
      Unverified
      2dc34cd6
  19. Oct 19, 2021
  20. Oct 18, 2021
Loading