Copilot commented on code in PR #38347:
URL: https://github.com/apache/superset/pull/38347#discussion_r2885563907
##########
superset/dashboards/api.py:
##########
@@ -729,6 +733,7 @@ def post(self) -> Response:
return self.response_400(message=error.messages)
try:
new_model = CreateDashboardCommand(item).run()
+ add_extra_log_payload(dashboard_id=new_model.id)
return self.response(201, id=new_model.id, result=item)
Review Comment:
Audit-log ID population is now added for `DashboardRestApi.post`, but there
doesn’t appear to be an integration test that asserts the persisted
`Log.dashboard_id` is set for the create flow (similar to the new update/delete
assertions). Adding a DB-level assertion after a dashboard create would help
prevent regressions specifically in the REST API path.
##########
tests/integration_tests/event_logger_tests.py:
##########
@@ -254,3 +254,117 @@ def test_func(arg1, add_extra_log_payload, karg1=1):
}
]
assert payload["duration_ms"] >= 100
+
+
+class TestMutationEndpointAuditIds(unittest.TestCase):
+ """
+ Regression tests for GitHub issue #38187.
+
+ Verifies that dashboard_id and slice_id are populated in the audit log
+ when REST API mutation endpoints are called (previously they were NULL).
+ """
+
+ # ------------------------------------------------------------------ #
+ # Helper: build a log_this_with_context wrapper with allow_extra_payload
+ # ------------------------------------------------------------------ #
+ def _make_wrapped(self, logger: DBEventLogger, action: str, id_kwarg: str):
+ """
+ Return a function wrapped with
log_this_with_context(allow_extra_payload=True)
+ that calls add_extra_log_payload(**{id_kwarg: 42}) — simulating the
pattern
+ used by the fixed mutation endpoints.
+ """
+
+ @logger.log_this_with_context(
+ action=action,
+ log_to_statsd=False,
+ allow_extra_payload=True,
+ )
+ def endpoint(add_extra_log_payload=lambda **kw: None):
+ add_extra_log_payload(**{id_kwarg: 42})
+ return "ok"
+
+ return endpoint
+
+ # ------------------------------------------------------------------ #
+ # DashboardRestApi tests
+ # ------------------------------------------------------------------ #
+
+ @patch.object(DBEventLogger, "log")
+ def test_dashboard_put_logs_dashboard_id(self, mock_log):
+ """DashboardRestApi.put must populate dashboard_id in log."""
+ logger = DBEventLogger()
+ endpoint = self._make_wrapped(logger, "DashboardRestApi.put",
"dashboard_id")
+
+ with app.test_request_context("/api/v1/dashboard/42"):
+ endpoint() # pylint: disable=no-value-for-parameter
+
+ assert mock_log.called
+ assert mock_log.call_args[1]["dashboard_id"] == 42
+
Review Comment:
The new `TestMutationEndpointAuditIds` tests don’t actually exercise the
DashboardRestApi/ChartRestApi mutation endpoints (they wrap a local `endpoint`
function instead). As written, they largely re-test the existing
`test_log_this_with_context_and_extra_payload` behavior rather than verifying
the new API changes. Consider either calling the real API endpoints here (or
adjusting the PR description) so these tests directly validate the regression
in #38187.
##########
superset/charts/api.py:
##########
@@ -363,6 +367,7 @@ def post(self) -> Response:
return self.response_400(message=error.messages)
try:
new_model = CreateChartCommand(item).run()
+ add_extra_log_payload(slice_id=new_model.id)
return self.response(201, id=new_model.id, result=item)
except DashboardsForbiddenError as ex:
Review Comment:
Audit-log ID population is now added for `ChartRestApi.post`, but there
doesn’t seem to be an integration test that verifies the `logs.slice_id` column
is set after creating a chart (parallel to the new update/delete assertions).
Adding a DB-level assertion on the latest `Log` row for `ChartRestApi.post`
would make this regression-proof.
--
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]
