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

bobbyshermi commented 1 week ago

What went wrong? 🤔

When using a data node control, the lock/unlock/write mechanism works fine. But we cannot always use a datanode control.

When locking a data node providing an "editor_id", this is only prevent another editor to lock the data node. It does not prevent the other editor to write the data node.

According to the documentation it should prevent another editor to write (append, upload) the data node.

Expected Behavior

When a data node is locked by an editor, no other editor should be able to write it. Same for append and _upload.

Question: When the data node is written without any editor_id (that's what the Orchestrator does), should this be accepted even if an editor has locked it?

Steps to Reproduce Issue

from taipy import Config, Scope, create_scenario
from taipy.core.exceptions import DataNodeIsBeingEdited

d1 = Config.configure_data_node("d1", scope=Scope.GLOBAL, default_data=1)
s1 = Config.configure_scenario("s1", additional_data_node_configs=[d1])
scenario_1 = create_scenario(s1)
dn1 = scenario_1.d1

dn1.lock_edit()
try:
    dn1.write(4, editor_id="editor_id")
    print("SHOULD HAVE RAISED AN EXCEPTION! IT DIDN'T.")
except DataNodeIsBeingEdited as e:
    pass
dn1.unlock_edit()

dn1.lock_edit("editor_id")
try:
    dn1.write(5, editor_id="other")
    print("SHOULD HAVE RAISED AN EXCEPTION! IT DIDN'T.")
except DataNodeIsBeingEdited as e:
    pass

Solution Proposed

The editor_id should be an optional parameter of the write method, just like the lock and unlock methods.


def write(self, data, job_id: Optional[JobId] = None, editor_id: Optional[str] = None, **kwargs: Dict[str, Any]):
        from ._data_manager_factory import _DataManagerFactory
        if editor_id:
            if not self.editor_id:
                raise DataNodeIsBeingEdited(self.id, self._editor_id)
            if (
                self.edit_in_progress
                and self.editor_id != editor_id
                and self.editor_expiration_date
                and self.editor_expiration_date > datetime.now()
            ):
                raise DataNodeIsBeingEdited(self.id, self._editor_id)
        self._write(data)
        self.track_edit(job_id=job_id, editor_id = editor_id, **kwargs)
        self.unlock_edit(editor_id)
        _DataManagerFactory._build_manager()._set(self)

Similar code or factorized for append and upload.

Acceptance Criteria

[ ] The Datanode.write() method must reflect the new behavior.
[ ] The Datanode.append() method must reflect the new behavior.
[ ] The _FileDataNodeMixin._upload() method must reflect the new behavior.
[ ] Unit tests must be updated or created in tests/core/data/test_data_node.py and tests/core/data/test_file_data_node.py to get at least 90% code coverage on the modified methods.
[ ] Create a related issue in taipy-doc for documentation and Release Notes.

Code of Conduct

[X] I have checked the existing issues.
[ ] I am willing to work on this issue (optional)

aarcex3 commented 1 week ago

Hi, can I work on this?

Anudhyan commented 1 week ago

Please assign me this issue @bobbyshermi @jrobinAV

AdibaNaushad commented 1 week ago

and i can assist you with it also include me

jrobinAV commented 1 week ago

Full test coverage is expected for this issue, as it may have an impact on the core of the Taipy orchestration system.

AdibaNaushad commented 1 week ago

@bobbyshermi @jrobinAV i have done some testing on the given code and i have both result and issue which i am giving below

"result of the Tests"

Basic Lock/Unlock: This verifies that you can lock and unlock a data node without issues.

Write Operation: Tests writing to the node when locked by the same editor ID and ensures the state is correct after the write.

Error Handling: Ensures that attempts to lock the node by a different editor ID correctly raise exceptions.

Re-locking: Tests the ability to lock the node again after unlocking and check the state.

Edge Cases: Checks that trying to unlock when not locked or writing when locked raises the correct exceptions.

"Issues with code"

Multiple Lock Attempts: The code tries to lock the same data node multiple times without unlocking it in between, which could lead to confusion in the output.

Error Handling: The way exceptions are handled is somewhat inconsistent. For example, there are sections where the code is expected to raise exceptions but continues executing without a clear structure.

Assertions and State Checks: Instead of just printing messages, adding assertions would help confirm expected states after operations, making it easier to identify where things might go wrong.

Redundant Code: There are repeated blocks of code, which can be encapsulated in functions for better readability and maintenance.

if this is approved and proper for the issue than please allow and guide me to do what ever is require to correct and move further step to do or any changes required, if i am going in right way please guide me .

jrobinAV commented 1 week ago

Hi @AdibaNaushad,

Thank you for your help.

I am not sure which tests you are referring to in your message. If you are talking about the piece of code that is under the "Step To Reproduce" section, this is not a test to run. It is just a piece of code using taipy that exhibits the bug so you can reproduce it. It is not meant to be added to the taipy repository as a unit test nor at actual production code.

The whole purpose of the issue is to:

Change the DataNode.write function (append and upload) to fix the bug. You can get some inspiration from the code that @bobbyshermi provided under the section "Solution Proposed".
Write new unit tests to prove the code works fine in all the use cases we want to cover and to detect regressions.

AdibaNaushad commented 1 week ago

@jrobinAV i have debug the code given above . i will do unit testing soon . if everything is correct or moving in right direction please allow me to move further. any guidance i need please tell me i am trying to do best for project .

the Debugging Strategy i use is :

Print Debugging: We will use additional print statements to track the status of lock_edit, editor_id, and edit_in_progress in each case to ensure that the methods are working as intended. Validate Behavior with Correct Editor: Check if the locking mechanism works correctly when the same editor_id is used for writing. Test Different editor_ids: Confirm that trying to write with a different editor_id raises the appropriate exceptions.

Debugging Changes:

print_status Function: This helper function prints the lock status, the current editor (if any), and the current value of the DataNode. This helps in tracking what’s happening after each operation.

Tracking Exceptions: The print statements and exception handling have been made more explicit, especially when checking if an exception is expected but not raised (e.g., when a different editor attempts to lock or write).

Detailed Output for Locking/Unlocking: The output now shows exactly when the DataNode is locked, unlocked, written to, or when an exception is raised.

from taipy import Config, Scope, create_scenario
from taipy.core.exceptions import DataNodeIsBeingEdited

# Configure the DataNode and Scenario
d1 = Config.configure_data_node("d1", scope=Scope.GLOBAL, default_data=1)
s1 = Config.configure_scenario("s1", additional_data_node_configs=[d1])
scenario_1 = create_scenario(s1)
dn1 = scenario_1.d1

# Helper function to print the status of the DataNode
def print_status(dn, label):
    print(f"{label} - Locked: {'Yes' if dn.edit_in_progress else 'No'}, Editor: {dn.editor_id if dn.editor_id else 'None'}, Value: {dn.read()}")

# Lock dn1 without editor_id
print("Locking dn1")
dn1.lock_edit()
print_status(dn1, "After locking dn1")
print("Unlocking dn1")
dn1.unlock_edit()
print_status(dn1, "After unlocking dn1\n")

# Lock dn1 without editor_id and write
print("Locking dn1 again")
dn1.lock_edit()
print_status(dn1, "After locking dn1")
print("Writing 2 to dn1")
dn1.write(2)
print_status(dn1, "After writing 2 to dn1")
print("Unlocking dn1")
dn1.unlock_edit()
print_status(dn1, "After unlocking dn1\n")

# Lock dn1 with editor_id "editor_id"
print("Locking dn1 with editor_id")
dn1.lock_edit("editor_id")
print_status(dn1, "After locking dn1 with editor_id")

# Unlock with the same editor_id
print("Unlocking dn1 with editor_id")
dn1.unlock_edit("editor_id")
print_status(dn1, "After unlocking dn1 with editor_id\n")

# Lock dn1 with editor_id and attempt lock with different editor
print("Locking dn1 with editor_id")
dn1.lock_edit("editor_id")
print_status(dn1, "After locking dn1 with editor_id")

print("Attempting to lock dn1 with another editor 'other'")
try:
    dn1.lock_edit("other")
except DataNodeIsBeingEdited as e:
    print(f"Exception caught: {e.message}")
print_status(dn1, "After attempting to lock with 'other'")
print("Unlocking dn1 with editor_id")
dn1.unlock_edit("editor_id")
print_status(dn1, "After unlocking dn1 with editor_id\n")

# Lock dn1 with editor_id and attempt to write without proper editor_id
print("Locking dn1 with editor_id")
dn1.lock_edit("editor_id")
print_status(dn1, "After locking dn1 with editor_id")

print("Attempting to write 3 to dn1 without editor_id")
try:
    dn1.write(3)
    print("Should this raise an exception? It didn't.")
except DataNodeIsBeingEdited as e:
    print(f"Exception caught: {e.message}")
print_status(dn1, "After attempting to write without editor_id")
print("Unlocking dn1 with editor_id")
dn1.unlock_edit("editor_id")
print_status(dn1, "After unlocking dn1 with editor_id\n")

# Lock dn1 and attempt to write with mismatched editor_id
print("Locking dn1 again")
dn1.lock_edit()
print_status(dn1, "After locking dn1")
print("Attempting to write 4 to dn1 with mismatched editor_id 'editor_id'")
try:
    dn1.write(4, editor_id="editor_id")
    print("Should have raised an exception! It didn't.")
except DataNodeIsBeingEdited as e:
    print(f"Exception caught: {e.message}")
print_status(dn1, "After attempting to write with mismatched editor_id")
print("Unlocking dn1")
dn1.unlock_edit()
print_status(dn1, "After unlocking dn1\n")

# Lock dn1 with editor_id and write with another editor
print("Locking dn1 with editor_id")
dn1.lock_edit("editor_id")
print_status(dn1, "After locking dn1 with editor_id")
print("Attempting to write 5 to dn1 with another editor_id 'other'")
try:
    dn1.write(5, editor_id="other")
    print("Should have raised an exception! It didn't.")
except DataNodeIsBeingEdited as e:
    print(f"Exception caught: {e.message}")
print_status(dn1, "After attempting to write with 'other'")
print("Unlocking dn1 with editor_id")
dn1.unlock_edit("editor_id")
print_status(dn1, "After unlocking dn1 with editor_id\n")

# Lock dn1 with editor_id and successfully write
print("Locking dn1 with editor_id")
dn1.lock_edit("editor_id")
print_status(dn1, "After locking dn1 with editor_id")
print("Writing 6 to dn1 with editor_id")
dn1.write(6, editor_id="editor_id")
print_status(dn1, "After writing 6 to dn1")
print("Unlocking dn1 with editor_id")
dn1.unlock_edit("editor_id")
print_status(dn1, "After unlocking dn1 with editor_id\n")

jrobinAV commented 1 week ago

@AdibaNaushad I don't understand what you are proposing here. The code you are editing is just some code that prints messages to exhibit the issue. It is not part of the taipy code base, and we don't want to add it. You can use it to understand the bug or to inspire you to write good unit tests, but there is no need to edit it.

Let me rephrase what we need:

We must change the Datanode.write() method.
We must change the Datanode.append() method.
We need to change the _FileDataNodeMixin._upload() method
Update and write new unit tests in tests/core/data/test_data_node.py and tests/core/data/test_file_data_node.py (to be created) to get a full code coverage on the modified methods.

For information, I edited the description to avoid confusion.

AdibaNaushad commented 1 week ago

@jrobinAV if i have done changes than should i comment to here approved or directly create a pull request to merge that you can review ?? i am fresher so please guide me and sorry for anything irrelevant disturbance i have done to you

jrobinAV commented 1 week ago

@AdibaNaushad No problem. Asking questions is part of the contributor's job.

Please create a pull request directly so we can comment and discuss around the code directly.

Thank you,

Ankur2606 commented 4 days ago

Looks niche , I'm taking on this issue and might as well spend my weekends on it 😃 .

PrakharJain1509 commented 3 days ago

@bobbyshermi Can I work on this issue ?

FlorianJacta commented 2 days ago

I have assigned it to you @Ankur2606 and @PrakharJain1509; thank you for your contribution!

PrakharJain1509 commented 2 days ago

@FlorianJacta @bobbyshermi I have created a PR for the same. Please have a review and let me know if any changes are required. #2122 Here is the PR link

Avaiga / taipy