Enforce Editor Lock for Write, Append, and Upload Operations in DataNode #2122

PrakharJain1509 · 2024-10-21T18:46:07Z

PR Description

Title: Enforce Editor Lock for Write, Append, and Upload Operations in DataNode

Summary

This PR addresses the issue #2017 where a DataNode could be modified by other editors despite being locked by one. It ensures that once a DataNode is locked with an editor_id, no other editor can perform write, append, or upload operations until the lock is released.

Changes Implemented

Modified the write(), append(), and _upload() methods:
- Enforced locking mechanism to prevent unauthorized modifications by other editors.
- Raised DataNodeIsBeingEdited exception when a different editor attempts to modify the locked DataNode.
- Allowed orchestrator writes without editor_id even during an active lock.
Updated unit tests in:
- test_data_node.py to validate the new behavior.
Ensured 90% code coverage on all modified methods.

Acceptance Criteria Met

Locking mechanism for write, append, and upload is now enforced as expected.
Orchestrator writes remain allowed without editor_id.
All relevant tests pass with the required code coverage.

This PR ensures consistent locking behavior across all relevant operations in line with the documentation and expected usage.

PrakharJain1509 · 2024-10-21T18:47:11Z

Hey, @FlorianJacta and @bobbyshermi
I have created this PR addressing all the changes asked, Please review it and let me know if any changes are required.

bobbyshermi · 2024-10-22T09:00:20Z

taipy/core/data/in_memory.py

+        if self.id not in in_memory_storage:
+            in_memory_storage[self.id] = []
+
+        in_memory_storage[self.id].append(data)  # Append new data


What if the previous data of the data node has no append method.
In other words, what if in_memory_storage[self.id] has no append method?

Ok I added a if instance condition to resolve that. Please review it and let me know if any changes required.

PrakharJain1509 · 2024-10-24T16:58:06Z

@bobbyshermi Did you review it sir?

PrakharJain1509 · 2024-10-24T16:59:04Z

@bobbyshermi @FlorianJacta @jrobinAV
Please let me know if any changes are required?

PrakharJain1509 · 2024-10-25T19:38:26Z

@bobbyshermi @jrobinAV @FlorianJacta Please inform me if this issue is open or closed?

PrakharJain1509 · 2024-10-26T21:19:28Z

@bobbyshermi @jrobinAV @FlorianJacta Please inform me if this issue is open or closed?
@bobbyshermi @jrobinAV @FlorianJacta
???

jrobinAV · 2024-10-31T07:53:32Z

@PrakharJain1509 The issue is still open, yes, and your PR is in the review process. Thank you for your patience. We will get back to you very soon.

toan-quach

Thanks for your contribution! The changes look good to me, but tests are failing. Please help take a look and fix it 😃

jrobinAV

This is a high-quality PR. Thank you very much.

Please have a look at the various comments.

Can you also create a new test file, tests/core/data/test_file_datanode_mixin.py, and test the upload changes you made? The test functions should look like the ones you already created for the write and append functions.

Thx,

jrobinAV · 2024-10-31T08:16:01Z

taipy/core/data/data_node.py

