Copilot commented on code in PR #3564:
URL: https://github.com/apache/tika/pull/3564#discussion_r4524528541
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/onenote/OneNoteDirectFileResource.java:
##########
@@ -23,60 +23,89 @@
import java.nio.ByteBuffer;
/**
- * This is copied mostly from the the
- * former org.apache.tika.parser.mp4.DirectFileReadDataSource
- * <p>
- * Implements a simple way to encapsulate a {@link
org.apache.tika.io.TikaInputStream} that you
- * will have to seek,read,repeat while parsing OneNote contents.
+ * Seekable reads over a OneNote file. The parser mostly reads a few bytes at
a time, so reads
+ * come from a window of the file rather than one system call each.
*/
class OneNoteDirectFileResource implements Closeable {
- private static final int TRANSFER_SIZE = 8192;
+ private static final int WINDOW_SIZE = 64 * 1024;
private final RandomAccessFile raf;
+ private final long size;
+ private final byte[] window = new byte[WINDOW_SIZE];
+ // file offset of window[0], and how many bytes of the window are valid
+ private long windowStart;
+ private int windowLength;
+ private long position;
public OneNoteDirectFileResource(File f) throws IOException {
this.raf = new RandomAccessFile(f, "r");
+ this.size = raf.length();
}
public int read() throws IOException {
- return raf.read();
+ if (position >= size) {
+ return -1;
+ }
+ if (!inWindow(position)) {
+ fill(position);
+ }
+ return window[(int) (position++ - windowStart)] & 0xFF;
}
public int read(ByteBuffer byteBuffer) throws IOException {
int len = byteBuffer.remaining();
int totalRead = 0;
- int bytesRead = 0;
- byte[] buf = new byte[TRANSFER_SIZE];
- while (totalRead < len) {
- int bytesToRead = Math.min((len - totalRead), TRANSFER_SIZE);
- bytesRead = raf.read(buf, 0, bytesToRead);
- if (bytesRead < 0) {
- break;
- } else {
- totalRead += bytesRead;
+ while (totalRead < len && position < size) {
+ if (!inWindow(position)) {
+ if (len - totalRead >= WINDOW_SIZE) {
+ // large reads bypass the window
+ int n = raf.getChannel().read(byteBuffer, position);
+ if (n < 0) {
+ break;
+ }
+ position += n;
+ totalRead += n;
+ continue;
+ }
+ fill(position);
}
- byteBuffer.put(buf, 0, bytesRead);
+ int offset = (int) (position - windowStart);
+ int n = Math.min(len - totalRead, windowLength - offset);
+ byteBuffer.put(window, offset, n);
+ position += n;
+ totalRead += n;
}
Review Comment:
The read loop can stall indefinitely if a read returns 0 bytes (e.g.,
`FileChannel.read(...)` returning 0, or `fill()` producing `windowLength ==
0`). In that case `position`/`totalRead` do not advance and the `while` loop
never terminates. Handle `n == 0` explicitly (e.g., treat as an error or break)
and in `fill()` throw if `n <= 0` when `pos < size` to avoid infinite loops on
unexpected short/empty reads.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/onenote/OneNoteDirectFileResource.java:
##########
@@ -23,60 +23,89 @@
import java.nio.ByteBuffer;
/**
- * This is copied mostly from the the
- * former org.apache.tika.parser.mp4.DirectFileReadDataSource
- * <p>
- * Implements a simple way to encapsulate a {@link
org.apache.tika.io.TikaInputStream} that you
- * will have to seek,read,repeat while parsing OneNote contents.
+ * Seekable reads over a OneNote file. The parser mostly reads a few bytes at
a time, so reads
+ * come from a window of the file rather than one system call each.
*/
class OneNoteDirectFileResource implements Closeable {
- private static final int TRANSFER_SIZE = 8192;
+ private static final int WINDOW_SIZE = 64 * 1024;
private final RandomAccessFile raf;
+ private final long size;
+ private final byte[] window = new byte[WINDOW_SIZE];
+ // file offset of window[0], and how many bytes of the window are valid
+ private long windowStart;
+ private int windowLength;
+ private long position;
public OneNoteDirectFileResource(File f) throws IOException {
this.raf = new RandomAccessFile(f, "r");
+ this.size = raf.length();
}
public int read() throws IOException {
- return raf.read();
+ if (position >= size) {
+ return -1;
+ }
+ if (!inWindow(position)) {
+ fill(position);
+ }
+ return window[(int) (position++ - windowStart)] & 0xFF;
}
public int read(ByteBuffer byteBuffer) throws IOException {
int len = byteBuffer.remaining();
int totalRead = 0;
- int bytesRead = 0;
- byte[] buf = new byte[TRANSFER_SIZE];
- while (totalRead < len) {
- int bytesToRead = Math.min((len - totalRead), TRANSFER_SIZE);
- bytesRead = raf.read(buf, 0, bytesToRead);
- if (bytesRead < 0) {
- break;
- } else {
- totalRead += bytesRead;
+ while (totalRead < len && position < size) {
+ if (!inWindow(position)) {
+ if (len - totalRead >= WINDOW_SIZE) {
+ // large reads bypass the window
+ int n = raf.getChannel().read(byteBuffer, position);
+ if (n < 0) {
+ break;
+ }
+ position += n;
+ totalRead += n;
+ continue;
+ }
+ fill(position);
}
- byteBuffer.put(buf, 0, bytesRead);
+ int offset = (int) (position - windowStart);
+ int n = Math.min(len - totalRead, windowLength - offset);
+ byteBuffer.put(window, offset, n);
+ position += n;
+ totalRead += n;
}
- if (bytesRead < 0 && position() == size() &&
byteBuffer.hasRemaining()) {
+ if (totalRead < len && position == size) {
throw new IOException("End of stream reached earlier than
expected");
}
- return ((bytesRead < 0) && (totalRead == 0)) ? -1 : totalRead;
+ return totalRead == 0 && len > 0 ? -1 : totalRead;
+ }
+
+ private boolean inWindow(long pos) {
+ return pos >= windowStart && pos < windowStart + windowLength;
+ }
+
+ private void fill(long pos) throws IOException {
+ raf.seek(pos);
+ int n = raf.read(window, 0, (int) Math.min(WINDOW_SIZE, size - pos));
+ windowStart = pos;
+ windowLength = Math.max(n, 0);
}
Review Comment:
The read loop can stall indefinitely if a read returns 0 bytes (e.g.,
`FileChannel.read(...)` returning 0, or `fill()` producing `windowLength ==
0`). In that case `position`/`totalRead` do not advance and the `while` loop
never terminates. Handle `n == 0` explicitly (e.g., treat as an error or break)
and in `fill()` throw if `n <= 0` when `pos < size` to avoid infinite loops on
unexpected short/empty reads.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/onenote/OneNoteTreeWalkerOptions.java:
##########
@@ -26,30 +26,41 @@
* Options when walking the one note tree.
*/
public class OneNoteTreeWalkerOptions implements Serializable {
- private boolean crawlAllFileNodesFromRoot = true;
+ private boolean crawlAllFileNodesFromRoot = false;
+ private boolean includeDeletedContent = false;
private boolean onlyLatestRevision = true;
private Set<OneNotePropertyEnum> utf16PropertiesToPrint = new HashSet<>(
- Arrays.asList(OneNotePropertyEnum.ImageFilename,
OneNotePropertyEnum.Author));
+ Arrays.asList(OneNotePropertyEnum.ImageFilename,
OneNotePropertyEnum.ImageAltText,
+ OneNotePropertyEnum.Author,
OneNotePropertyEnum.FolderChildFilename));
/**
- * Do this to ignore revisions and just parse all file nodes from the root
recursively.
+ * Whether text comes from every revision, page history included, rather
than only each
+ * page's current revision. Metadata and attachments come from every
revision either way.
*/
public boolean isCrawlAllFileNodesFromRoot() {
return crawlAllFileNodesFromRoot;
}
Review Comment:
The option name `crawlAllFileNodesFromRoot` now appears to control 'include
page history' semantics (per the updated Javadoc and its use from
`OneNoteParserConfig.isIncludePageHistory()`), which makes the API misleading.
Consider introducing an explicitly named option like `includePageHistory` on
`OneNoteTreeWalkerOptions` (and either deprecating the old name or restoring
its original meaning) to prevent incorrect usage by callers constructing
options directly.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/onenote/OneNoteTreeWalkerOptions.java:
##########
@@ -26,30 +26,41 @@
* Options when walking the one note tree.
*/
public class OneNoteTreeWalkerOptions implements Serializable {
- private boolean crawlAllFileNodesFromRoot = true;
+ private boolean crawlAllFileNodesFromRoot = false;
+ private boolean includeDeletedContent = false;
Review Comment:
The option name `crawlAllFileNodesFromRoot` now appears to control 'include
page history' semantics (per the updated Javadoc and its use from
`OneNoteParserConfig.isIncludePageHistory()`), which makes the API misleading.
Consider introducing an explicitly named option like `includePageHistory` on
`OneNoteTreeWalkerOptions` (and either deprecating the old name or restoring
its original meaning) to prevent incorrect usage by callers constructing
options directly.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/onenote/OneNoteTreeWalkerOptions.java:
##########
@@ -26,30 +26,41 @@
* Options when walking the one note tree.
*/
public class OneNoteTreeWalkerOptions implements Serializable {
- private boolean crawlAllFileNodesFromRoot = true;
+ private boolean crawlAllFileNodesFromRoot = false;
+ private boolean includeDeletedContent = false;
private boolean onlyLatestRevision = true;
private Set<OneNotePropertyEnum> utf16PropertiesToPrint = new HashSet<>(
- Arrays.asList(OneNotePropertyEnum.ImageFilename,
OneNotePropertyEnum.Author));
+ Arrays.asList(OneNotePropertyEnum.ImageFilename,
OneNotePropertyEnum.ImageAltText,
+ OneNotePropertyEnum.Author,
OneNotePropertyEnum.FolderChildFilename));
/**
- * Do this to ignore revisions and just parse all file nodes from the root
recursively.
+ * Whether text comes from every revision, page history included, rather
than only each
+ * page's current revision. Metadata and attachments come from every
revision either way.
*/
public boolean isCrawlAllFileNodesFromRoot() {
return crawlAllFileNodesFromRoot;
}
/**
- * Do this to ignore revisions and just parse all file nodes from the root
recursively.
- *
- * @param crawlAllFileNodesFromRoot
- * @return
+ * @param crawlAllFileNodesFromRoot whether to emit text from every
revision
+ * @return this
*/
public OneNoteTreeWalkerOptions setCrawlAllFileNodesFromRoot(
boolean crawlAllFileNodesFromRoot) {
this.crawlAllFileNodesFromRoot = crawlAllFileNodesFromRoot;
return this;
}
Review Comment:
The option name `crawlAllFileNodesFromRoot` now appears to control 'include
page history' semantics (per the updated Javadoc and its use from
`OneNoteParserConfig.isIncludePageHistory()`), which makes the API misleading.
Consider introducing an explicitly named option like `includePageHistory` on
`OneNoteTreeWalkerOptions` (and either deprecating the old name or restoring
its original meaning) to prevent incorrect usage by callers constructing
options directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
