require FORCE_FLAG_KW uniformly for overwrites by put and create #721

d-w-moore · 2025-05-01T13:51:48Z

No description provided.

d-w-moore · 2025-05-01T13:52:06Z

tests are upcoming.

alanking · 2025-05-01T14:00:18Z

irods/manager/data_object_manager.py

+        if force:
+            options[kw.FORCE_FLAG_KW]=''
+
+        if self.exists(path) and kw.FORCE_FLAG_KW not in options:


Let's reverse the conditions as is being done in put:

Suggested change

if self.exists(path) and kw.FORCE_FLAG_KW not in options:

if kw.FORCE_FLAG_KW not in options and self.exists(path):

alanking · 2025-05-01T14:02:44Z

irods/manager/data_object_manager.py

+        # TODO: We could deprecate and then later remove the force parameter in favor of FORCE_FLAG_KW.
+        #       That seems in order, since FORCE_FLAG_KW is the predominant pattern for asserting overwrites.


Go ahead and make an issue for this to attach to the TODO.

alanking · 2025-05-01T14:05:26Z

irods/manager/data_object_manager.py

@@ -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)


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.

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

trel · 2025-05-01T16:15:19Z

thought experiment question.... about blast radius...

how many existing python applications will start to fail on their puts and need to be updated to support a force flag?

second question...
given this is what we want to do... is it better/best to wait until 3.2.0 rather than a patch release? if we're considering the current behavior a bug... then 3.1.1 is fine.

korydraughn · 2025-05-01T16:52:39Z

That's a good question. If we're introducing a breaking change, then that means a major version bump.

alanking · 2025-05-01T17:08:07Z

That's a good question. If we're introducing a breaking change, then that means a major version bump.

#132 is a bug so, although this would be a behavioral change, I would expect this in a minor release if not a patch release since the current behavior is incorrect. Thoughts?

korydraughn · 2025-05-01T17:20:45Z

If it's a bug, then yes. I agree it's fine to include it in a minor/patch release.

trel · 2025-05-01T17:21:12Z

i'm leaning to 'number go up' and people can tweak their force flag usage. we're not going to accidentally overwrite MORE things... it will be fewer things.

d-w-moore · 2025-05-01T18:30:32Z

thought experiment question.... about blast radius...

how many existing python applications will start to fail on their puts and need to be updated to support a force flag?

second question... given this is what we want to do... is it better/best to wait until 3.2.0 rather than a patch release? if we're considering the current behavior a bug... then 3.1.1 is fine.

No source installations will fail, because the default behavior of force = False will be "absorbed" into **opts, as well as presence of the keyword if/when provided.
So consider if we do:

def force_is_not_true( force=False , **opts): 
  return True==(force or opts.get('force'))

We now have a situation in which we can leverage that force either has a boolean equivalent that's True, or (if given as None/False/bool-false-equivalents, or not given at all ) then it will not be True. That's our present dichotomy.

then:

force_is_not_true(force = False) # => False
force_is_not_true( ) => False
force_is_not_true( force = True) => True

at which point we can really just delete the force keyword, even without deprecating first.

trel · 2025-05-01T19:13:10Z

I'm having trouble even reading that comment. force and negative and false...

d-w-moore · 2025-05-01T21:13:36Z

I'm having trouble even reading that comment. force and negative and false...

Sorry. It was written in a hurry and without clarity of thought. I'm going to redo it.

d-w-moore · 2025-05-02T00:38:48Z

So in a nutshell, we can replace

    def create(self, path, resource=None, force=False, **options):
        # ... function body

with

    def create(self, path, resource=None, **options):
      force = options.get('force',False)
      ##.... rest of function body

and in fact no applications should break. That's because despite removing the force=False in the function signature,Python will make sure any force=<value> keyword argument passed into the function call is folded into the **options dict.

Note - I wouldn't be so very keen to get rid of an option like force=False thing except that leaving it in at the same time we're beginning to officially respect FORCE_FLAG_KW in options would be confusing, to many, I fear.

There are other uses of a force parameter in the PRC presently, but they don't alias to use of FORCE_FLAG_KW.

trel · 2025-05-02T01:37:27Z

that makes sense. thank you

d-w-moore · 2025-05-02T10:48:05Z

