[WIP] Rework of Storages Registry based approach #17881
Conversation
- Working to makes test pass - Slow removal of already migrated code
mereghost
force-pushed
the
refactor/redesign-storage-interactions
branch
from
d6231a3 to
200fd3a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...c/common/storages/adapters/providers/nextcloud/commands/copy_template_folder_command_spec.rb
Dismissed
Show dismissed
Hide dismissed
modules/storages/spec/services/storages/nextcloud_managed_folder_sync_service_spec.rb
Dismissed
Show dismissed
Hide dismissed
modules/storages/spec/services/storages/nextcloud_managed_folder_sync_service_spec.rb
Dismissed
Show dismissed
Hide dismissed
modules/storages/spec/services/storages/nextcloud_managed_folder_sync_service_spec.rb
Dismissed
Show dismissed
Hide dismissed
| class StrategyContract < Dry::Validation::Contract | ||
| params do | ||
| # The included list need to be made dynamic | ||
| required(:key).filled(:symbol, included_in?: %i[noop basic_auth oauth_client_credentials oauth_user_token]) |
There was a problem hiding this comment.
Goood question. Ideally we could have this somewhere and just reference it. XD
|
|
||
| # TODO: Add tests for this - 2025-01-15 @mereghost | ||
| def self.authorization_state(storage:, user:) | ||
| auth_strategy = Input::Strategy.build(user:, key: :oauth_user_token) |
There was a problem hiding this comment.
this is still a result at this point. this is missing error handling, right?
Or, you could now probably build a real monadic chain, right? As I see two results - the strategy.build and the auth_check.call.
There was a problem hiding this comment.
That's the idea, the monadic chain.
The code that's asking for this will be touched soon, so I'll have to take a better look at it.
There was a problem hiding this comment.
🟢 the rework of the strategies looks quite nice
| include Dry::Monads::Result(Results::Error) | ||
|
|
||
| def self.call(storage:, auth_strategy:, input_data:) | ||
| new(storage).call(auth_strategy: auth_strategy, input_data: input_data) |
There was a problem hiding this comment.
🟡 shorthand for parameters in .call?
There was a problem hiding this comment.
🤦🏼 Somehow I missed this.
There was a problem hiding this comment.
🟢 nice, so the base class replaces the util class
There was a problem hiding this comment.
❓ Why did you dissolve the idea of the internal namespace? Now I'd stumble across the fact, that the propfind query has a fully different interface then its brethren. It even overrides self.call and stuff like this.
There was a problem hiding this comment.
It wasn't a conscious choice... it kinda happened. I played with the idea of making the interface the same but never fully committed to it.
They aren't exposed by the registry, so the interface is a lesser issue to me. I might still reintroduce the namespace.
This is a large chunk of code. No, it can't be done piecemeal.
It can be reviewed piecemeal tho. :P
What is this?
For a long time we experimented and introduced ideas to the
Storagescodebase that were left half-baked, half implemented or outright abandoned.This work tries to extrapolate these ideas and move forward with this type of implementation.
Why?
Storagescode will forever get more complexity as we support more and more features and should, by all means, be invisible to the outside. Outside code should just call a couple of services and work with their results using the already familiarServiceResultabstraction.Module internal code should need to keep this level of promises. Besides we came up with a bunch of other abstractions (input data, result objects) that we never ended up implementing for all the cases.
What has changed?
For storages client code? Hopefully nothing.
Storagesmodule has been radically transformed tho.