You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sis.apache.org by de...@apache.org on 2022/01/06 01:20:18 UTC
[sis] 05/07: Avoid wrapping `BufferedReader` if possible in `DataStoreProvider.probeContent(…)`. This is for allowing users to invoke `BufferedReader.readLine()`.
This is an automated email from the ASF dual-hosted git repository.
desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git
commit cbc555d3db045b2f5118971dcda922869857726d
Author: Martin Desruisseaux <ma...@geomatys.com>
AuthorDate: Wed Jan 5 21:17:22 2022 +0100
Avoid wrapping `BufferedReader` if possible in `DataStoreProvider.probeContent(…)`.
This is for allowing users to invoke `BufferedReader.readLine()`.
---
.../org/apache/sis/internal/storage/csv/Store.java | 10 +-
.../internal/storage/io/RewindableLineReader.java | 104 +++++++++++++++++++--
.../org/apache/sis/storage/DataStoreProvider.java | 15 ++-
.../org/apache/sis/storage/ProbeInputStream.java | 1 +
.../java/org/apache/sis/storage/ProbeReader.java | 4 +
.../apache/sis/storage/DataStoreProviderTest.java | 68 +++++++++++---
6 files changed, 170 insertions(+), 32 deletions(-)
diff --git a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/csv/Store.java b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/csv/Store.java
index 9829e26..a89e5fe 100644
--- a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/csv/Store.java
+++ b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/csv/Store.java
@@ -66,7 +66,6 @@ import org.apache.sis.storage.DataOptionKey;
import org.apache.sis.storage.DataStoreException;
import org.apache.sis.storage.DataStoreContentException;
import org.apache.sis.storage.DataStoreReferencingException;
-import org.apache.sis.storage.UnsupportedStorageException;
import org.apache.sis.storage.StorageConnector;
import org.apache.sis.storage.FeatureSet;
import org.apache.sis.setup.OptionKey;
@@ -235,12 +234,7 @@ final class Store extends URIDataStore implements FeatureSet {
*/
public Store(final StoreProvider provider, final StorageConnector connector) throws DataStoreException {
super(provider, connector);
- final Reader r = connector.getStorageAs(Reader.class);
- connector.closeAllExcept(r);
- if (r == null) {
- throw new UnsupportedStorageException(super.getLocale(), StoreProvider.NAME,
- connector.getStorage(), connector.getOption(OptionKey.OPEN_OPTIONS));
- }
+ final Reader r = connector.commit(Reader.class, StoreProvider.NAME);
source = (r instanceof BufferedReader) ? (BufferedReader) r : new LineNumberReader(r);
geometries = Geometries.implementation(connector.getOption(OptionKey.GEOMETRY_LIBRARY));
dissociate = FoliationRepresentation.FRAGMENTED.equals(connector.getOption(DataOptionKey.FOLIATION_REPRESENTATION));
@@ -317,6 +311,8 @@ final class Store extends URIDataStore implements FeatureSet {
* which is set after the last header line. If the mark is no longer valid, then we have to create a new line reader.
* In this later case, we have to skip the header lines (i.e. we reproduce the constructor loop, but without parsing
* metadata).
+ *
+ * @todo Not yet used. This is planned for a future version of {@link #features(boolean)} method implementation.
*/
private void rewind() throws IOException {
final BufferedReader reader = source;
diff --git a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/io/RewindableLineReader.java b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/io/RewindableLineReader.java
index 7fe6ce4..2e9c49a 100644
--- a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/io/RewindableLineReader.java
+++ b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/io/RewindableLineReader.java
@@ -21,6 +21,7 @@ import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.LineNumberReader;
import java.nio.charset.Charset;
+import org.apache.sis.util.resources.Errors;
import org.apache.sis.io.InvalidSeekException;
@@ -35,10 +36,11 @@ import org.apache.sis.io.InvalidSeekException;
* </ul>
*
* @author Martin Desruisseaux (Geomatys)
- * @version 0.8
+ * @version 1.2
* @since 0.8
* @module
*/
+@SuppressWarnings("SynchronizeOnNonFinalField")
public final class RewindableLineReader extends LineNumberReader {
/**
* Size of the buffer, in number of characters.
@@ -59,6 +61,15 @@ public final class RewindableLineReader extends LineNumberReader {
private final Charset encoding;
/**
+ * Whether calls to {@link #mark(int)} and {@link #reset()} should throw {@link IOException}.
+ * This is initially {@code false} and may be set to {@code true} if the caller wants to block
+ * users from overwriting the mark (s)he just did. This flag changes also the {@link #close()}
+ * behavior. It is used for probing the content of the same file by different data stores and we
+ * want a safety against implementations that do not follow the {@code probeContent(…)} contract.
+ */
+ private boolean isMarkProtected;
+
+ /**
* Creates a line reader wrapping the given input stream.
*
* @param input the input stream from which to read characters.
@@ -72,13 +83,13 @@ public final class RewindableLineReader extends LineNumberReader {
this.input = (InputStreamAdapter) input;
}
this.encoding = encoding;
- super.mark(BUFFER_SIZE);
/*
* By default, this.lock is set to InputStreamReader. But InputStreamReader.lock has itself
* been set on the given InputStreamAdapter. So we set this.lock to InputStreamReader.lock
* in order to have a single synchronization lock.
*/
lock = input;
+ super.mark(BUFFER_SIZE);
}
/**
@@ -89,11 +100,10 @@ public final class RewindableLineReader extends LineNumberReader {
* @return the reader to use for next read operation (may be {@code this}).
* @throws IOException if an error occurred while rewinding the reader.
*/
- @SuppressWarnings("SynchronizeOnNonFinalField")
public RewindableLineReader rewind() throws IOException {
synchronized (lock) {
try {
- reset();
+ super.reset();
return this;
} catch (IOException e1) {
final InputStreamAdapter stream = input;
@@ -114,8 +124,8 @@ public final class RewindableLineReader extends LineNumberReader {
stream.keepOpen = false;
}
/*
- * Try to seek to the data origin. Note that 'seek(0)' below does not necessarily
- * move to the beginning of file, since ChannelDataInput may contain an offset.
+ * Try to seek to the data origin. Note that `seek(0)` below does not necessarily
+ * move to the beginning of file, because `ChannelDataInput` may contain an offset.
*/
try {
stream.input.seek(0);
@@ -129,16 +139,92 @@ public final class RewindableLineReader extends LineNumberReader {
}
/**
- * Closes this reader.
+ * Marks current stream position and blocks all subsequent calls to {@link #mark(int)},
+ * {@link #reset()} and {@link #close()} until {@link #protectedReset()} is invoked.
+ *
+ * @throws IOException if the stream can not be marked.
+ */
+ public final void protectedMark() throws IOException {
+ synchronized (lock) {
+ super.mark(BUFFER_SIZE);
+ isMarkProtected = true;
+ }
+ }
+
+ /**
+ * Stops the protection given by {@link #protectedMark()} and reset the stream to its marked position.
+ *
+ * @throws IOException if the stream can not be reset.
+ */
+ public final void protectedReset() throws IOException {
+ synchronized (lock) {
+ isMarkProtected = false;
+ super.reset();
+ }
+ }
+
+ /**
+ * Tells whether this stream supports the mark and reset operations.
+ * This is {@code true} by default but can be set to {@code false}
+ * if the mark is reserved to internal usage.
+ *
+ * @return true if this stream supports the mark operation.
+ */
+ @Override
+ public boolean markSupported() {
+ synchronized (lock) {
+ return !isMarkProtected;
+ }
+ }
+
+ /**
+ * Marks the present position in the stream if allowed to do so.
+ * If {@link #isMarkProtected} is {@code true}, then this method throws an exception.
+ *
+ * @param readlimit limit on the number of characters that may be read.
+ * @throws IOException if the stream can not be marked.
+ */
+ @Override
+ public void mark(final int readlimit) throws IOException {
+ synchronized (lock) {
+ if (isMarkProtected) {
+ throw new IOException(Errors.format(Errors.Keys.UnsupportedOperation_1, "mark"));
+ }
+ super.mark(readlimit);
+ }
+ }
+
+ /**
+ * Resets the stream if allowed to do so.
+ * If {@link #isMarkProtected} is {@code true}, then this method throws an exception.
+ *
+ * @throws IOException if the stream can not be reset.
+ */
+ @Override
+ public void reset() throws IOException {
+ synchronized (lock) {
+ if (isMarkProtected) {
+ throw new IOException(Errors.format(Errors.Keys.UnsupportedOperation_1, "reset"));
+ }
+ super.reset();
+ }
+ }
+
+ /**
+ * Closes this reader. The underlying stream will be either reset or closed
+ * depending on the {@link #isMarkProtected} mode.
*
* @throws IOException if an error occurred while closing the reader.
*/
@Override
- @SuppressWarnings("SynchronizeOnNonFinalField")
public void close() throws IOException {
synchronized (lock) {
input = null;
- super.close();
+ if (isMarkProtected) {
+ super.reset();
+ } else {
+ super.close();
+ }
}
}
}
diff --git a/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreProvider.java b/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreProvider.java
index c3c927f..235ca30 100644
--- a/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreProvider.java
+++ b/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreProvider.java
@@ -27,6 +27,7 @@ import org.opengis.metadata.distribution.Format;
import org.apache.sis.internal.simple.SimpleFormat;
import org.apache.sis.internal.storage.URIDataStore;
import org.apache.sis.internal.storage.io.Markable;
+import org.apache.sis.internal.storage.io.RewindableLineReader;
import org.apache.sis.metadata.iso.citation.DefaultCitation;
import org.apache.sis.metadata.iso.distribution.DefaultFormat;
import org.apache.sis.measure.Range;
@@ -392,12 +393,22 @@ public abstract class DataStoreProvider {
stream.seek(position);
} else if (input instanceof InputStream) {
/*
- * `InputStream` supports at most one mark. So we keep it for ourselve
- * and wrap the stream in an object that prevent user from using marks.
+ * `InputStream` supports at most one mark. So we keep it for ourselves
+ * and wrap the stream in an object that prevent users from using marks.
*/
final ProbeInputStream stream = new ProbeInputStream(connector, (InputStream) input);
result = prober.test(type.cast(stream));
stream.close(); // Reset (not close) the wrapped stream.
+ } else if (input instanceof RewindableLineReader) {
+ /*
+ * `Reader` supports at most one mark. So we keep it for ourselves and prevent users
+ * from using marks, but without wrapper if we can safely expose a `BufferedReader`
+ * (because users may want to use the `BufferedReader.readLine()` method).
+ */
+ final RewindableLineReader r = (RewindableLineReader) input;
+ r.protectedMark();
+ result = prober.test(input);
+ r.protectedReset();
} else if (input instanceof Reader) {
final Reader stream = new ProbeReader(connector, (Reader) input);
result = prober.test(type.cast(stream));
diff --git a/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeInputStream.java b/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeInputStream.java
index df1448e..a32c4f0 100644
--- a/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeInputStream.java
+++ b/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeInputStream.java
@@ -26,6 +26,7 @@ import org.apache.sis.internal.storage.Resources;
/**
* A temporary input stream used for probing purposes.
* This stream does not allow mark/reset operations because the mark is reserved for this class.
+ * The {@link #close()} method closes this stream but not the wrapped stream, which is only reset.
*
* @author Martin Desruisseaux (Geomatys)
* @version 1.2
diff --git a/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeReader.java b/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeReader.java
index d65db0d..de869fa 100644
--- a/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeReader.java
+++ b/storage/sis-storage/src/main/java/org/apache/sis/storage/ProbeReader.java
@@ -26,6 +26,10 @@ import org.apache.sis.internal.storage.Resources;
/**
* A temporary character reader used for probing purposes.
* This reader does not allow mark/reset operations because the mark is reserved for this class.
+ * The {@link #close()} method closes this reader but not the wrapped reader, which is only reset.
+ *
+ * <p>Note: this wrapper is not used if the reader is an instance of
+ * {@link org.apache.sis.internal.storage.io.RewindableLineReader}.</p>
*
* @author Martin Desruisseaux (Geomatys)
* @version 1.2
diff --git a/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoreProviderTest.java b/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoreProviderTest.java
index f2a4d5e..c311793 100644
--- a/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoreProviderTest.java
+++ b/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoreProviderTest.java
@@ -17,9 +17,12 @@
package org.apache.sis.storage;
import java.io.Reader;
+import java.io.BufferedReader;
import java.io.InputStream;
import java.io.IOException;
+import java.io.InputStreamReader;
import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
import org.opengis.parameter.ParameterDescriptorGroup;
import org.apache.sis.test.DependsOn;
import org.apache.sis.test.TestCase;
@@ -116,7 +119,7 @@ public final strictfp class DataStoreProviderTest extends TestCase {
*/
@Test
public void testProbeWithURL() throws DataStoreException {
- testProbe(false);
+ testProbeWithInputStream(false);
}
/**
@@ -127,13 +130,13 @@ public final strictfp class DataStoreProviderTest extends TestCase {
*/
@Test
public void testProbeWithInputStream() throws DataStoreException {
- testProbe(true);
+ testProbeWithInputStream(true);
}
/**
* Implementation of {@link #testProbeWithURL()} and {@link #testProbeWithInputStream()}.
*/
- private void testProbe(final boolean asStream) throws DataStoreException {
+ private void testProbeWithInputStream(final boolean asStream) throws DataStoreException {
/*
* Read a few bytes and verify that user can not overwrite the mark.
*/
@@ -168,11 +171,57 @@ public final strictfp class DataStoreProviderTest extends TestCase {
*/
@Test
public void testProbeWithReader() throws DataStoreException {
+ final StorageConnector connector = testProbeWithReader(false);
+ /*
+ * Attempt to read with `InputStream` should not be possible because
+ * the connector does not know the original stream (we wrapped it).
+ */
+ try {
+ verifyProbeWithInputStream(connector);
+ fail("Operation should not be allowed.");
+ } catch (ForwardOnlyStorageException e) {
+ final String message = e.getMessage();
+ assertTrue(message, message.contains("InputStreamReader"));
+ }
+ }
+
+ /**
+ * Verifies that {@link DataStoreProvider#probeContent(StorageConnector, Class, Prober)}
+ * with a {@link BufferedReader} leaves the position of the original stream unchanged.
+ *
+ * @throws DataStoreException if an error occurred while using the storage connector.
+ */
+ @Test
+ public void testProbeWithBufferedReader() throws DataStoreException {
+ final StorageConnector connector = testProbeWithReader(true);
+ /*
+ * Get the input in another form (InputStream) and verifies that we still
+ * have the same first characters. Then try again reading with a `Reader`.
+ * The intent is to verify that it is not corrupted by `InputStream` use.
+ */
+ verifyProbeWithInputStream(connector);
+ verifyProbeWithReader(connector);
+ }
+
+ /**
+ * Implementation of {@link #testProbeWithReader()} and {@link #testProbeWithBufferedReader()}.
+ * If {@code buffered} is {@code true}, this method will creates itself a non-buffered reader
+ * (because reader created by {@link StorageConnector} are buffered by default).
+ *
+ * @param buffered whether to use a buffered reader.
+ * @return the storage connector created by this method, for allowing caller to do more tests.
+ */
+ private StorageConnector testProbeWithReader(final boolean buffered) throws DataStoreException {
+ StorageConnector connector = StorageConnectorTest.create(true);
+ if (!buffered) {
+ final InputStream stream = (InputStream) connector.getStorage();
+ connector = new StorageConnector(new InputStreamReader(stream, StandardCharsets.US_ASCII));
+ }
/*
* Read a few bytes and verify that user can not overwrite the mark.
*/
- final StorageConnector connector = StorageConnectorTest.create(true);
assertEquals(provider.probeContent(connector, Reader.class, stream -> {
+ assertEquals(buffered, stream instanceof BufferedReader);
assertFalse(stream.markSupported());
stream.skip(5);
try {
@@ -188,15 +237,6 @@ public final strictfp class DataStoreProviderTest extends TestCase {
* beginning of the file despite above reading of some bytes.
*/
verifyProbeWithReader(connector);
- /*
- * Get the input in another form (InputStream) and verifies
- * that we still have the same first characters.
- */
- verifyProbeWithInputStream(connector);
- /*
- * Try again reading with a `Reader`. The intent is to verify
- * that it is not corrupted by above use of `InputStream`.
- */
- verifyProbeWithReader(connector);
+ return connector;
}
}