eschutho commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3554035547
##########
superset/datasets/api.py:
##########
@@ -489,17 +552,69 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(SqlaTable, pk)
+
try:
+ # Two commands, two commits, two Continuum transactions for an
+ # ``override_columns`` save — deliberately NOT merged into one
+ # transaction. A single-transaction design was attempted and
+ # reverted: ``DBEventLogger`` writes request logs through the
+ # SHARED scoped session and calls ``commit()`` /
+ # ``rollback()`` on it mid-request (superset/utils/log.py),
+ # so any save held uncommitted across a logged sub-action can
+ # be committed half-done (Postgres/MySQL) or rolled back
+ # entirely on a transient logger failure (SQLite's
+ # "database is locked"). Until the event logger gets its own
+ # session, per-command commit boundaries are the only shape
+ # whose failure modes are honest. Consequence the
+ # version-history UI must tolerate: one logical save can
+ # surface as two version transactions stamped the same second.
changed_model = UpdateDatasetCommand(pk, item,
override_columns).run()
+ # Capture the post-update identifiers BEFORE the refresh:
+ # RefreshDatasetCommand commits its own transaction, so reading
+ # afterwards would attribute the refresh's version to the
+ # user's update (and old→new would span two transactions).
+ new_info = current_entity_version_info(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ etag_version_uuid = new_info.version_uuid
if override_columns:
RefreshDatasetCommand(pk).run()
- response = self.response(200, id=changed_model.id, result=item)
+ # The ETag must reflect the entity's *current live* version,
+ # which after the refresh is the refresh's transaction —
+ # re-read it rather than reusing the pre-refresh uuid.
+ etag_version_uuid = current_entity_etag_uuid(
+ SqlaTable, changed_model.id, changed_model.uuid
+ )
+ response = self.response(
+ 200,
+ id=changed_model.id,
+ result=item,
+ old_version=old_info.version,
+ new_version=new_info.version,
+ old_transaction_id=old_info.transaction_id,
+ new_transaction_id=new_info.transaction_id,
+ old_version_uuid=old_info.version_uuid,
+ new_version_uuid=new_info.version_uuid,
+ )
+ set_version_etag(response, etag_version_uuid)
except DatasetNotFoundError:
response = self.response_404()
except DatasetForbiddenError:
response = self.response_403()
except DatasetInvalidError as ex:
response = self.response_422(message=ex.normalized_messages())
+ except DatasetRefreshFailedError as ex:
+ logger.error(
+ "Error refreshing dataset during update %s: %s",
+ self.__class__.__name__,
+ str(ex),
+ exc_info=True,
Review Comment:
if you use exception instead of error, you won't need the exc_info
--
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]
