0be2972872
* Key cached session tokens on the app db
with-empty-h2-app-db! swaps in a different application database, and this cache was keyed on the username
alone. A token minted against the previous database names a session that does not exist in the new one, so
the request 401s, and the token minted inside leaks back out when the original is restored. The retry in
client-fn cleared the whole cache and tried again, which papered over it often enough to look like flake:
"Failed to authenticate X after three tries".
Key it on the app db as well, like every neighbouring cache does via memoize-for-application-db.
databases-list-can-upload-test and dismiss-spinner-test go from failing to passing; both were in the CI
failure list, the former across 12 jobs.
* Fix rollback-only app DB transaction semantics
* Reject rollback-only combinations that cannot roll back
- :nested-transaction-rule :ignore skips the savepoint :rollback-only needs, so the pair is rejected at any
depth rather than silently committing
- a throw out of the transaction body now rolls the connection back explicitly, since restoring autocommit
commits whatever is still open -- a requested rollback that failed was committing the work it had to discard
- the unsupported-options error names the options, and drops the "application database" framing that
misattributed failures on other java.sql.Connections
* Restore transaction state even when the savepoint rollback throws
The callback accumulators are discarded unconditionally at the top of rollback!, so leaving the state
restore behind a throwing .rollback left the tree half-unwound: an outer scope that caught the error and
committed would run its before-commit callbacks against the failed nested scope's state.
* Create test-data Databases outside the caller's transaction
Their ids are memoized for the whole app db, so the Database has to outlive whatever scope first asked for
one. Created inside a test's rollback-only with-temp it is rolled back while the memo keeps its id, and every
later lookup hands out an id for a row that never existed.
do-outside-transaction is the same detachment run-after-commit-callbacks! already did privately.
* Don't cache a personal collection id created inside a transaction
The cache's premise is that a Personal Collection cannot be deleted, so its id never changes. A rollback
breaks that: the collection goes away and the cached id points at a row that never existed, which reads as
a 403 on every later request that resolves the user's personal collection.
* Skip the test-database cache in a transaction rather than detaching
Detaching made the Database durable but took a second connection while the caller's transaction was still
open. When that transaction holds cluster lock rows -- data_permissions and search index setup both take
them -- the second connection waits on them until the lock acquisition times out.
Bypassing the cache keeps the work on the caller's connection, and matches how the personal collection id
cache handles the same problem. do-outside-transaction goes with it, having no other callers.
* Stop the outer commit when a requested rollback failed
The full rollback only runs when this call turned autocommit off, so for a nested scope or a caller-managed
connection a failed rollback! just propagated. Code between there and the outermost scope can catch it, and
the commit branch would then commit the writes the caller asked to discard.
Mark the tree instead, and refuse to commit it. Throwing rather than quietly rolling back: the caller asked
to commit and cannot, and dropping the outer work silently trades one bug for a worse one.
* Tolerate a savepoint an outermost rollback-only can no longer roll back to
A statement in the body can commit the transaction under us -- DDL does that implicitly on H2 and MySQL --
which invalidates the savepoint. Those writes are durable by then, so failing the scope reports a problem
nobody can act on while leaving the data exactly where it was.
At the outermost scope, discard whatever is still pending and log where it happened. Nested scopes still
poison the tree and throw, since there the enclosing work is pending and must not be committed.
* Note the option of rejecting DDL inside a savepoint-bearing transaction
* Never commit a transaction whose rollback failed
Four review passes flagged the same hole from different angles. Restoring autocommit commits whatever is
still pending, and after a rollback we were relying on to discard those writes, that is exactly the work
we were told to throw away. Track it and leave autocommit off instead, letting the pool roll the
connection back on check-in.
Two narrower routes to the same place:
- rollback! now marks the tree itself, so an exception-driven rollback that fails poisons it too. Before,
only a requested rollback did, and the error path could still reach a commit.
- the mark is rechecked after before-commit callbacks drain. A callback can catch a nested scope whose
rollback failed, which lands after the earlier check has already passed.
Two tests changed meaning as a result: a nested scope whose rollback throws now stops the outer scope
committing, so it no longer completes and its own after-commit callbacks no longer run. That is the point
-- its writes are still there.
Also adds the regression test for personal collection id caching, models autocommit honestly in the mock
rather than pinning it true, and records what the in-transaction test-Database lookup gives up.
* Build the test-data Database before with-temp opens its transaction
Created inside a rollback-only scope it never survives, so it is rebuilt for every test, and the ids
memoized in metabase.test.data.impl outlive it. Cards then get inserted against table ids for rows that no
longer exist: FK_REPORT_CARD_REF_TABLE_ID on a fresh app db.
Materialise it before the transaction opens, where it can commit. The table and field lookup maps get the
same in-transaction bypass the Database id already had, for the case where a dataset is first asked for
inside a scope anyway -- neither is enough on its own.
Reproduce with a throwaway app db, which is what CI has and a local one does not:
MB_DB_TYPE=h2 MB_DB_FILE=/tmp/fresh-$$ ./bin/test-agent :only '[metabase.queries.models.card-test]'
* Only pre-warm the dataset for the default driver
db-id resolves through the driver under test, so in a driver-test namespace the pre-warm reached for that
warehouse's dataset -- work a with-temp there may never need, against a warehouse that may not be up.
Driver failures went from 2 to 12 in the push that added it.
The FK problem it fixes is on the app-db side, for the default dataset, so confine it there. Best effort
even then: a dataset that cannot be built must not take an unrelated with-temp down with it.
* Evict temporary users from personal collection cache
* Evict the personal collection cache on the rollback, not on the user
Clearing the temp User's own entry misses the case that actually bites: a committed user's Personal
Collection is created lazily on its first permission check, which often lands inside somebody else's
with-temp. Nothing evicts it when that scope rolls back, and every later request for that user 403s.
Key off the with-temp boundary instead, and drop whatever it may have cached. Costs a re-lookup per user
afterwards; correct whichever user and whichever scope created the entry.
agent-api + activity-feed went from 21 failures to none.
* Skip the dataset pre-warm where the app db is deliberately empty
with-empty-h2-app-db! rebinds the app db, and the memo key includes its identifier, so the pre-warm missed
the cache and created a test-data Database inside a scope whose whole contract is emptiness.
databases-list-can-upload-test snapshots an empty Database set and saw two.
Bind *skip-dataset-prewarm?* there instead. Also drops the earlier :h2 restriction, which came from a bad
reading of two CI runs sampled at different completion stages: it could never have helped here, since those
helpers are H2, and it took the pre-warm away from driver tests that needed it.
Reproduce with a throwaway app db, which is what CI has and a local one does not:
MB_DB_TYPE=h2 ./bin/test-agent :only '[metabase.queries.models.card-test]'
* Register the cache eviction with a unique key
metabase.test.redefs already holds `:around :default` on do-with-temp*, and methodical overwrites methods
sharing a key rather than composing them. Whichever namespace loaded last would win, silently dropping
either this eviction or the rollback-only transaction wrapper. It happened to work; it should not depend on
load order.
Also notes what a failed connection-level discard leaves behind. Only a caller-supplied connection is
exposed: on one we own, the same flag stops autocommit being restored, so the writes are never committed.
It logs rather than throws because throwing from that catch would mask the exception being unwound, and
master never attempted the discard at all, so a caller-supplied connection is no worse off than before.
Signalling it properly would mean telling the caller some other way.
* Note that session tokens are cached without regard for transactions
The user ids directly above skip the memo inside a transaction, since users created in a `with-temp` are
discarded with it. The token cache has no equivalent guard, so a session rolled back with its scope leaves a
token behind and the request fails with "Failed to authenticate X after three tries". The retry in
`client-fn` hides it more often than not.
* Note that a rollback can leave a stale personal collection id cached
"A Personal Collection cannot be deleted" does not hold for a transaction that rolls back. Tests evict on
the with-temp boundary; production has no equivalent and keeps the id for the rest of the TTL. Accepting
that for now -- it predates honoring :rollback-only, and closing it properly wants an after-rollback hook
alongside do-before-commit/do-after-commit.
* Follow the created-globally rename in the token cache TODO
* Build the query for export inside the guard, not beside it
`:query_json` already returns nil for a card whose query cannot be exported, but `lib/query` was called in the
argument position, outside `try-export-query`. A stored `dataset_query` with no stages builds a query that fails
its own output schema, so one such Card takes down every response that walks a set of Cards -- the typed-schemas
endpoint among them.
metabase.search.appdb.scoring-test alongside metabase.typed-schemas-rest.api-test goes from 13 failures to 0.
* Don't run a test request inside a rollback-only transaction
`user-request` takes a temporary Session when it is handed a User or an id rather than a `:rasta`-style keyword,
and the handler runs in-process on this thread. Now that the with-temp rolls back for real, every app db write
the request made is discarded when it returns -- the warehouse table writes survive on their own connection and
the API still answers 200, so the test sees a successful call that changed nothing.
Insert the Session and delete it in a `finally` instead. A caller already inside a transaction still gets the
same behaviour it always had; what goes away is the extra rollback scope around the request itself.
metabase-enterprise.action-v2.models.undo-test goes from 25 failures to 0, passing all 134 assertions, which
matches master.
* Mint the test users' sessions before anything opens a transaction
`authenticate!` ran about 150 times in a single `pulse.api.pulse-test` run, nearly always inside a transaction.
A session minted inside a `with-temp` is rolled back with the scope, the next request 401s, and the retry in
`client-fn` cleared the whole cache -- every user's token, not just the dead one -- so everyone re-authenticated
in-transaction and the cycle repeated. Each of those inserts bumps the user's `last_login` through the
`:model/Session` after-insert hook, which is what breaks a test that reads the creator before making its request.
Mint the four test users' sessions at initialization, where nothing has opened a transaction yet, so they are
durable. On a 401 forget only that user's token, so one temporary user's rolled-back session cannot evict them.
pulse.api.pulse-test goes from one failure to none, matching master. publish-refresh-error-event-sends-admin-email-test
passes too, where it fails standalone on master: that email only goes to admins with a `last_login`, and
crowberto's was rolled back with every login.
* Ask for the FieldValues the chain filter test needs
The full FieldValues row for `categories.name` is created lazily, and `update-field-values-for-table!` only
refreshes rows that already exist -- so a run that creates it inside a `with-temp` discards it and nothing
makes another. `select-one-pk` then returns nil and the test dies on "Not something with an ID".
Same shape as the rest of this branch: a durable row created on demand, now rolled back with the scope that
created it. Ask for the row instead of assuming an earlier test left one behind.
* Run the metric leaf queries one at a time inside a transaction
A transaction owns a single connection and `future` hands that binding to every thread it starts, so the
leaves' app db writes -- used-card stats behind a cluster lock, query rows -- interleave on one session.
Savepoints are per session: a thread rolling back to its own destroys the ones its siblings are holding, the
next release fails with "savepoint does not exist", and the transaction aborts, taking the rest of the test
with it. That is why api-arithmetic-test loses a different test on each CI run.
Only tests get here. A request runs in no transaction, where each future takes its own pooled connection and
the parallelism is worth having.
On a throwaway Postgres the namespace errors on two tests per run before this and passes three runs out of
three after.
* Commit a lock row that would otherwise roll back with the caller
A lock row is written once and read forever after, and acquisition leans on that: rival first-time inserters
block on the winner's uncommitted unique-index entry and retry once it commits. Inside a `with-temp` that
commit never comes, so the row is gone again and the next acquisition replays the race -- on MariaDB until the
lock-wait timeout expires.
Insert it on a connection of our own when the caller was already in a transaction, which is the only case
where the row would not survive. Nothing in production takes a cluster lock from inside another transaction,
so this leaves that path alone -- worth keeping that way, since lock names are dynamic in places and rows that
used to roll back with a failed transaction would otherwise persist.
Reasoning rather than measurement; the TODO on the function says so and says what to do if it does not help.
* Bound the out-of-band insert on its own connection
`set-query-timeout!` returns false on MySQL running MariaDB Connector/J, and the fallback sets
`innodb_lock_wait_timeout` on the session -- but `acquire-lock-row!` sets it on the caller's connection, not
the one the insert opens. Blocked by a rival's uncommitted row, the insert would then wait out the server
default instead of the second it was given, on exactly the databases this is meant to help.
Give it the same treatment on its own connection. Also note in the docstring that the second connection is
only taken until the row exists, which is what bounds the cost of holding two at once.
* Resolve the venues id before counting queries
The cache is fine; the query the test counts is its own `(mt/id :venues)`. Inside a transaction the test-data
id lookup deliberately skips its memo, so the call lands in the window that is supposed to see none.
* Keep the pre-warm out of app dbs that are meant to be empty
`with-empty-h2-app-db!` already opts out of the dataset pre-warm; the serialization test dbs and the
schema-migration ones never did. A `with-temp` inside them materialised the test-data Database, and an extract
then saw a Database no assertion expects -- serialization.v2.load-test/database-test wanted one and found two.
* Do not insert a lock row out of band outside tests
The claim in "Commit a lock row that would otherwise roll back with the caller" was wrong: production does take
cluster locks inside another transaction. `load-from-h2!` inserts Databases, and so takes the batch-permissions
locks, inside one big load transaction. Our fresh connection then blocked on a next-key gap lock the caller's
own SELECT ... FOR UPDATE was holding, and a thread cannot outwait itself -- it timed out, retried, and failed.
That is what broke dump-to-h2, load-from-h2 and rotate-encryption-key on MariaDB.
Gate the out-of-band insert on `config/is-test?`, which is the only place a transaction may never commit, so
production goes back to exactly what it did before. Keep a fallback for the same shape inside tests, where
`load-from-h2!` can block on itself just as readily: on a lock-wait timeout, insert on the caller's connection.
* Stop three card tests leaving rows behind
`updating-model-query-does-not-shift-metadata-overrides-test` had no cleanup at all, and
`create-card-with-type-and-dataset-test` opened a plain `t2/with-transaction` where its siblings pass
`{:rollback-only true}` -- so both committed cards with a `{}` dataset_query that every later test walking the
card table then had to survive. That leak is older than this branch; honoring the rollback is what makes the
second fix work.
`get-series-for-card-permission-test` read crowberto's personal collection with `select-one-pk`, and a nil
collection_id puts the card in the root collection, which rasta can read -- so the test quietly asserted 403
against a card that was never private. Ask for the collection instead.
* Truncate the heartbeat fixture times to milliseconds
Postgres stores microseconds, so a timestamp ending .7369995 comes back a millisecond above its own
millis-truncation and the assertion misses by one.
* Tolerate a card whose query will not parse when finding related entities
Nothing at the schema level stops a stored `dataset_query` being empty, and such a card simply has no
context-bearing forms. Without the guard one of them takes down the related-entities computation, and with it
every x-ray, for everyone else. Same treatment `card-details` already gives an unexportable query.
* Give the end-to-end schema test a Database of its own
The test asserts exactly which questions, metrics and models fall inside the test-data Database's scope, which
holds only as long as no other namespace commits a Card against that Database. Two such leaks have been fixed
on this branch already and there is at least one more; the assertion cannot be made safe by chasing them,
because any test added later can break it again.
Run it against a copy of the Database instead, so nothing else can write there.
A throwaway namespace that commits one Card on the test-data Database ahead of it reproduces the CI failure
exactly (core_test.clj:147) and passes with this change.
* Recognise the MySQL lock-wait timeout in the in-band fallback
`lock-wait-timed-out?` knew about `SQLTimeoutException` and our own `::acquisition-timeout` marker, but not
ER_LOCK_WAIT_TIMEOUT itself. The server raises that on its own schedule, so on MySQL and MariaDB -- the
databases the fallback exists for -- it could arrive as a plain `SQLException` and the fallback would not fire.
Check the code through the cause chain.
* Take the revisions with the cards
`with-model-cleanup` deletes rows directly, which skips the Card hook that would have removed the revisions
this test's POST and two PUTs create. Name Revision in the cleanup as well.
* Truncate once, and align the chain filter's temp values
The second `truncate-to` could not do anything the first had not.
* Create the test users before the related tests race for them
`user->id` skips its memo inside a transaction and creates the users there instead, so on this branch they roll
back with the scope and the next test makes them again. These tests are parallel and every one of them takes a
`with-temp` Card whose creator defaults to rasta, so several threads insert the same user at once and all but
one time out on the row lock. The namespace goes from three errors to none.
Also covers the guard in `related/definition`: a Card whose stored query will not parse contributes no
context-bearing forms, and a genuinely similar Card still outranks it.
* Cover the card whose query will not build
The guard in `card-details` had no test: a Card with a stage-less stored query answers without `:query_json`,
and the Card beside it in the same response is unaffected.
* Cover the batch path and the lock-wait code
The first version of the card test called `get-table-details` once per card, which cannot see the failure it
claims to cover -- the bug was that one bad card killed the whole `cards-details` seq the others were in. Go
through that seq instead. Removing the guard turns the test red at the same malli output error CI was showing,
which the per-card version did not.
Also cover `lock-wait-timed-out?`: MySQL's ER_LOCK_WAIT_TIMEOUT on its own and inside a cause chain, and any
other error still propagating rather than being taken for a timeout.
* Create the temp search index before the transaction, not inside it
`with-temp-index-table` runs CREATE TABLE and DROP TABLE on the ambient connection, and on H2 and MySQL that
commits the transaction around it. Six tests opened it inside a `with-temp`, so the rollback had nothing left
to take back and every row they created leaked -- a Transform, some Cards and Collections, a published Table, a
Document -- to any later test that walks the app db.
That one leaked Transform is the whole of the serialization failures: a collection-scoped export includes every
Transform in the instance, so all four tests came back with one entity more than they expected.
Take the index table before the `with-temp` instead. The nested scope reuses it, so nothing else moves.
`metabase-enterprise.search.scoring-test` also had no fixtures at all, and fails on its own: the first lazy
`initialize :db` fires mid-test and recreates the H2 database underneath a test already writing index
bookkeeping.
The 133 namespaces that run before serialization.api-test in the Enterprise job reproduce the CI failures
exactly -- 5011 assertions, 5 failures -- and pass with this, at 4714 assertions.
* Say the cluster lock comments plainly
Lead with the point rather than building up to it, and drop the clause chains. No behaviour change.
The TODO above `clear-cached-session-tokens!` was stale: it asked for the `created-globally` treatment, which
landed earlier on this branch. Point at what is actually left instead -- initialization can still first happen
inside a `with-temp`, and then the sessions roll back with it.
* Keep the non-index assertions where the app db cannot index
`with-temp-index-table` is a `when`, so wrapping a whole test in it skips the whole test on MySQL and MariaDB.
Two of the tests moved in the last commit also assert things that need no index, and those assertions went
quiet there. Add a sibling that runs the body either way, and use it for those two.
`search.appdb.scoring-test` had no initialization fixture, the same gap its enterprise counterpart had: the
first lazy `initialize` can recreate the H2 db underneath a test already writing index bookkeeping.
* Sort the fixtures require
* Download through the client that retries a 401
`downloaded-card-filenames-test` passed a raw cached token straight to `client-full-response`, so it skipped
the retry every other test gets from `client-fn`. A session rolled back with the scope that made it then came
back here as a bare 401, which is what the BigQuery job has been failing on.
`user-http-request-full-response` still returns the headers the test reads.
* Improve rollback test isolation and comments
* Do not poison the transaction tree when the savepoint is already gone
MySQL and MariaDB roll back the whole transaction to break a deadlock, discarding every savepoint with it. The
ROLLBACK TO SAVEPOINT that follows fails with "SAVEPOINT does not exist", and marking the tree rollback-required
there is backwards: the server has already discarded the writes, so nothing is pending for an enclosing scope to
commit by mistake. A rollback-only scope whose DDL committed implicitly already gets this treatment.
The refusal message named a rollback-only transaction, but the ordinary error path sets the flag too.
Also validate :nested-transaction-rule with a throw. It is a public option, and `assert` compiles away under
`*assert*` false. Sorting the unsupported keys keeps the message stable across runs.
* Keep the test request's session out of the login hooks
:model/Session's after-insert hook publishes :event/user-login, which stamps User.last_login, and
:event/user-joined for a User that has never logged in. Deleting the Session afterwards undoes neither, so every
user-http-request left both behind for whatever ran next.
Write the row instead. created_at has to come along, since the model supplied it.
* Ask whether the app db can hold the index, not whether any engine can
supports-index? is true for any active engine. With semantic search on MySQL or MariaDB, the macro that exists to
run the body without an app-db index took the app-db branch and tried to create the table anyway.
* Say something when a stored query is dropped
Both guards swallowed every exception silently, so a Card whose query failed to build for an unrelated reason
looked exactly like one that has no query. try-export-query, right next to the Metabot guard, already warns.
Related entities logs at debug: it runs for every candidate Card while ranking.
* Commit the out-of-band lock row rather than assume autocommit
The docstring said the row was committed, but nothing committed it -- the insert relied on the pooled connection
arriving in autocommit, which the pool does not promise. The caller's SELECT cannot find the row otherwise.
* Let a failed dataset pre-warm be heard
Now that the pre-warm runs for every driver, a warehouse that is down fails it on every top-level with-temp. At
debug that is invisible on a normal run and resurfaces much later as a Database that does not exist. Warn once
per message.
Also note that mt/id costs a query inside a transaction, which is what a with-call-count window has to plan for.
* Polish prewarm comments
* Mark the tree rollback-required even when the savepoint vanished
A vanished savepoint does not mean nothing is pending. DDL committing implicitly, or a server breaking a deadlock,
ends the transaction and discards the writes so far -- but anything written afterwards sits in a fresh transaction
and is still pending. Skipping the flag there let an enclosing scope swallow the rollback error and commit exactly
those writes.
Say so at the point of the catch, since the reasoning does not survive a quick read.
* Gate every app-db index helper on app-db support
with-temp-index-table and with-appdb-search-if-available-otherwise-legacy still asked supports-index?, which is
true for any active engine. With semantic search on MySQL or MariaDB they ran app-db DDL the app DB cannot take,
including from scopes nested inside the fallback body that exists to avoid exactly that.
Cover the borrowed session too: it must leave last_login empty, and delete itself afterwards.
* Cover the out-of-band lock row against a connection in a transaction
Hands the dedicated connection over with autocommit off and checks the row survives the caller's rollback. Without
the explicit commit the insert dies with the connection, and acquisition then fails on the row it just wrote.
Runs on MySQL and PostgreSQL only; H2 takes an in-process lock and writes no row.
* Pin the vanished-savepoint case against the outer commit
A savepoint the server already discarded says nothing about what was written after it. The existing rollback tests
throw generic exceptions, so nothing stopped the SQLState 3B001 / MySQL 1305 exemption from coming back.
* Pin the engine in the otherwise-legacy fallback
Where the app DB cannot hold an index, the fallback took whatever default engine was active. Semantic can be
active there, and the bodies are written for an index the app DB does not have. Run them under legacy search, as
the macro's name says. with-legacy-search moves above its first use.
* Polish rollback and search comments
* Stop two remapping scopes deadlocking on MySQL and MariaDB
A remapping writes a Dimension for a Field the whole suite shares, and holds it until its scope ends. Two threads
doing that at once deadlock on the unique index over dimension.field_id: each holds the record lock the other's
uniqueness check wants, while asking for the insert-intention gap in front of it. MySQL and MariaDB pick a victim
and roll it back, which drops its savepoints, so the rollback that follows fails on a savepoint that is gone.
Serialise remapping scopes against each other. Tests that never remap keep running in parallel alongside them.
The helper already carried a TODO saying it was not thread-safe.
Reproduced on MariaDB 12.3 and confirmed on master, so this predates the branch. custom-values-test failed nearly
every run before, and three runs in a row are clean after. Verified on H2 as well.
* Serialise every temporary Dimension, not just remapping scopes
Remapping is not the only way a test writes a Dimension for a shared Field: card-test, card/metadata-test,
field-values-test and data-studio's table-test all create them directly, in parallel, on the Orders and Products
fields. Those still raced with the remapping scopes.
Move the lock to `do-with-temp*` for :model/Dimension so every site is covered, including ones added later.
`locking` is reentrant, so a `with-temp` holding several Dimensions still works.
* Take the search index out of six more with-temp transactions
Same fix as the earlier pass over the search tests, for the ones that pass a warehouse driver: their index scope
sits inside `with-temp`, so `drop-table!` runs DDL on the ambient connection. H2 and MySQL commit the transaction
to do it, which makes the temporary Transforms durable and destroys the savepoint. The rollback that follows can
no longer take them back, and it discards toucan's compensating deletes along with everything else still pending,
so the Transforms outlive the test.
Three tests later in the same namespace count transforms and saw the strays: root-collection-items-include-
transforms-test expected one and found four.
These were missed the first time because `mt/normal-drivers-with-feature :transforms/table` skips them on H2, so
only a driver run reaches them.
Reproduced with a MariaDB warehouse: transform-test went 3 failures to 0, and the four namespaces that failed
together now match master at 2532 assertions. H2 is unchanged at 109.
* Let the failed-rollback warning reach the console
The shared Console appender only accepts FATAL, so the warning naming a rollback-only scope that could not roll
back went to logs/test-log.json and nowhere a CI run gets read. It took hours to find six tests leaking rows
because of it; the warning had been firing the whole time.
Give that logger an appender that accepts WARN. The message carries the mb-test key, so it names the test that
broke its own isolation rather than the later one that trips over the rows.
* Hoist the search index scope above with-temp in two document search tests
Both tests opened `with-appdb-search-and-legacy-search` inside `mt/with-temp`.
That scope creates and drops the index table on the ambient connection, and on H2
and MySQL the DDL commits the transaction `with-temp` meant to roll back, so the
temp Documents leaked into later tests.
Found by the throw this branch adds: both showed up as errors at connection.clj:293
in the app DB test jobs.
* Serialize the reduced-fields card test
It holds a rollback-only savepoint across two `with-temp` Cards and an API call. Run in parallel, something else
commits the transaction that savepoint lives in, so both Cards are discarded and the downstream query reports its
source Card as missing.
Only MySQL surfaces it, and only under a full suite: the test passes on its own, and six local configurations could
not reproduce it. Confirmed by running the MySQL and MariaDB driver jobs with this single change -- all four green,
against four runs where they failed.
* Materialize the test users' Personal Collections before with-temp opens
`user->personal-collection` is a get-or-create and sits on the permissions path every API request takes. A
namespace that does not initialize `:test-users-personal-collections` creates Lucky's the first time a request
calculates permissions -- which happens inside a `with-temp` -- so the transaction rolls it back. Every later
request tries to create it again, and the parallel ones time out on each other's COLLECTION table lock:
GET /api/database expected a status code of 200, got 500.
Timeout trying to lock table; INSERT INTO COLLECTION (..., "Lucky Pigeon's Personal Collection", ...)
That is what failed `databases-list-include-saved-questions-test` on the Mongo, Redshift and Athena jobs -- the
empty database list was the 500, not a visibility problem.
Prewarm them next to the dataset, under the same skip flag, so helpers that need an empty app DB are unaffected.
This also closes the related hole where `:test-users` itself first initializes inside `with-temp`, since that
component initializes it.
Confirmed on the Mongo driver jobs: MongoDB Latest went from failing to green.
* Update the test-user session note now that initialization is prewarmed
77 lines
4.3 KiB
Clojure
77 lines
4.3 KiB
Clojure
(ns metabase-enterprise.sandbox.api.util-test
|
|
(:require
|
|
[clojure.test :refer :all]
|
|
[metabase-enterprise.sandbox.api.util :as sandbox.api.util]
|
|
[metabase-enterprise.test :as met]
|
|
[metabase.permissions.models.data-permissions :as data-perms]
|
|
[metabase.test :as mt]
|
|
[metabase.util :as u]
|
|
[toucan2.core :as t2]))
|
|
|
|
(deftest sandbox-caching-test
|
|
(testing "`sandboxed-user?` and `enforced-sandboxes-for-tables` use the cache"
|
|
(mt/with-temp [:model/User user {}]
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
;; retrieve the cache now (and realize its values) so it doesn't get included in call count
|
|
(doall @data-perms/*sandboxes-for-user*)
|
|
;; make the cache wrong
|
|
(t2/delete! :model/Sandbox :group_id (:id &group))
|
|
;; Resolve this outside the call-count window. Within a `with-temp` transaction, test-data ID lookups
|
|
;; intentionally bypass their cache, so `(mt/id :venues)` executes another query.
|
|
(let [venues-id (mt/id :venues)]
|
|
;; Subsequent calls should use the cache without querying the DB.
|
|
(t2/with-call-count [call-count]
|
|
(is (sandbox.api.util/sandboxed-user?))
|
|
(is (zero? (call-count)))
|
|
(is (= 1 (count (sandbox.api.util/enforced-sandboxes-for-tables [venues-id]))))
|
|
(is (zero? (call-count)))))))))
|
|
|
|
(deftest enforce-sandbox?-test
|
|
(testing "If a user is in a single group with a sandbox, the sandbox should be enforced"
|
|
(mt/with-temp [:model/User user {}]
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(is (sandbox.api.util/sandboxed-user?)))))
|
|
(testing "If a user is in another group with view data access, the sandbox should not be enforced"
|
|
(mt/with-temp [:model/User user {}]
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(mt/with-full-data-perms-for-all-users!
|
|
(is (not (sandbox.api.util/sandboxed-user?)))))))
|
|
(testing "If a user is in another group with another sandbox defined on the table, the user should be considered sandboxed"
|
|
;; This (conflicting sandboxes) is an invalid state for the QP but `enforce-sandbox?` should return true in order
|
|
;; to fail closed
|
|
(mt/with-temp [:model/User user {}]
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(is (sandbox.api.util/sandboxed-user?))))))
|
|
(testing "If a user is in another group with an impersonation policy defined on the table, the user should be considered sandboxed"
|
|
;; Similar to above, this is also an unsupported configuration for querying, but we want to treat this user as
|
|
;; sandboxed
|
|
(mt/with-temp [:model/User {user-id :id} {}
|
|
:model/PermissionsGroup {group-id :id} {}
|
|
:model/PermissionsGroupMembership _ {:user_id user-id
|
|
:group_id group-id}]
|
|
(met/with-gtaps-for-user! user-id {:gtaps {:venues {}}}
|
|
(mt/with-temp [:model/ConnectionImpersonation _ {:db_id (mt/id)
|
|
:group_id group-id
|
|
:attribute "test-attribute"}]
|
|
(is (sandbox.api.util/sandboxed-user?))))))
|
|
(testing "If a user is in two groups with conflicting sandboxes, *and* a third group that grants full access to the table,
|
|
neither sandbox is enforced"
|
|
(mt/with-temp [:model/User user {}]
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(met/with-gtaps-for-user! (u/the-id user) {:gtaps {:venues {}}}
|
|
(mt/with-full-data-perms-for-all-users!
|
|
(is (not (sandbox.api.util/sandboxed-user?)))))))))
|
|
|
|
(defn- has-segmented-perms-when-segmented-db-exists?! [user-kw]
|
|
(testing "User is sandboxed when they are not in any other groups that provide unrestricted access"
|
|
(met/with-gtaps-for-user! user-kw {:gtaps {:venues {}}}
|
|
(sandbox.api.util/sandboxed-user?))))
|
|
|
|
(deftest never-segment-admins-test
|
|
(testing "Admins should not be classified as segmented users -- enterprise #147"
|
|
(testing "Non-admin"
|
|
(is (has-segmented-perms-when-segmented-db-exists?! :rasta)))
|
|
(testing "Admin"
|
|
(is (not (has-segmented-perms-when-segmented-db-exists?! :crowberto))))))
|