Skip to content

require FORCE_FLAG_KW uniformly for overwrites by put and create #721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions irods/exception.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class InvalidInputArgument(PycommandsException):
pass


class LogicalPathAlreadyExists(PycommandsException):
pass


class DataObjectDoesNotExist(DoesNotExist):
pass

Expand Down
20 changes: 15 additions & 5 deletions irods/manager/data_object_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,9 @@ def put(
)
else:
obj = irods_path
if kw.FORCE_FLAG_KW not in options and self.exists(obj):
raise ex.OVERWRITE_WITHOUT_FORCE_FLAG
options.pop(kw.FORCE_FLAG_KW, None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we don't need to pop this, but if it seems better to do so, perhaps a "why" comment would be appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Popping is a matter of form, I think. Don't allow options to be included for an API call that doesn't respect them


with open(local_path, "rb") as f:
sizelist = []
Expand Down Expand Up @@ -459,8 +462,18 @@ def parallel_put(
updatables=updatables,
)

def create(self, path, resource=None, force=False, **options):
options[kw.DATA_TYPE_KW] = "generic"
def create(self, path, resource=None, force=None, **options):
"""
Create a new data object with the given logical path.

'resource', if provided, is the root node of a storage resource hierarchy where the object is preferentially to be created.
'force', when False, raises an LogicalPathAlreadyExists if there is already a data object at the logical path specified.
"""

if not force and self.exists(path):
raise ex.LogicalPathAlreadyExists

options = {**options, kw.DATA_TYPE_KW: "generic"}

if resource:
options[kw.DEST_RESC_NAME_KW] = resource
Expand All @@ -471,9 +484,6 @@ def create(self, path, resource=None, force=False, **options):
except AttributeError:
pass

if force:
options[kw.FORCE_FLAG_KW] = ""

message_body = FileOpenRequest(
objPath=path,
createMode=0o644,
Expand Down
54 changes: 54 additions & 0 deletions irods/test/data_obj_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from datetime import datetime, timezone
import base64
import collections
import concurrent.futures
import contextlib
import errno
Expand Down Expand Up @@ -51,6 +52,7 @@ def is_localhost_synonym(name):

from irods.access import iRODSAccess
from irods.models import Collection, DataObject
from irods.path import iRODSPath
from irods.test.helpers import iRODSUserLogins
import irods.exception as ex
from irods.column import Criterion
Expand Down Expand Up @@ -2951,6 +2953,58 @@ def test_replica_truncate__issue_534(self):
if data_objs.exists(data_path):
data_objs.unlink(data_path, force=True)

def test_data_create_and_put_without_forcing__issue_132(self):
test_path = iRODSPath(
self.coll_path, unique_name(my_function_name(), datetime.now())
)
contents = b"** CONTENTS **"
Data_Object_Properties = collections.namedtuple(
"repl_properties", ["modify_times", "resource_names"]
)

def check_or_return_properties(data_repls, reference_value=None):
properties_dict = {_.resource_name: _.modify_time for _ in data_repls}
properties = Data_Object_Properties(
modify_times=list(properties_dict.values()),
resource_names=list(properties_dict.keys()),
)
if (sentinel := reference_value) is not None:
return sentinel == properties
else:
return properties

# Create a data object with known content. Record a reference state for later comparison.
test_data_object = self.sess.data_objects.create(test_path, force=True)
with test_data_object.open("w") as f:
f.write(contents)
orig_repl_state = check_or_return_properties(test_data_object.replicas)

# Create should have made only a single replica.
self.assertEqual(1, len(orig_repl_state.resource_names))

# If using force=False, then an attempt to create an object at the same path should raise an exception.
with self.assertRaises(ex.LogicalPathAlreadyExists):
d2 = self.sess.data_objects.create(test_path, force=False)

# Assert that a put without FORCE_FLAG_KW raises an Exception.
with NamedTemporaryFile() as t:
altered_contents = b"[[" + contents + b"]]"
t.write(altered_contents)
t.close()
with self.assertRaises(ex.OVERWRITE_WITHOUT_FORCE_FLAG):
self.sess.data_objects.put(t.name, test_path)

# Assert that the data object's contents are unchanged.
with test_data_object.open("r") as f:
self.assertEqual(f.read(), contents)

# Assert that the modify times and hosting resources haven't changed.
self.assertTrue(
check_or_return_properties(
self.sess.data_objects.get(test_path).replicas, reference_value = orig_repl_state
)
)


if __name__ == "__main__":
# let the tests find the parent irods lib
Expand Down