EnxDev commented on code in PR #43583:
URL: https://github.com/apache/superset/pull/43583#discussion_r3880684643
##########
superset/datasets/api.py:
##########
@@ -626,6 +650,21 @@ def put(self, pk: int) -> Response:
# ``ENABLE_VERSIONING_CAPTURE`` is off).
old_info = current_entity_version_info(SqlaTable, pk)
+ try:
+ raise_for_stale_write(concurrency_token_from(old_info))
+ except StaleEntityError:
+ return set_version_etag(
+ self.response(
+ 412,
+ message=_(
+ "The dataset was changed by another user or browser
tab "
+ "after you opened it. Reopen it to pick up the latest "
+ "version, then reapply your changes."
+ ),
+ ),
+ concurrency_token_from(old_info),
+ )
+
Review Comment:
The version check and the write are not atomic. Two concurrent `PUT`
requests could both read the same `old_info.version_uuid`, pass the check, and
then overwrite each other. This would reintroduce the lost-update bug for
near-simultaneous requests.
Could we make this atomic using a conditional update such as `WHERE
version_uuid = :expected_version_uuid`, or by locking the row before performing
the check and update?
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -111,6 +113,23 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
const [isEditing, setIsEditing] = useState<boolean>(false);
const [modal, contextHolder] = Modal.useModal();
const [confirmModalOpen, setConfirmModalOpen] = useState(false);
+
+ useEffect(() => {
+ setVersionEtag(etag);
+ if (etag || !show || !datasource.id) {
+ return;
+ }
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/${datasource.id}`,
+ })
+ .then(({ response }) => {
+ setVersionEtag(response.headers.get('ETag') ?? undefined);
Review Comment:
Some callers outside `DatasetList`, including Explore’s `DatasourceControl`
and `ChangeDatasourceModal`, do not pass an `etag`. In particular, the modal
fetches it asynchronously after initializing the form. If the user saves before
that request completes, the update is sent without `If-Match` and remains
unguarded, allowing the same silent-overwrite issue through the Explore flow.
Could we ensure every update path requires the `etag`, or prevent saving
until it is available?
--
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]
