bioinformatics-ua / dicoogle

Dicoogle - Open Source PACS
http://www.dicoogle.com/
GNU General Public License v3.0
445 stars 132 forks source link

Revisiting the deprecation of StorageInterface#handles #692

Closed Enet4 closed 1 month ago

Enet4 commented 4 months ago

In 2016, the method handles in StorageInterface was proposed to be deprecated in #257. The rationale was that there was only single reasonable implementation of it, and there was at least one case in the wild of an incorrect implementation.

The deprecation and default implementation was effectively applied in #280 for Dicoogle 3 in 2018, and since then we have also updated the Dicoogle learning pack to advise against using and implementing the method:

handles(URI) is a deprecated method, which was made to verify whether the given URI can relate to an item (existent or inexistent) in this storage. Since this was meant to be based uniquely on the scheme of the URI, implementers should ignore it, and consumers should instead check for equality of the scheme:

boolean handles = Objects.equals(storageInterface.getScheme(), uri.getScheme());

Per the reasoning in #257, this check was envisioned to be based exclusively on an exact match of the URI scheme against the scheme of the storage provider implementation (StorageInterface#getScheme()). But now a different use case arose:

handles should be in place. For instance in case we want to keep blob:// or blob+ssl://.

_Originally posted by @bastiao in https://github.com/bioinformatics-ua/dicoogle/pull/691#discussion_r1613087590_

Discussion is open.

Enet4 commented 1 month ago

I say we decide what to do here and bring the necessary changes upstream for v4.0.0.

The way I see it, we can un-deprecate it and let developers customize the handles logic a bit, while keeping the default implementation that tests the scheme for equality. This should hopefully prevent the original issue in #257 from happening again.

bastiao commented 1 month ago

That sounds good to me. Do you take care of it?