[
https://issues.apache.org/jira/browse/TIKA-5057?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141933#comment-18141933
]
ASF GitHub Bot commented on TIKA-5057:
--------------------------------------
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.
> General improvements to onenote parser
> --------------------------------------
>
> Key: TIKA-5057
> URL: https://issues.apache.org/jira/browse/TIKA-5057
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Trivial
>
> When I have extra tokens, I'll have an agent focus on a parser at a time for
> robustness and simplicity. Let's start with onenote. CC [~henrylindemanglean]
> :robot: swarm recommends:
> {noformat}
> Native .one path (the common format, unrelated to TIKA-5055):
> 1. Exponential parse on shared child lists (OneNotePtr:714). A file-node
> list referenced twice is parsed twice. Only the current path is checked for
> cycles, so a crafted file of a few KB blows up as 2^depth. Three reviewers
> found this independently.
> 2. Exponential tree walk (OneNoteTreeWalker:648). The same shape: shared
> objects are walked once per reference path. Each visit also builds and keeps
> a HashMap of decoded properties, which OneNoteParser:144 throws away.
> 3. Wrong references, lost text (OneNotePtr:1185). The OID/OSID/ContextID
> stream position is copied to a local variable and never written back. Every
> reference property starts again at stream[0], so later references are never
> walked. It also removes the
> cap on reference counts.
> 4. Duplicate text in real files (OneNoteTreeWalker:569-592). Reproduced:
> testOneNote2.one produces 176 paragraphs, 105 of them "Untitled picture.png".
> Image filenames and TextExtendedAscii text are never deduplicated and ignore
> the revision filter.
> 5. Unchecked counts and budgets (OneNotePtr:838, :518, :809):
> - ObjectInfoDependencyOverrideData counts are unbounded.
> - The global-ID cap applies per node, with no total.
> - A negative FileDataStoreObject length gets through.
> 6. Smaller issues:
> - Embedded file data is read in full before shouldParseEmbedded is asked
> (OneNoteTreeWalker:435).
> - The UTF-16 NUL terminator stays in author names and image filenames.
> Legacy string dump, used whenever a parse falls back
> (OneNoteLegacyDumpStrings:95):
> - Never resets its buffer at 1 MB boundaries. A long run is emitted again
> at every boundary, so work grows quadratically and normal files over 1 MB get
> duplicated text.
> - Small bugs: it drops the last byte of each buffer,
> Character.isWhitespace(i) tests the loop index instead of the character, and
> split(" ") allocates one String per word.
> Fallback after a partial walk (OneNoteParser:172, :197): on both paths, the
> full legacy dump is added after whatever the failed walk already wrote, so
> content appears twice.
> FSSHTTPB:
> - GUIDs are MD5 hashes, not decoded (BitReader:140, AdapterHelper:37). They
> come from UUID.nameUUIDFromBytes. As a result:
> - Encrypted sections are never detected; the check at
> MSOneStoreParser:328 can't match.
> - Embedded relationship IDs contain hashed GUIDs.
> - Every cell mapping copies its whole revision chain
> (MSOneStoreParser:362), so the cost grows with mappings × objects.
> - Slow spots:
> - removeSupersededObjects is O(n²) (:393).
> - resourceInfoFromReferencingObject scans every object for each unnamed
> blob (MSOneStorePackage:1140).
> - hasUsablePicture copies a whole blob just to check it's non-empty
> (:1118).
> - Small port bugs:
> - cProperties is read as signed, so values ≥ 0x8000 abort the parse.
> - An undefined property type throws an NPE.
> - StorageManifestSchemaGUID reads from offset 0 instead of the current
> position.
> - BitConverter's bounds check ignores the index.
> Dead code:
> - The serialize/write path: 65 files, about 970 lines.
> - The FSSHTTPD chunking/build side.
> - getContent() and its builders.
> - Unreferenced classes: Error, PropertyIDType, RequestTypes, UMath.
> - Debug print() methods and option branches that are never set.
> {noformat}
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
