Sync Fork #20
Open
Sync Fork #20
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* fix: avoid persisting settings that are hidden from dashboard UI * test: add `sanitizeDashcardSettings` unit spec * refactor: use `_.pick` for sanitizing dashcard settings
Co-authored-by: github-actions <github-actions@github.com>
…f the range (#67533) * feat: allow setting custom y axis range, even if points are outside of the range * fix: adjust Waterfall null x value loki screenshot test
* fix: hide untested and unsupported db engines in table editing section * move allowed engines to constants file * test: add hidden editing toggle test case
…#67581) * Prevent unnecessary height calculation * Prioritize local layout from `react-grid-layout` for height calculation * Refix the issue for real * Add tests
* Acquire locks before changing data permissions
Data permission have a read-modify-write cycle:
- we read the existing permissions to see what's currently set
- then we decide what to do (e.g. delete the existing DB level
permissions to write a granular table-level permission)
- then we do the write
This has a race condition if multiple Metabase instances are modifying
permissions at the same time.
For example, say we have:
Initial state: `{:db_id 1 :table_id nil :perm_value :unrestricted}`
Instance A: set table 1 to :blocked
Instance B: set database to :blocked
T1: A reads state, sees db-level permission, plans to expand to table-level
T2: B reads state, sees db-level permission, plans to replace it
T3: A deletes db-level, inserts table-level rows for all tables
T4: B deletes db-level (no-op), inserts new db-level :blocked
Result (CORRUPT):
```
{:db_id 1 :table_id 1 :perm_value :blocked} ;; from A
{:db_id 1 :table_id 2 :perm_value :unrestricted} ;; from A
{:db_id 1 :table_id nil :perm_value :blocked} ;; from B
```
Both db-level AND table-level permissions now exist.
For the permissions API, we take a cluster lock, but sync operations
bypassed this cluster lock. This PR creates a lower-level cluster lock
per `[database, group, perm-type]` so only one instance/thread can do
the read/decide/write cycle for a particular db/group/perm at a time.
* Add a test for race conditions setting data perms
This took a while before I was able to track down the fact that using
`(mt/with-temp )` caused both threads to share the same connection, so
they could obtain the lock simultaneously 🤦
* Use a slightly coarser lock
I realized that if an instance has a huge number of groups, we'll obtain
a lock and ask Postgres to maintain an active savepoint for each group,
which might be untenable.
I think the best solution here is to use slightly coarser locks. This
might increase contention slightly, since we won't be able to change
permissions for `db 1, group 1, perm type view-data` at the same time
that we change permissions for `db 1, group 2, perm type view-data`. But
I think the tradeoff here is necessary.
) * Add field grouping of host and port * Add host and port settings to postgres, mysql * Add host and port grouping to db drivers * Add database story with host and port group * Add groupFields unit tests * Fix typo Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix missing tunnel parameter value Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactor and simplify field grouping function * feat: add nested fields support for db format * feat: remove id field, adapt new format foy postgres, mongo, mysql, remove leftovers * feat: support nested fields, add backdrop component * fix: group inputs loosing focus on type/render * refactor: simplify custom containers logic * style: adjust backdrop container style * refactor: field mappers for db forms * refactor: move host and port config to the new format * refactor: cleanup old grouping approach * refactor: remove unneeded property * fix: typo in the html prop name * internal: validate container style comping from BE this value can be set be external drivers so it's better to validate it Signed-off-by: Jakub Chodorowicz <jakub.chodorowicz@metabase.com> * fix: type errors * fix: driver translation test * refator: use existing getFlattenedFields function * fix: flatten properties for connection property uniquness * backtrack from ssl grouping for mongo --------- Signed-off-by: Jakub Chodorowicz <jakub.chodorowicz@metabase.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- https://github.com/metabase/metabase/security/code-scanning/543 about snowflake's of grpc netty shaded. We already have this version elsewhere so now it matches - https://github.com/metabase/metabase/security/code-scanning/549 and https://github.com/metabase/metabase/security/code-scanning/557 issue in databricks. We bumped that and now it uses the new coordinate `at.yawk.lz4/lz4-java 1.10.1` - https://github.com/metabase/metabase/security/code-scanning/496 bump commons-lang3. actually just exclude it from the driver jar so it will be found on parent classpath with coordinate from deps.edn: `{:mvn/version "3.19.0"}`
* disable chunked encoding if using path style access * mage task to start python runner with mock s3
Fixes #65838. This issue was about dashboard filters showing the wrong value when the filter was bound to an FK field from a table with multiple FKs to the same foreign table. The dashboard parameters logic on the BE was choosing which FK to use for the filter values arbitrarily, meaning that binding the filter to one of them would work fine, but the other would give incoherent results. The fix is to be deliberate about selecting the parameter's column. That doesn't solve all ambiguous cases, but we really can't help if there's a multi-hop join that has parallel edges in the middle!
* kick ci * Disable remaps when calculating columns for a query. * Add tests for result_metadata remappings * Add comments * Whitespace
* feat: add git settings modal to data studio * Refactor GitSettingsModal to reuse existing settings form / Adjust some styles * Fix and improve unit tests * Adjust font size according to variant * fix unit test * fixup! mock API not other plugins in this layout * Split unit test files and improve endpoints setup * Implement PLUGIN_REMOTE_SYNC.GitSyncSetupMenuItem with internal rendering condition --------- Co-authored-by: Anderson <anderson@metabase.com>
* Update issue tempaltes * Delete .github/ISSUE_TEMPLATE/task.md * Update verbiage for new feature request
Updated curl command formatting and clarified instructions for exporting and importing Metabase data.
* use tree table in the data model section of data studio * review
* Add some additional incremental transforms tests * Add missing require * Use driver/table-name-length-limit
Allow clicking while animating
* Implement read-only on Library page * Add some gorgeous unit tests * Make segment and measures read-only / Add tests * Apply read-only logic to measure list and segment list pages
When a user creates a question based on a model, `source_card_id` was set to the model's ID. If that question was later converted to native SQL, `source_card_id` wouldn't get cleared. This means that deleting the model would cause the question to be deleted even though they no longer have any relationship to one another. The code fix won't help anyone whose data is already incorrect, so this adds a migration to correct the inconsistent data. I added an assertion to the t2 hook to catch this at dev time in the future. And ultimately, the delete cascade seems overkill, so there's a migration to replace that with SET NULL instead.
Treats 0 n_live_tup as NULL, due to confusion reported when auto analyze thresholds have not been met on a table. To justify: - You have a non-zero estimate (No change) - A zero estimate but actually have rows (We now hide the estimate) - A zero estimate and actually have zero rows (We now hide the estimate, unfortunately) This behaviour is considered on balance preferable to the behaviour now where users will likely be confused, even thought we explicitly label the field 'estimated'. Other options considered that we might explore in the future: - Forcing ANALYZE, e.g. on sync - Bounded/LIMIT count - Some policy based on last analyze timestamps - Better tooltips or explaination on frontend We should revisit in the future once we have a better idea for how estimates should work across drivers (as its only postgres for now).
Native sql validation is a bit too unreliable for general deployment at the moment, so we are disabling it for check_* endpoints and the broken entities list unless/until we can make it more reliable. Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
* feat: prevent duplicate glossary terms from being added * test: add unit spec
This is the permission we're correctly checking with the below `api/check-403` but by using `warehouses/get-database` (which, btw, has the MOST confusing possible arguments I've ever seen) we were throwing a 403 if we didn't have read access to the whole database (which checks *create-queries* perms on the entire database). We still need to exclude mirror databases. In the medium term there might be a better way to do this - maybe we could somehow have two different models, so `(t2/select :model/Database)` would always exclude mirror databases, while `(t2/select :model/DatabaseIncludingMirrorDatabase)` would return them? I dunno.
* initial work * fix test * fix another test * source entity malli schema * clean up and add test * add tests * clean up * fix tests * drop finding_details column * fix analysis version * Update enterprise/backend/src/metabase_enterprise/dependencies/models/analysis_finding.clj Co-authored-by: Timothy S. Dean <timothy.dean@metabase.com> * use transaction * use node-errors refactor suggestion * use maybe-resolve-join * add schemas * add comment --------- Co-authored-by: Timothy S. Dean <timothy.dean@metabase.com>
Co-authored-by: Joseph Adams <danielowenderefaka@gmail.com>
* Require free text for ui-bug and other feedback types Add Yup validation to MetabotFeedbackModal that requires free text feedback when issue type is ui-bug or other. Issue types requiring free text are maintained in a constant array for easy extension. * Address PR feedback: improve type safety and UX in feedback modal - Replace 'as any' with proper TypeScript type assertion - Fix type inconsistency between schema and initial value for issue_type - Add dynamic label showing '(required)' when ui-bug or other is selected Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * pr feedback --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Sloan Sparger <sloansparger@gmail.com>
update docs, pr template
* Fix missing label for cartesian * Add e2e tests * Add comment * Update Loki Snapshots --------- Co-authored-by: Metabase Automation <github-automation@metabase.com>
…67877) * adding segment editor to scalar * temp * adding tooltip * pretend i'm a clojure dev, disable tooltip when there are no segments * adding unit test and story * loki tests * silly me * I am a clojure god * adjustments * adding docstring, adjusting loki test * combining scalar loki tests * CardSegment -> ScalarSegment * BE update, filter unvalid segments
Update generated docs (master) Co-authored-by: github-actions <github-actions@github.com>
* feat: configure initial read-only mode for python transforms * refactor: hide UI elements in readonly mode * refactor: move database selector to the topbar * style: add in/out animations for bottom panel * refactor: move common library controls to the topbar * test: add unit specs for python transform editor sections * test: add e2e spec * fix: show correct run query button state after edits * refactor: general cleanup & test trimming * refactor: remove source controls * refactor: trim down test mocks * fix: pass missing `editable` flag to transform editor * fix: types * fix: e2e data source selector * refactor: use `isEditMode` instead of `readOnly`
Addresses #67704. When a UNIX timestamp is accessed through an implicit join, the coercion to DateTime wasn't being applied. The `allow-casting?` check requires `pos-int?` on `::source-table` to apply coercion. For implicit joins, `::source-table` is set to a string, so it was being skipped. Fix: in `add-alias-info/add-source-to-field-ref`, wet the flag `:qp/allow-coercion-for-columns-without-integer-qp.add.source-table`. I cribbed this approach from the SparkSQL driver, when it has to rewrite a numeric ID with a string.
* feat: add empty states for library items * test: add e2e specs * refactor: cleanup library section layout * test: compact e2e specs into 2 major flows
Previously, when running a transform job (e.g., daily), it would fail if a single transform in the run failed. This PR makes it so that the job continues to run transforms that don't depend on any failed transform. As previously, if any transform in the run fails, the job is marked as failed.
* feat: add library stats to anonymous daily stats ping * test: add unit spec for library stats * test: use the `mt/with-temp` macro
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
For those employed by Metabase: if you are merging into master, please add either a
backportor ano-backportlabelto this PR. You will not be able to merge until you do this step. Refer to the section Do I need to backport this
PR?
in the Metabase Branching Strategy document for more details. If you're not employed by Metabase, this section does not
apply to you, and the label will be taken care of by your reviewer.
Closes https://github.com/metabase/metabase/issues/[issue_number]
Description
Describe the overall approach and the problem being solved.
How to verify
Describe the steps to verify that the changes are working as expected.
Demo
Upload a demo video or before/after screenshots if sensible or remove the section
Checklist