mikebridge commented on PR #42469: URL: https://github.com/apache/superset/pull/42469#issuecomment-5116981509
Worked through the 12 open review threads — pushed `c65d09f94e`. Summary for anyone picking this up: **Taken (6 threads):** - @rusackas's three test nits, applied verbatim — the 403 paths now assert the entity was untouched, matching what the capture-disabled test already did. - Copilot's `entity_uuid` point: the engine's preloaded-entity shortcut now raises if `entity.uuid` and `entity_uuid` disagree, with a unit test. No live bug (the one caller loads by that UUID), but the failure mode — restoring one row while the audit trail names another — is worth a guard. - Copilot's logging point: `logger.exception` on the 422 path so the traceback survives. **Not taken (6 threads), reasoning in each:** - `datetime.now()` → UTC would be wrong here: `changed_on` is defined with `default=datetime.now, onupdate=datetime.now` in `models/helpers.py`, so UTC would leave one column holding two time bases depending on whether the last write was a save or a restore. Same change was proposed and reverted on #41549. - The `capture_enabled()` init-vs-request-time gap is real and already documented in that function's docstring, mitigation included. - `_RESTORE_RELATIONS` string keying is deliberate: it fails closed with a `LookupError` on an unregistered model, and keying on the class would reintroduce the model-import cycle this module avoids. 100 versioning unit tests green, pre-commit clean. The approval survived the push and I've kept the change surface to tests plus two small guards. I don't have merge rights on this repo — when CI comes back green, could a committer merge? This is the last gate on #41551 (version-history UI), which is otherwise ready and waiting on it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