thought experiment question.... about blast radius...

how many existing python applications will start to fail on their puts and need to be updated to support a force flag?

second question... given this is what we want to do... is it better/best to wait until 3.2.0 rather than a patch release? if we're considering the current behavior a bug... then 3.1.1 is fine.

To the more direct question: Yes, definitely a valid concern, as applications will break due to the PRC now honoring the semantics of FORCE_FLAG_KW unless we change the default value of force to True. I'd have no qualms in doing that, especially if we agree to the absorption of the force keyword into options and document all of this.

Yes, I need to do a little more work on the docstring...

d-w-moore · 2025-05-02T11:00:58Z

you know what? Nevermind. I'm wrong. @trel is correct.

What a complete mess.

The traditional behavior of allowing an overwrite despite the advertised default of force=False is, well, a blatant lie to the application developer. Unfortunately, to correct that, we have to break applications.

We should wait for a major release.

d-w-moore · 2025-05-02T11:24:25Z

The alternative, if we want something sensible in v3.1.1 to somewhat solve this mess but not break others' source code, is this:

changing the default value of force to True, i.e. correcting the lie.
allow forced overwrites to happen via either of:
- force = True being passed , whether explicitly or by defaulting; or,
- passing FORCE_FLAG_KW in options

In this way, even in the upcoming v3.1.1, applications will behave as they have behaved, except for those where FORCE_FLAG_KW was included in options accidentally (without effect, up to now).

The disadvantage: honoring the FORCE_FLAG_KW semantics will require that application programmers always explicitly specify force = False. That is the "ick" part, the fact that there are two independent but oppositely-defaulting ways to achieve the same thing.

That alone is really enough to make me opt to wait for a major release.

trel · 2025-05-02T13:16:20Z

If I understand the main confusion/difference... the PRC .put is really an open/write/close.

Not sure that helps resolve our decision here, but it's definitely part of the conversation.

korydraughn · 2025-05-02T13:52:54Z

Adding this as a discush item.

d-w-moore · 2025-05-02T17:43:14Z

Ok, better idea ... since the CREATE/OPEN api doesn't respect FORCE_FLAG_KW (or absence of it) anyway, we shouldn't pretend the create( ) call needs it either. So in this next release of 3.1.1, we could just correct the real bug and make it internally consistent.

That is, we won't change the force = False, or remove the force keyword either. We'll simply make it throw a custom exception, OverwriteWithoutForceSelected , when the logical path pre-exists, or something by a better name if someone can think of it. The reason I don't want to throw OVERWRITE_WITHOUT_FORCE_FLAG is because the force flag it mentions refers to the actual FORCE_FLAG_KW, used by and for the server

As we've discussed, the deviation of this call from its advertised behavior (truncating / overwriting even with force=False) is a bug in itself, which most likely very few, or no-one, may be relying on.

korydraughn · 2025-05-02T17:54:21Z

The reason I don't want to throw OVERWRITE_WITHOUT_FORCE_FLAG is because the force flag it mentions refers to the actual FORCE_FLAG_KW, used by and for the server

I agree that that is a much better approach. Making it clear that this is specific to the PRC is good.

Alternative names:

DataObjectExistsAtLogicalPath
LogicalPathAlreadyExists
OverwriteRequiresForceFlag
PreexistingDataObjectAtLogicalPath
LogicalPathNameCollision

korydraughn · 2025-05-02T18:06:20Z

More names:

LogicalPathInUse
LogicalPathAlreadyInUse
OverwriteNotAllowedWithoutForceFlag
OverwriteNotAllowed

trel · 2025-05-02T18:13:18Z

LogicalPathAlreadyExists catches my eye

d-w-moore · 2025-05-02T18:23:03Z

LogicalPathAlreadyExists catches my eye

Say no more. Now a part of this PR...! That was my first choice too, among Kory's suggestions.

…write PUT will respect FORCE_FLAG_KW, to mimic iput; whereas create will now respect the force=True option when requested.

d-w-moore · 2025-05-09T17:26:27Z

thought experiment question.... about blast radius...

how many existing python applications will start to fail on their puts and need to be updated to support a force flag?

second question... given this is what we want to do... is it better/best to wait until 3.2.0 rather than a patch release? if we're considering the current behavior a bug... then 3.1.1 is fine.

My thoughts

It's likely some major applications will require some major updates in whatever window / milestone we choose for this
On consideration, it can probably wait for v3.2.0. It's been this way for a while. in the meanwhile:
We could introduce {put|create}_without_force( ) for those who want correct behavior in v3.1.1 - whatsay team???
If we agree it's a bug , that CREATE and PUT can cause data loss the way they are ... then shall we label it a bug?

trel · 2025-05-09T17:42:21Z

data loss from CREATE feels very wrong

data loss (aka overwrite) from PUT does not feel as incorrect.. just a bit, perhaps, pointy/dangerous/surprising for a new user.

alanking reviewed May 1, 2025

View reviewed changes

d-w-moore marked this pull request as draft May 2, 2025 12:42

d-w-moore force-pushed the 132.m branch from 6055c16 to b9b1850 Compare May 2, 2025 18:07

[_132] data object put and create can raise an error rather than over…

95de290

…write PUT will respect FORCE_FLAG_KW, to mimic iput; whereas create will now respect the force=True option when requested.

d-w-moore force-pushed the 132.m branch from 92e5e6e to 95de290 Compare May 4, 2025 15:48

corrections

362d119

d-w-moore self-assigned this May 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

require FORCE_FLAG_KW uniformly for overwrites by put and create #721

require FORCE_FLAG_KW uniformly for overwrites by put and create #721

d-w-moore commented May 1, 2025

d-w-moore commented May 1, 2025

alanking May 1, 2025

alanking May 1, 2025

alanking May 1, 2025

d-w-moore May 1, 2025

trel commented May 1, 2025

korydraughn commented May 1, 2025

alanking commented May 1, 2025

korydraughn commented May 1, 2025

trel commented May 1, 2025

d-w-moore commented May 1, 2025 •

edited

Loading

trel commented May 1, 2025

d-w-moore commented May 1, 2025

d-w-moore commented May 2, 2025 •

edited

Loading

trel commented May 2, 2025

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

trel commented May 2, 2025

korydraughn commented May 2, 2025

d-w-moore commented May 2, 2025 •

edited

Loading

korydraughn commented May 2, 2025

korydraughn commented May 2, 2025

trel commented May 2, 2025

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 9, 2025

trel commented May 9, 2025

	if self.exists(path) and kw.FORCE_FLAG_KW not in options:
	if kw.FORCE_FLAG_KW not in options and self.exists(path):

		# TODO: We could deprecate and then later remove the force parameter in favor of FORCE_FLAG_KW.
		# That seems in order, since FORCE_FLAG_KW is the predominant pattern for asserting overwrites.

require FORCE_FLAG_KW uniformly for overwrites by put and create #721

Are you sure you want to change the base?

require FORCE_FLAG_KW uniformly for overwrites by put and create #721

Conversation

d-w-moore commented May 1, 2025

d-w-moore commented May 1, 2025

alanking May 1, 2025

Choose a reason for hiding this comment

alanking May 1, 2025

Choose a reason for hiding this comment

alanking May 1, 2025

Choose a reason for hiding this comment

d-w-moore May 1, 2025

Choose a reason for hiding this comment

trel commented May 1, 2025

korydraughn commented May 1, 2025

alanking commented May 1, 2025

korydraughn commented May 1, 2025

trel commented May 1, 2025

d-w-moore commented May 1, 2025 • edited Loading

trel commented May 1, 2025

d-w-moore commented May 1, 2025

d-w-moore commented May 2, 2025 • edited Loading

trel commented May 2, 2025

d-w-moore commented May 2, 2025 • edited Loading

d-w-moore commented May 2, 2025 • edited Loading

d-w-moore commented May 2, 2025 • edited Loading

trel commented May 2, 2025

korydraughn commented May 2, 2025

d-w-moore commented May 2, 2025 • edited Loading

korydraughn commented May 2, 2025

korydraughn commented May 2, 2025

trel commented May 2, 2025

d-w-moore commented May 2, 2025 • edited Loading

d-w-moore commented May 9, 2025

trel commented May 9, 2025

d-w-moore commented May 1, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading

d-w-moore commented May 2, 2025 •

edited

Loading