Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Aug 29, 2024
  2. Aug 28, 2024
    • github-automation-metabase's avatar
    • github-automation-metabase's avatar
    • github-automation-metabase's avatar
      :robot: backported "Much better collection permission performance" (#47251) · cf55fb3d
      github-automation-metabase authored
      * Much better collection permission performance (#46942)
      
      PREFACE: THIS IS A BACKPORT COMMIT. And it was a painful one, because some
      previous very relevant changes hadn't been backported. So there are some
      changes from the original, non-backported, commit:
      
      - removed all support for getting the trash collection, getting things
      that were `archived_directly`, or getting collections by
      `archive_operation_id`. None of these exist in this branch.
      
      - in this branch, functions like
      `permissions-set->visible-collection-ids` did not check for archived
      status or anything other than permissions. On `master`, they do. So, the
      new function that replaced them,
      `collection/visible-collection-filter-clause`, also checks for archived
      status as well as permissions. To resolve this, I chose to just default
      `collection/visible-collection-filter-clause` to ignore archived status
      and return both archived and unarchived collections.
      
      On to the original commit message:
      
      There are a few separate changes here:
      
      - Migrations: add and populate indexed columns perm_value, perm_type, and collection_id to permissions
      
      These fields allows us to efficiently run queries based on collection permissions in the DB without string manipulation. Keeping the table as permissions allows us to do this migration in-place. Note that in some cases collections may have been deleted from the database without deleting the associated permissions row (since there was no foreign key before). We need to be defensive here: if we have a permissions row without a corresponding collection, delete the row before running the rest of the migration.
      
      - Write perm_value, perm_type, and collection_id for new collection permissions
      
      A very simple before-insert method sets these fields before a collection permission is written to the DB.
      
      - Replace collection/permissions-set->visible-collection-ids and collection/visible-collection-ids->honeysql-filter-clause with collection/honeysql-filter-clause
      
      Previously, just about everywhere we used permissions-set->visible-collection-ids, what we were essentially doing was an in-app join: select all the collection IDs you have permission on, then convert it to a SQL clause like WHERE collection_id IN ( all of those collection IDs).
      
      Replace both of these with honeysql-filter-clause, which uses the new fields we added to permissions above to construct a honeysql filter representing "all the collections I have permissions on", without needing to round-trip them to the application and back to the DB.
      
      Of course, we can then write a function visible-collection-ids, which uses honeysql-filter-clause, for those cases where we do actually need the whole bunch in the application (we use this, for example, when constructing the effective-location for a collection).
      
      I also added one more toggle to the VisibilityConfig that's passed into the honeysql-filter-clause (and used to be passed to permissions-set->visible-collection-ids), allowing you to select only the effective children of some collection.
      
      * Backport: Speed up calculation of effective_ancestors
      
      https://github.com/metabase/metabase/pull/47324
      
      Previously, `visible-collection-ids` was effectively "free" in that we'd
      cached `collection-id->collection` for *all* collections, within a
      single request, and then locally filtered it for permissions without
      needing to hit the database again.
      
      To be honest, there is probably a better fix for this - we're repeatedly
      calling `visible-collection-ids` when we probably could just save a
      single copy of it and use it when calculating the effective location of
      every collection.
      
      However, this is a very quick and low-risk fix, and I want to prioritize
      getting this done, and then we can improve it later.
      
      Locally, I copied down the database from stats and timed the
      `/api/search?model_ancestors=true` endpoint.
      
      Before my "speedup" PR (https://github.com/metabase/metabase/pull/46942)
      it took ~7 seconds to return results.
      
      After my "speedup" PR, it took ~15s to return results. :grimace:
      
      With this change, it takes 818ms to return results. :tada:
      
      
      
      ---------
      
      Co-authored-by: default avatarJohn Swanson <john.swanson@metabase.com>
    • github-automation-metabase's avatar
      Support ctrl + click on command palette links again (#47328) (#47365) · 0f2a3959
      github-automation-metabase authored
      
      * remove prevent default on CP links
      
      * unit flakes?
      
      Co-authored-by: default avatarNick Fitzpatrick <nick@metabase.com>
    • github-automation-metabase's avatar
    • github-automation-metabase's avatar
      [QP] Return datetimes in SQL Server, even with date-sized truncation (#47248) (#47350) · 53c52a0c
      github-automation-metabase authored
      
      Previously, truncating a `:type/DateTime` column by `:month` or `:day`
      would return a `:type/Date`, which subtly broke the query.
      
      In particular, if you try to order-by the breakout column `Created At (month)`
      then it would not get de-duplicated, causing a SQL error about conflicting
      ORDER BY clauses.
      
      Fixes #46992.
      
      Co-authored-by: default avatarBraden Shepherdson <braden@metabase.com>
    • Alexander Solovyov's avatar
  3. Aug 27, 2024
  4. Aug 26, 2024
Loading