scijava / scijava-common

A plugin framework and application container with built-in extensibility mechanism :electric_plug:
BSD 2-Clause "Simplified" License
85 stars 53 forks source link

Creating DataHandle<Location> from FileLocation and BytesLocation #468

Closed Daniel-Alievsky closed 11 months ago

Daniel-Alievsky commented 11 months ago

Most popular situations, when we need DataHandle, are processing files and byte arrays, implemented in the classes FileHandle and BytesHandle. Unfortunately, the only correct way to create the necessary objects is using SciHava context technique:

context.getService(DataHandleService.class).create(location)

But initializing context sub-system requires time and special code. I would be very desirable to avoid Context class in these very simple situation.

I've found necessary solution as a pair of simple functions:

    @SuppressWarnings("rawtypes, unchecked")
    public static DataHandle<Location> getFileHandle(FileLocation fileLocation) {
        Objects.requireNonNull(fileLocation, "Null fileLocation");
        FileHandle fileHandle = new FileHandle();
        fileHandle.set(fileLocation);
        return (DataHandle) fileHandle;
    }

    @SuppressWarnings("rawtypes, unchecked")
    public static DataHandle<Location> getBytesHandle(BytesLocation bytesLocation) {
        Objects.requireNonNull(bytesLocation, "Null bytesLocation");
        BytesHandle bytesHandle = new BytesHandle();
        bytesHandle.set(bytesLocation);
        return (DataHandle) bytesHandle;
    }

Could you provide such convenient methods inside FileLocation/ByteLocation classes?

Besides, you see that it turned out to be necessary to use @SupressWarning to make necessary DataHandle<Location>. Maybe you should rework generics in your module to provide ability to return DataHandle<Location> without compiler warnings? Or, at least, please make the methods getFileHandle/getBytesHandle with @SupressWarning a part of your own classes, to avoid your users to write unsafe code.

It is a duplicate of the issue https://github.com/scifio/scifio/issues/508 - it seems here is more suitable place. So you may close that issue in SCIFIO project.

ctrueden commented 11 months ago

you see that it turned out to be necessary to use @SupressWarning to make necessary DataHandle<Location>.

I fixed a generics typing issue with ReadBufferDataHandle (b3b432672d7594a74ac6138ab5e137a5e96eb787) as part of #497 bug-fix, and the 2.95.1 release contains this update. So the SuppressWarnings should no longer be necessary, because you can simply do:

    public static FileHandle getFileHandle(FileLocation fileLocation) {
        Objects.requireNonNull(fileLocation, "Null fileLocation");
        FileHandle fileHandle = new FileHandle();
        fileHandle.set(fileLocation);
        return fileHandle;
    }

    public static BytesHandle getBytesHandle(BytesLocation bytesLocation) {
        Objects.requireNonNull(bytesLocation, "Null bytesLocation");
        BytesHandle bytesHandle = new BytesHandle();
        bytesHandle.set(bytesLocation);
        return bytesHandle;
    }

And then pass the returned DataHandle of whatever subclass directly to the ReadBufferDataHandle constructor type-safely.

I think your approach to avoid creating a Context here is correct (it's what I did also while developing the fix for #467), but these utility methods are more verbose than necessary; it's only constructing the object and then calling set, so I just added a new constructor signature to FileHandle and ByteHandle accepting the location to wrap directly: 0f0cc605dc93ca99b934089df4d8ced68798d380, and released scijava-common 2.96.0. I also added the same for URLHandle, part of scijava-io-http (scijava/scijava-io-http@a5b8a1770a2c2f9074d2aa1550afe2c03ca30426), and released scijava-io-http 0.3.0, in case this is useful to you or others.

Daniel-Alievsky commented 11 months ago

Unfortunately, it is not enough! In your SCIFIO TiffParser code, "in" stream is declared as DataHandle<Location> in And the same type is returned by getStream() method:

    /** Gets the stream from which TIFF data is being parsed. */
    public DataHandle<Location> getStream() {
        return in;
    }

I cannot change the returning type of this method, for example, to DataHandle<? extends Location>, because it is used in other classes, for example, TIFFFormat. Probably other projects also use this method.

It is obviously a fundamental problem of your generics architecture. I've described it in another issue: https://github.com/scijava/scijava-common/issues/469 My unsafe method is just a workaround, but it is a bad workaround - it "hides" the problem, not resolves it. But, in any case, it is not worse that the existing behavior.