@@ -442,9 +444,16 @@ def write(self, data, job_id: Optional[JobId] = None, **kwargs: Dict[str, Any]):
        """
        from ._data_manager_factory import _DataManagerFactory

+        if self.edit_in_progress and editor_id and self.editor_id != editor_id:


If I understand correctly, this covers the case where the data node is already locked by a different editor than the one attempting to write it. We also need to check that the self.editor_expiration_date (if it exists) is not past.

jrobinAV · 2024-10-31T08:23:47Z

tests/core/data/test_data_node.py

@@ -745,3 +745,33 @@ def test_change_data_node_name(self):
        # This new syntax will be the only one allowed: https://github.com/Avaiga/taipy-core/issues/806
        dn.properties["name"] = "baz"
        assert dn.name == "baz"
+


I believe we are missing a few test cases here:

An editor editor_id_1 locked the data node, but the expiration date passed. Another "manual" editor, editor_id_2, should succeed in writing the data node.

An editor editor_id_1 locked the data node, but the expiration date passed. Another "manual" editor, editor_id_2, should succeed in appending the data node.

The orchestrator locks the data node. A "manual" editor should fail to write the data node.

The orchestrator locks the data node. The Orchestrator should succeed in writing the data node.
The orchestrator locks the data node. A "manual" editor should fail to append it.

jrobinAV · 2024-10-31T08:30:06Z

taipy/core/data/data_node.py

+            raise DataNodeIsBeingEdited(self.id, self.editor_id)
+
+        if not editor_id and self.edit_in_progress:
+            print("Orchestrator writing without editor_id")


This covers the case where the Orchestrator attempts to write a data node locked by an editor. We can eventually add an info log using the _logger attribute, but we don't want any print in the production code. Please remove it.

jrobinAV · 2024-10-31T08:40:52Z

taipy/core/data/data_node.py

        self._write(data)
-        self.track_edit(job_id=job_id, **kwargs)
-        self.unlock_edit()
+        self.last_edit_date = datetime.now()


The last_edit_date setting is already done in the track_edit method called right after. Please revert this line.

jrobinAV · 2024-10-31T08:45:03Z

taipy/core/data/data_node.py

+
+        if self._editor_expiration_date and datetime.now() > self._editor_expiration_date:
+            self.unlock_edit()
+


I don't see what case we want to cover here. For me, the next condition already covers this case. Can you elaborate? Otherwise, we should revert it.

jrobinAV · 2024-10-31T09:56:30Z

taipy/core/data/_file_datanode_mixin.py

-
-        self.track_edit(timestamp=datetime.now())  # type: ignore[attr-defined]
+        self._write(data)
+        self.track_edit(job_id=job_id, editor_id=editor_id, **kwargs)  # type: ignore[attr-defined]


Suggested change

self.track_edit(job_id=job_id, editor_id=editor_id, **kwargs) # type: ignore[attr-defined]

self.track_edit(timestamp=datetime.now(), editor_id=editor_id, **kwargs) # type: ignore[attr-defined]

jrobinAV · 2024-10-31T09:58:36Z

taipy/core/data/_file_datanode_mixin.py

-
-        self.track_edit(timestamp=datetime.now())  # type: ignore[attr-defined]
+        self._write(data)
+        self.track_edit(job_id=job_id, editor_id=editor_id, **kwargs)  # type: ignore[attr-defined]


THere is no possible job_id. This cannot be called by the orchestrator.

jrobinAV · 2024-10-31T09:59:23Z

taipy/core/data/_file_datanode_mixin.py

+        if self.edit_in_progress and self.editor_id != editor_id:
+            raise DataNodeIsBeingEdited(self.id, self.editor_id)
+


Instead of raising an exception, you should build a specific Reason and add it to the reason_collection, as it is done below in the code for instance. You can re-use the DataNodeEditInProgress reason class.

I would also move this code after the creation of the reason_collection variable:
reason_collection = ReasonCollection()

jrobinAV · 2024-10-31T10:41:55Z

taipy/core/data/in_memory.py

+
+    def _append(self, data):
+        """Append data to the existing data in the in-memory storage."""
+        if self.id not in in_memory_storage:


self.id must not be None. you can remove it from the condition.

jrobinAV · 2024-10-31T10:43:00Z

taipy/core/data/in_memory.py

+        if not isinstance(in_memory_storage[self.id], list):
+            in_memory_storage[self.id] = [in_memory_storage[self.id]]


If in_memory_storage[self.id] is not a list, a NotImplementedError should be raised.

jrobinAV

Comments should be resolved first.

PrakharJain1509 · 2024-11-01T09:44:56Z

Sure, Give me some time, Will do so in some days.

jrobinAV · 2024-11-13T17:58:39Z

@PrakharJain1509 FYI, this PR is slightly related to your changes.

#2240

jrobinAV · 2024-11-18T17:25:21Z

@PrakharJain1509 Do you plan to keep working on this PR?
Please let us know if you need any help.

jrobinAV · 2024-11-20T17:46:11Z

PR #2265 already solves the issue.

PrakharJain1509 and others added 2 commits October 22, 2024 00:10

Fix: Enforce editor lock for write, append, and upload in DataNode.

7fc9c4c

Merge branch 'Avaiga:develop' into develop

80a3627

PrakharJain1509 mentioned this pull request Oct 21, 2024

[🐛 BUG] Data node's editor id mechanism does not work #2017

Closed

7 tasks

FlorianJacta requested a review from jrobinAV October 22, 2024 07:25

FlorianJacta assigned PrakharJain1509 Oct 22, 2024

Merge branch 'develop' into develop

fa18f0c

bobbyshermi suggested changes Oct 22, 2024

View reviewed changes

jrobinAV added Core Related to Taipy Core 🟧 Priority: High Must be addressed as soon Core: Data node labels Oct 22, 2024

PrakharJain1509 added 2 commits October 22, 2024 15:28

Debugged issues.

5677a46

Merge remote-tracking branch 'origin/develop' into develop

0e9825a

PrakharJain1509 requested a review from bobbyshermi October 22, 2024 09:59

Debugged issues.

87152de

jrobinAV added hacktoberfest hacktoberfest issues hacktoberfest - 100💎 Issues rewarded by 100 points labels Oct 31, 2024

jrobinAV requested review from toan-quach and removed request for bobbyshermi October 31, 2024 07:54

toan-quach previously approved these changes Oct 31, 2024

View reviewed changes

jrobinAV reviewed Oct 31, 2024

View reviewed changes

jrobinAV requested changes Oct 31, 2024

View reviewed changes

Merge branch 'develop' into develop

08d70c3

jrobinAV dismissed toan-quach’s stale review via 08d70c3 November 14, 2024 14:41

jrobinAV mentioned this pull request Nov 20, 2024

Feature/#2017 editor #2265

Merged

9 tasks

jrobinAV closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce Editor Lock for Write, Append, and Upload Operations in DataNode #2122

Enforce Editor Lock for Write, Append, and Upload Operations in DataNode #2122

PrakharJain1509 commented Oct 21, 2024 •

edited by jrobinAV

Loading

PrakharJain1509 commented Oct 21, 2024 •

edited

Loading

bobbyshermi Oct 22, 2024

PrakharJain1509 Oct 22, 2024

PrakharJain1509 commented Oct 24, 2024

PrakharJain1509 commented Oct 24, 2024 •

edited

Loading

PrakharJain1509 commented Oct 25, 2024

PrakharJain1509 commented Oct 26, 2024

jrobinAV commented Oct 31, 2024

toan-quach left a comment •

edited

Loading

jrobinAV left a comment

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV Oct 31, 2024

jrobinAV left a comment

PrakharJain1509 commented Nov 1, 2024

jrobinAV commented Nov 13, 2024 •

edited

Loading

jrobinAV commented Nov 18, 2024

jrobinAV commented Nov 20, 2024


		if self._editor_expiration_date and datetime.now() > self._editor_expiration_date:
		self.unlock_edit()

	self.track_edit(job_id=job_id, editor_id=editor_id, **kwargs) # type: ignore[attr-defined]
	self.track_edit(timestamp=datetime.now(), editor_id=editor_id, **kwargs) # type: ignore[attr-defined]

		if self.edit_in_progress and self.editor_id != editor_id:
		raise DataNodeIsBeingEdited(self.id, self.editor_id)

		if not isinstance(in_memory_storage[self.id], list):
		in_memory_storage[self.id] = [in_memory_storage[self.id]]

Enforce Editor Lock for Write, Append, and Upload Operations in DataNode #2122

Enforce Editor Lock for Write, Append, and Upload Operations in DataNode #2122

Conversation

PrakharJain1509 commented Oct 21, 2024 • edited by jrobinAV Loading

PR Description

Summary

Changes Implemented

Acceptance Criteria Met

PrakharJain1509 commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrakharJain1509 commented Oct 24, 2024

PrakharJain1509 commented Oct 24, 2024 • edited Loading

PrakharJain1509 commented Oct 25, 2024

PrakharJain1509 commented Oct 26, 2024

jrobinAV commented Oct 31, 2024

toan-quach left a comment • edited Loading

Choose a reason for hiding this comment

jrobinAV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobinAV left a comment

Choose a reason for hiding this comment

PrakharJain1509 commented Nov 1, 2024

jrobinAV commented Nov 13, 2024 • edited Loading

jrobinAV commented Nov 18, 2024

jrobinAV commented Nov 20, 2024

PrakharJain1509 commented Oct 21, 2024 •

edited by jrobinAV

Loading

PrakharJain1509 commented Oct 21, 2024 •

edited

Loading

PrakharJain1509 commented Oct 24, 2024 •

edited

Loading

toan-quach left a comment •

edited

Loading

jrobinAV commented Nov 13, 2024 •

edited

Loading