Skip to content
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

Make dict views behave like their unrestricted versions #147

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

perrinjerome
Copy link
Contributor

@perrinjerome perrinjerome commented Mar 8, 2024

unlike the restricted versions, the unrestricted versions:

  • are not iterators, they are views
  • have a len
  • are false when the mapping is empty, true otherwise
  • are instances of collections.abc.MappingView

This change aligns the behavior of restricted versions with the behavior of unrestricted one, while keeping the "guard", which validates the access with the security manager.

During this refactoring, also change .items() to validate each keys and values, like .keys() and .values() do.

@perrinjerome perrinjerome marked this pull request as ready for review March 8, 2024 07:28
@perrinjerome
Copy link
Contributor Author

What was most problematic for me when porting a python2 code base was constructs like:

d = {}
...
if len(d.keys()):
  ...

which was causing an error "SafeIter has no len" and

d = {}
...
if d.keys():
  ...

which was silently true when d is empty.

Most of the actual implementation is delegated to collections.abc, so the code is not so complex.

@perrinjerome perrinjerome marked this pull request as draft March 8, 2024 12:51
@perrinjerome
Copy link
Contributor Author

There was something wrong with the first version of the patch, it is now ok.

@perrinjerome perrinjerome marked this pull request as ready for review March 9, 2024 09:55
@icemac icemac requested review from dataflake and d-maurer March 11, 2024 07:28
Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

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

Looks fine, but then again I do not have any deep knowledge about the code.

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

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

Even though I now know how to access branches in foreign repositories (thanks to @navytux), I would prefer if you would make your future PRs directly in the target repository.

src/AccessControl/ZopeGuards.py Show resolved Hide resolved
src/AccessControl/ZopeGuards.py Show resolved Hide resolved
@perrinjerome perrinjerome requested a review from d-maurer March 11, 2024 14:11
Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

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

Maybe, you consider adding my comment suggestion.

src/AccessControl/ZopeGuards.py Show resolved Hide resolved
@icemac
Copy link
Member

icemac commented Mar 13, 2024

I think appveyor breaks because this PR is created from a fork of the repository instead of a branch. Could you please recreate it on a branch inside the zopefoundation repository to see that also the windows version runs successfully.

(As a rule of thumb: Creating branches in the main repostory (instead of forks) is running smoother and is liked more by the zopefoundation maintainers.)

unlike the restricted versions, the unrestricted versions:
 - are not iterators, they are views
 - have a len
 - are false when the mapping is empty, true otherwise
 - are instances of collections.abc.MappingView

This change aligns the behavior of restricted versions with the behavior
of unrestricted one, while keeping the "guard", which validates the
access with the security manager.

During this refactoring, also change `.items()` to validate
ach keys and values, like `.keys()` and `.values()` do.

Co-authored-by: Dieter Maurer <[email protected]>
@perrinjerome
Copy link
Contributor Author

Thank you @icemac I made a new pull request from zopefoundation with the same changes #148

@perrinjerome perrinjerome merged commit 82ab107 into zopefoundation:master Mar 13, 2024
67 checks passed
@perrinjerome
Copy link
Contributor Author

After some time the appveyor build was OK, so I merged, thank you everybody !

@perrinjerome
Copy link
Contributor Author

After some time the appveyor build was OK, so I merged, thank you everybody !

actually, it was probably not "after some time", but after I pushed the same commit in zopefoundation/AccessControl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants