-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Controlled iteration order for interfaces #7609
Conversation
@wardi, it's ready for review |
@@ -124,7 +124,7 @@ def get_user_from_token(token: str, | |||
# do preprocessing in reverse order, allowing onion-like | |||
# "unwrapping" of the data, added during postprocessing, when | |||
# token was created | |||
for plugin in _get_plugins(): | |||
for plugin in reversed(list(_get_plugins())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one back to reversing the old way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two sides to processing API tokens. When it's created and encoded into a string, all plugins can add additional properties to the token or modify existing properties. When the application receives the token, it decodes the token from a string and extracts all the properties. It sounds like a middleware for an API token, that's why data extraction should happen in reverse order compared to data injection.
This line used reversed
originally, but I misinterpret it and set reverse_order to the whole interface. After additional checks(in order to add a comment with the reason behind the reverse order for the interface), I realized, that this interface has 4 methods and 3 of them are executed in normal order and only data extraction from the token received from user happens in reverse order. That's why I restored APIToken to its original form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, can you add the reason that the reversed plugin order can't be used here to the comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done
Add
ckan_reverse_iteration_order
flag to the Interface class. Whenever it is set toTrue
,PluginImplementations
will return plugins implementing this interface in reverse order.It reduces the risk of writing code where iteration goes not in the expected order. In addition, it makes more explicit, which interfaces are iterated in reverse order.
Finally(but I'd rather not mention it in the docs), it allows you to write
IActions.ckan_reverse_iteration_order = True
in order to revert the order of plugins during action registration(just an example, of course, you can do it with any interface). It may save someone who wants to patch things locally if we decide to change the order for a particular interface in the future or discover, that order was non-intentionally changedInterfaces that are traversed in reverse order: