Skip to content

[#505] create an atomic ACLs endpoint#808

Open
d-w-moore wants to merge 13 commits intoirods:mainfrom
d-w-moore:505.m
Open

[#505] create an atomic ACLs endpoint#808
d-w-moore wants to merge 13 commits intoirods:mainfrom
d-w-moore:505.m

Conversation

@d-w-moore
Copy link
Collaborator

No test yet, will undraft when I have one.
Exercised now with this script:

# run as rods after 
# $ irm -fr b; itouch b
# $ iadmin mkzone twilight remote
# $ iadmin mkuser dan rodsuser
# $ iadmin mkuser charlie#twilight rodsuser

import irods
from irods.access import iRODSAccess
from irods.exception import iRODSException

s = irods.helpers.make_session()

try:
  pass
  s.acls._call_atomic_acl_api(
   "/tempZone/home/rods/b",
   iRODSAccess("write","","dan"),
   iRODSAccess("write","","public"),
   iRODSAccess("read","","charlie","twilight"),
 )
except iRODSException as exc:
  print(f'{exc!r}')
  e = exc
  print (e.server_msg.get_json_encoded_struct())
#except Exception as exc:
#  print(repr(exc))
else:
  print ('---> success')
  pass

@d-w-moore d-w-moore marked this pull request as draft March 20, 2026 23:28
@d-w-moore d-w-moore self-assigned this Mar 21, 2026
@d-w-moore d-w-moore marked this pull request as ready for review March 21, 2026 04:45
@d-w-moore
Copy link
Collaborator Author

How would the team feel about my deprecating _iRODSAccess_pre_4_3_0, since we're now officially not supporting iRODS < 4.3.0 in PRC?

@korydraughn
Copy link
Contributor

The leading underscore indicates that symbol is private to the implementation. If that's the case, no objections here.

@korydraughn
Copy link
Contributor

Then again, if it's an implementation detail, then we don't have to deprecate it. We can just remove it.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Mar 23, 2026

Ready for review. ACLOperation now created as part of an improved interface for atomic ACLs call.

@korydraughn
Copy link
Contributor

Spotted this in the type checking workflow.

irods/manager/access_manager.py:54: error: Incompatible types in assignment (expression has type "list[Any]", target has type "str")  [assignment]

Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Looking good so far.

irods/access.py Outdated
Comment on lines +94 to +99
def __lt__(self, other):
return (
self.access_name < other.access_name
and self.user_name < other.user_name
and self.user_zone < other.user_zone
) and iRODSPath(self.path) < iRODSPath(other.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this affect the Access class?
Do users need to know about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for drawing my attention to it, as it was improperly defined.

Yes, it maybe should have a comment at least.

It mainly just gives the class a defined sorting order. Now, if someone attempts pretty-printing a mixed list of these items, they'll get something the eye is somewhat able to follow, although I guess if the two formats were more consistent and the permissions were in order it could be better.... Currently though , you can see they are sortable and end up alphabetical by permission, with the path being last in sort order and not significant for items not having a 'path' member.

>>> from pprint import pp; from irods.access import iRODSAccess,ACLOperation
>>> pp(sorted([ACLOperation('read_metadata','dan'),iRODSAccess('read_metadata','/t','bob'),ACLOperation('read','alice'), 
  iRODSAccess('write','/t','alice'),ACLOperation('own','rods')])) 
[<ACLOperation: access_name='own' user_name='rods' user_zone=''>,
 <ACLOperation: access_name='read' user_name='alice' user_zone=''>,
 <iRODSAccess read_metadata /t bob >,
 <ACLOperation: access_name='read_metadata' user_name='dan' user_zone=''>,
 <iRODSAccess write /t alice >]

return hash((self.access_name, iRODSPath(self.path), self.user_name, self.user_zone))

def copy(self, decanonicalize=False):
def copy(self, decanonicalize=False, implied_zone=''):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is implied_zone?
Do we expect users of the PRC to ever use this parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's useful for comparison purposes if you can tell __eq__ that a null length zone field implies the current zone name.

Copy link
Contributor

Choose a reason for hiding this comment

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

When would that case appear? Can you show an example?

Copy link
Collaborator Author

@d-w-moore d-w-moore Mar 25, 2026

Choose a reason for hiding this comment

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

Here's one:

>>> from irods.access import ACLOperation
>>> session = irods.helpers.make_session()
>>> coll = session.collections.get('/tempZone/home/rods')
>>>   
>>> # -- set a possible ACL operation for test/request
>>> aclo=ACLOperation('read_object', 'public')
>>> from pprint import pp
>>> # -- request current acls on the object
>>> got = session.acls.get(coll)
>>> pp(got)
[<iRODSAccess own /tempZone/home/rods rods(rodsadmin) tempZone>, 
 <iRODSAccess read_object /tempZone/home/rods public(rodsgroup) tempZone>]
>>>
# Failed attempt to see if acl already set (via direct equality test)
>>> aclo == got[1] 
False
>>>
>>> modified_acl = aclo.copy(implied_zone='tempZone')
>>> modified_got = got[1].copy(implied_zone='tempZone')
>>>
>>> # Equality test now works due to zone "normalization" 
>>> modified_acl == modified_got
True
>>> # therefore we'll also get:
>>> modified_acl in [acl.copy(implied_zone='tempZone') for acl in got]
True

Copy link
Contributor

Choose a reason for hiding this comment

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

Is calling <object>.copy(implied_zone=<zone>) required for equality testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Letting it ride.

Resolving.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could be over-engineering this...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine. We discussed it and reached a decision.

We can always change things later if someone finds a good reason for us to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I realize it causes confusion or at least a double take .... If you think it might need a readme section, just say it and I can write up a short one with example.

Copy link
Contributor

Choose a reason for hiding this comment

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

A simple example showing the capability is all that's needed, I think.

Co-authored-by: Kory Draughn <korydraughn@ymail.com>
irods/access.py Outdated
Comment on lines +94 to +99
def __lt__(self, other):
return (
self.access_name < other.access_name
and self.user_name < other.user_name
and self.user_zone < other.user_zone
) and iRODSPath(self.path) < iRODSPath(other.path)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for drawing my attention to it, as it was improperly defined.

Yes, it maybe should have a comment at least.

It mainly just gives the class a defined sorting order. Now, if someone attempts pretty-printing a mixed list of these items, they'll get something the eye is somewhat able to follow, although I guess if the two formats were more consistent and the permissions were in order it could be better.... Currently though , you can see they are sortable and end up alphabetical by permission, with the path being last in sort order and not significant for items not having a 'path' member.

>>> from pprint import pp; from irods.access import iRODSAccess,ACLOperation
>>> pp(sorted([ACLOperation('read_metadata','dan'),iRODSAccess('read_metadata','/t','bob'),ACLOperation('read','alice'), 
  iRODSAccess('write','/t','alice'),ACLOperation('own','rods')])) 
[<ACLOperation: access_name='own' user_name='rods' user_zone=''>,
 <ACLOperation: access_name='read' user_name='alice' user_zone=''>,
 <iRODSAccess read_metadata /t bob >,
 <ACLOperation: access_name='read_metadata' user_name='dan' user_zone=''>,
 <iRODSAccess write /t alice >]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants