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

Cleanup storage interface #28996

Closed
jvillafanez opened this issue Sep 13, 2017 · 2 comments
Closed

Cleanup storage interface #28996

jvillafanez opened this issue Sep 13, 2017 · 2 comments

Comments

@jvillafanez
Copy link
Member

current problems:

  • We have, at least, 3 different storage interfaces:
    • \OCP\Files\Storage\IStorage which is supposed to be the one that should be implemented
    • \OCP\Files\Storage, the old one, which extends the new one even though it has more or less the same methods defined (?). This should be deleted. It has been marked as deprecated in 9.0.0.
    • \OC\Files\Storage\Storage. I don't know why this is used.
  • There are some methods in the \OCP\Files\Storage\IStorage class using \OCP\Files\Storage objects. This will need to be changed.
  • The \OC\Files\Storage\Common implements \OC\Files\Storage\Storage, but it should implement the public interface
  • The \OCP\Files\Storage\ILockingStorage should extend \OCP\Files\Storage\IStorage because it's a specialization of the interface. Having the interface isolated doesn't make sense.
  • \OCP\Files\Storage\StorageAdapter extends \OC\Files\Storage\Common so others can extend from there instead of the common one. This isn't really bad but it feels like a patch.

expectations

  • Only one interface. \OCP\Files\Storage\IStorage should fit as long as it has all the methods required. The rest of the interface should be removed, and classes that use them should adjust the code.
  • The methods should require \OCP\Files\Storage\IStorage interface, and only that one.
  • The \OC\Files\Storage\Common should be removed. The methods should be moved to the \OCP\Files\Storage\StorageAdapter, which is public.
  • The \OCP\Files\Storage\StorageAdapter will implement the \OCP\Files\Storage\IStorage interface, maybe with some abstract methods. All storage implementations are expected to inherit from \OCP\Files\Storage\StorageAdapter class. Special cases might implement the \OCP\Files\Storage\IStorage interface directly.
  • As said, \OCP\Files\Storage\ILockingStorage should inherit from \OCP\Files\Storage\IStorage. Note that core will take care of 2 (or more) interfaces, which might be already happening. We might consider to move the locking methods to the \OCP\Files\Storage\IStorage interface if core requires locking mechanisms (no need to have 2 interface if only one will be usable)
  • Consider to remove the \OCP\Files\Storage\IStorage interface to use only the "new" / "to-be-created" \OCP\Files\Storage\StorageAdapter as an abstract class. Adding new common or default functionality could be easier without bothering the implementations needlessly.

Any other expectation? Do we agree on this? How far are we from the expectations? How can we move from the current state to the one we (or at least I) expect?

I've only scratched the surface, so there might be more things that we could change, or other things that might prevent that change. So far, the proposed changes seem feasible to do, but we still should analize all the cases to decide how to proceed.

@PVince81 @DeepDiver1975 for further discussion.

@jvillafanez jvillafanez added this to the triage milestone Sep 13, 2017
@PVince81
Copy link
Contributor

@jvillafanez I think the expectations make sense.

I think @butonic had a ticket a while ago where he tried to use IStorage but a lot of stuff broke.
We need to attempt this again at some point.

@PVince81
Copy link
Contributor

@butonic reuse this ticket to post the hackweek discussions summary ?

There were lots of ITree, IBlobStorage, etc interfaces

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

No branches or pull requests

4 participants