Skip to content

OPENNLP-1931: Regex removal (3a/10): Read opennlp-dl vocab and config JSON with a strict scanner - #1277

Open
krickert wants to merge 1 commit into
apache:mainfrom
ai-pipestream:OPENNLP-1931-dl-json-scan
Open

krickert wants to merge 1 commit into
apache:mainfrom
ai-pipestream:OPENNLP-1931-dl-json-scan

Conversation

@krickert

@krickert krickert commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Replaces the regular expressions in AbstractDL.loadJsonVocab and DocumentCategorizerConfig.fromJson with JsonScan, a cursor-based JSON scanner that follows RFC 8259 for structure, whitespace, and string escapes.

AI assisted edit (reviewed and edited):

  • A JSON vocabulary is one object mapping tokens to non-negative integer IDs. Fractional, negative, and overflowing IDs are rejected with the token named. Any other layout, including a Hugging Face tokenizer.json, is rejected with an error describing the expected format.
  • Configuration labels come from the top-level id2label object. Keys and labels are decoded from their escapes, including quotes, Unicode escapes, and braces inside strings. A value that is not a string is rejected.
  • Malformed vocabulary and configuration files surface as InvalidFormatException with the offset of the problem.
  • A leading byte order mark is ignored in JSON and plain-text vocabularies and in config.json, as RFC 8259 section 8.1 allows. This fixes OPENNLP-1953. The check lives in two new StringUtil methods, startsWithByteOrderMark and stripByteOrderMark, so other readers can share it.
  • Nested values are scanned with an explicit stack instead of recursion, and escape searches are bounded to the current string. Python's NaN, Infinity, and -Infinity are accepted in skipped values and rejected where a value is read. Raw control characters inside strings are accepted as content, as before.
  • JsonScan is marked @Internal(since = "3.0.0"). It is public only because DocumentCategorizerConfig uses it.
  • The document categorizer chapter describes the accepted vocabulary and configuration layouts and the error behavior, and says that tokenizer.json is not accepted as a vocabulary yet.
    tokenizer.json support is deferred to OPENNLP-1988 and a separate PR (OPENNLP-1988: (3b/10) Support compatible WordPiece tokenizer.json files #1319).

Changes since the last review

  • Byte order mark handling moved into the shared StringUtil helpers.
  • doccat.xml: shorter vocabulary and configuration paragraphs, plus the tokenizer.json note.
  • JsonScan: a constant for the repeated "key must not be null" message, and the unreachable bound check in unescape is gone.
  • LoadVocabTest and DocumentCategorizerDLTest write their files under a JUnit @TempDir.
  • The branch is squashed to one commit.
    Testing
  • 630 opennlp-dl tests and the opennlp-api tests pass locally. Checkstyle is clean and the opennlp-docs module builds.
  • DocumentCategorizerDLEval passed all eight enabled tests with the nlptown BERT sentiment model, including automatic labels and concurrent inference. The GPU test is disabled and was skipped.
  • Eval build: (link)

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little time, so here is a GPT 5.6-sol review instead for now

No blocking findings. No additional API or parsing regression found. The old and new JSON implementations agreed across 60,000 generated cases.

Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.

@krickert
krickert force-pushed the OPENNLP-1931-dl-json-scan branch from 944ab39 to fed91ba Compare September 15, 2026 16:00
@rzo1

rzo1 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Here are some additional comments. Def. needs an eval build (DocumentCategorizerDLEval reads its label config through the changed fromJson). Compatibility on well-formed files is good: 26 real HF classification config.json files and several vocab.json files gave identical maps old vs new. The problems are hostile or slightly non-strict files, undocumented behavior changes, and the API surface.

Blocking

  1. JsonScan.java:291, :294, :331, :358. endOfValue → endOfObject/endOfArray is recursive, so untrusted input throws StackOverflowError, not IAE. A 20 KB file of [ or {"a": + 5000×{"a": is enough, truncated input included, and the Error reaches the AbstractDL/DocumentCategorizerDL constructors. Skip nested values iteratively (depth counter plus a bracket-kind stack), or cap nesting (Jackson's default is 1000) and throw. Add deep and truncated-deep tests.

  2. DocumentCategorizerConfig.java:55-56. The whole config.json is now validated, so config files that loaded before throw, although only id2label is needed:

    • A UTF-8 BOM (Files.readString keeps U+FEFF): old {0=negative,1=positive}, new IAE offset 0 expected '{'. RFC 8259 §8.1 allows ignoring a BOM.
    • Python's json.dumps default output with NaN/Infinity in an unrelated member: old OK, new IAE expected a value.
    • A trailing comma or a // comment line: old OK, new IAE.

    Skip a leading U+FEFF and accept NaN/Infinity/-Infinity in skipped values (config.json is written by Python), or document the break in the release notes and JIRA. Pin the choice with tests.

  3. DocumentCategorizerConfig.java:56-63. Category names returned by getBestCategory/scoreMap change for existing models. It is a fix, but a silent one:

    • {"0":"négatif","1":"say \"hi\"","2":"x}y","3":"z"}: old {0=négatif, 1=say \} with labels 2 and 3 dropped, new {0=négatif, 1=say "hi", 2=x}y, 3=z}.
    • A config with id2label nested (e.g. text_config.id2label) used to give labels and now silently gives {}. Decide between an empty map and throwing, and test it.
  4. AbstractDL.java:499-547. tokenizer.json vocabulary support is a new feature inside a "regex removal" PR, and not mentioned in the title, description or JIRA. On bert-base-uncased the old map had 30524 entries including junk (type_id=1, max_input_chars_per_word=100), the new one has 30522; all-MiniLM-L6-v2 drops 6 junk keys (Fixed=128, …); roberta-base is unchanged. added_tokens absent from model.vocab are silently not entries. Split it into its own JIRA/PR, or at least describe it and document added_tokens.

  5. DocumentCategorizerDL.java:156, AbstractDL.java:91/:168. A malformed resource file now surfaces as unchecked IAE from constructors that declare IOException, so callers catching IOException no longer catch it. Before this PR a bad config never threw. Wrap at the file boundary (loadVocabFile, readCategoriesFromFile) into InvalidFormatException and add @throws.

  6. JsonScan.java:29-31, :61-62 vs :176-186, :283-289. The Javadoc says escapes follow RFC 8259 and document throws if the text "is malformed at any position". Not true: closingQuote doesn't check escapes, so {"a":["\q"]}, {"a":{"\q":1}} and {"a":"\q"} pass. Check escapes in closingQuote (one pass), or fix both Javadocs and pin the nested and array cases.

  7. JsonScan.java:37, :53, :88-165. New public API in the exported opennlp.dl package (6 static methods plus a public record with a public constructor) for 2 callers; @Internal doesn't stop it from being frozen. Arguments are not validated: members("{}", -1) and a hand-built Member(…, 5, 9) throw SIOOBE, member(null, …) and stringValue(text, null) throw NPE. Keep it package-private, move the id2label reading into opennlp.dl, or expose one narrow reader. Validate with IAE and add @throws.

  8. PR description. It says the opposite of the head: "Behavior is unchanged, including the quirks of the old patterns … a decimal fraction gives its integer prefix, a negative id is skipped, the id2label object is cut at the first closing brace", with "400,000 generated inputs with zero differences". Commit ae1aa71 replaced that; 1.5 and -1 now throw (LoadVocabTest:165-170), braces inside labels are kept, and the test counts are from the old design. Rewrite it to list every change above. Suggested title: "OPENNLP-1931: Reject malformed JSON vocabulary and config files in opennlp-dl; read the vocabulary of tokenizer.json".

Minor

  • DocumentCategorizerConfig.java:55. StringUtil.isBlank depends on the whitespace mode, while JsonScan uses RFC whitespace, so a single U+001C gives {} under legacy and IAE under unicode (U+0085 the other way round). Use JsonScan.skipWhitespace(json, 0) == json.length().
  • DocumentCategorizerConfig.java:51-53. fromJson(null) changed from NPE to IAE on a public record method. Mention it.
  • NameFinderDL.java:130-135/153-158, SentenceVectorsDL.java:72-75/90-93, DocumentCategorizerDL.java:119-122. These constructors also go through loadVocabFile but didn't get the new @throws. Align all public DL constructors.
  • AbstractDL.java:532-547. A Unigram tokenizer.json (model.vocab is an array) falls back to the top-level object and fails with Value of "version" must be a non-negative integer: "1.0" (papluca/xlm-roberta-base-language-detection). Throw a clear "unsupported tokenizer.json model.vocab layout" error and test it.
  • AbstractDL.java:187 (pre-existing). A vocab.json with a BOM fails startsWith("{") and is silently read as plain text with wrong ids. Worth fixing here, since the new Javadoc at :160 promises "first non-whitespace character is a brace".
  • JsonScan.java:299. new String[] {TRUE, FALSE, NULL} per literal. Declare a constant.
  • JsonScan.java:460-468, :222-225. Inline hex ranges and magic 6/4/2/20. Use HexFormat.isHexDigit/HexFormat.fromHexDigits and name the lengths.
  • JsonScan.java:327-329. afterColon returns -1 and the error path calls skipWhitespace again. Call expect(text, colon, ':') inline.
  • JsonScan.java:198-199. unescape allocates a StringBuilder per key even without a backslash. Fast path via indexOf('\\', start) >= end.
  • JsonScan.java:32-33. "so a label wrapped over two lines still reads" is rationale; state the contract, and say U+0000-U+001F are also accepted in keys ({"a<U+0001>":1} passes).
  • JsonScan.java:176, :198, :244, :261, :278. Package-private only for tests. Make them private and test through document/members.
  • JsonScanTest.java:97-101. One test covers null for two methods. Split or parameterize it.
  • JsonScanTest.java:37-39, :124-126, :195-197, :222-224. Remove the banner comments.
  • Tests. Add deep nesting, BOM, NaN/Infinity, bad escapes in nested keys and arrays, -01/-0 inside a document, negative offsets, a Unigram tokenizer.json, nested id2label, and ensure_ascii labels.
  • doccat.xml:176-198. Verbose parser internals; "a line break inside a token is kept as content" contradicts RFC 8259. Cut to the user-visible rules: accepted layouts, ids must be non-negative ints, malformed files throw.
  • namefinder.xml:153-156. Re-wraps an unchanged sentence. Revert.

Verified:

  • JsonScan vs jackson-core 2.20.1 on 1M generated inputs: 0 differences on valid JSON; numbers, literals, trailing commas, truncation and whitespace all match.
  • Scanning is linear (100 MB vocab in 384 ms).
  • No JSON library is on the opennlp-dl classpath, so a small scanner is defensible if items 1, 6 and 7 are fixed.

1 similar comment
@rzo1

rzo1 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Here are some additional comments. Def. needs an eval build (DocumentCategorizerDLEval reads its label config through the changed fromJson). Compatibility on well-formed files is good: 26 real HF classification config.json files and several vocab.json files gave identical maps old vs new. The problems are hostile or slightly non-strict files, undocumented behavior changes, and the API surface.

Blocking

  1. JsonScan.java:291, :294, :331, :358. endOfValue → endOfObject/endOfArray is recursive, so untrusted input throws StackOverflowError, not IAE. A 20 KB file of [ or {"a": + 5000×{"a": is enough, truncated input included, and the Error reaches the AbstractDL/DocumentCategorizerDL constructors. Skip nested values iteratively (depth counter plus a bracket-kind stack), or cap nesting (Jackson's default is 1000) and throw. Add deep and truncated-deep tests.

  2. DocumentCategorizerConfig.java:55-56. The whole config.json is now validated, so config files that loaded before throw, although only id2label is needed:

    • A UTF-8 BOM (Files.readString keeps U+FEFF): old {0=negative,1=positive}, new IAE offset 0 expected '{'. RFC 8259 §8.1 allows ignoring a BOM.
    • Python's json.dumps default output with NaN/Infinity in an unrelated member: old OK, new IAE expected a value.
    • A trailing comma or a // comment line: old OK, new IAE.

    Skip a leading U+FEFF and accept NaN/Infinity/-Infinity in skipped values (config.json is written by Python), or document the break in the release notes and JIRA. Pin the choice with tests.

  3. DocumentCategorizerConfig.java:56-63. Category names returned by getBestCategory/scoreMap change for existing models. It is a fix, but a silent one:

    • {"0":"négatif","1":"say \"hi\"","2":"x}y","3":"z"}: old {0=négatif, 1=say \} with labels 2 and 3 dropped, new {0=négatif, 1=say "hi", 2=x}y, 3=z}.
    • A config with id2label nested (e.g. text_config.id2label) used to give labels and now silently gives {}. Decide between an empty map and throwing, and test it.
  4. AbstractDL.java:499-547. tokenizer.json vocabulary support is a new feature inside a "regex removal" PR, and not mentioned in the title, description or JIRA. On bert-base-uncased the old map had 30524 entries including junk (type_id=1, max_input_chars_per_word=100), the new one has 30522; all-MiniLM-L6-v2 drops 6 junk keys (Fixed=128, …); roberta-base is unchanged. added_tokens absent from model.vocab are silently not entries. Split it into its own JIRA/PR, or at least describe it and document added_tokens.

  5. DocumentCategorizerDL.java:156, AbstractDL.java:91/:168. A malformed resource file now surfaces as unchecked IAE from constructors that declare IOException, so callers catching IOException no longer catch it. Before this PR a bad config never threw. Wrap at the file boundary (loadVocabFile, readCategoriesFromFile) into InvalidFormatException and add @throws.

  6. JsonScan.java:29-31, :61-62 vs :176-186, :283-289. The Javadoc says escapes follow RFC 8259 and document throws if the text "is malformed at any position". Not true: closingQuote doesn't check escapes, so {"a":["\q"]}, {"a":{"\q":1}} and {"a":"\q"} pass. Check escapes in closingQuote (one pass), or fix both Javadocs and pin the nested and array cases.

  7. JsonScan.java:37, :53, :88-165. New public API in the exported opennlp.dl package (6 static methods plus a public record with a public constructor) for 2 callers; @Internal doesn't stop it from being frozen. Arguments are not validated: members("{}", -1) and a hand-built Member(…, 5, 9) throw SIOOBE, member(null, …) and stringValue(text, null) throw NPE. Keep it package-private, move the id2label reading into opennlp.dl, or expose one narrow reader. Validate with IAE and add @throws.

  8. PR description. It says the opposite of the head: "Behavior is unchanged, including the quirks of the old patterns … a decimal fraction gives its integer prefix, a negative id is skipped, the id2label object is cut at the first closing brace", with "400,000 generated inputs with zero differences". Commit ae1aa71 replaced that; 1.5 and -1 now throw (LoadVocabTest:165-170), braces inside labels are kept, and the test counts are from the old design. Rewrite it to list every change above. Suggested title: "OPENNLP-1931: Reject malformed JSON vocabulary and config files in opennlp-dl; read the vocabulary of tokenizer.json".

Minor

  • DocumentCategorizerConfig.java:55. StringUtil.isBlank depends on the whitespace mode, while JsonScan uses RFC whitespace, so a single U+001C gives {} under legacy and IAE under unicode (U+0085 the other way round). Use JsonScan.skipWhitespace(json, 0) == json.length().
  • DocumentCategorizerConfig.java:51-53. fromJson(null) changed from NPE to IAE on a public record method. Mention it.
  • NameFinderDL.java:130-135/153-158, SentenceVectorsDL.java:72-75/90-93, DocumentCategorizerDL.java:119-122. These constructors also go through loadVocabFile but didn't get the new @throws. Align all public DL constructors.
  • AbstractDL.java:532-547. A Unigram tokenizer.json (model.vocab is an array) falls back to the top-level object and fails with Value of "version" must be a non-negative integer: "1.0" (papluca/xlm-roberta-base-language-detection). Throw a clear "unsupported tokenizer.json model.vocab layout" error and test it.
  • AbstractDL.java:187 (pre-existing). A vocab.json with a BOM fails startsWith("{") and is silently read as plain text with wrong ids. Worth fixing here, since the new Javadoc at :160 promises "first non-whitespace character is a brace".
  • JsonScan.java:299. new String[] {TRUE, FALSE, NULL} per literal. Declare a constant.
  • JsonScan.java:460-468, :222-225. Inline hex ranges and magic 6/4/2/20. Use HexFormat.isHexDigit/HexFormat.fromHexDigits and name the lengths.
  • JsonScan.java:327-329. afterColon returns -1 and the error path calls skipWhitespace again. Call expect(text, colon, ':') inline.
  • JsonScan.java:198-199. unescape allocates a StringBuilder per key even without a backslash. Fast path via indexOf('\\', start) >= end.
  • JsonScan.java:32-33. "so a label wrapped over two lines still reads" is rationale; state the contract, and say U+0000-U+001F are also accepted in keys ({"a<U+0001>":1} passes).
  • JsonScan.java:176, :198, :244, :261, :278. Package-private only for tests. Make them private and test through document/members.
  • JsonScanTest.java:97-101. One test covers null for two methods. Split or parameterize it.
  • JsonScanTest.java:37-39, :124-126, :195-197, :222-224. Remove the banner comments.
  • Tests. Add deep nesting, BOM, NaN/Infinity, bad escapes in nested keys and arrays, -01/-0 inside a document, negative offsets, a Unigram tokenizer.json, nested id2label, and ensure_ascii labels.
  • doccat.xml:176-198. Verbose parser internals; "a line break inside a token is kept as content" contradicts RFC 8259. Cut to the user-visible rules: accepted layouts, ids must be non-negative ints, malformed files throw.
  • namefinder.xml:153-156. Re-wraps an unchanged sentence. Revert.

Verified:

  • JsonScan vs jackson-core 2.20.1 on 1M generated inputs: 0 differences on valid JSON; numbers, literals, trailing commas, truncation and whitespace all match.
  • Scanning is linear (100 MB vocab in 384 ms).
  • No JSON library is on the opennlp-dl classpath, so a small scanner is defensible if items 1, 6 and 7 are fixed.

@rzo1

rzo1 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Follow-up: the pre-existing BOM problem in AbstractDL.loadVocabFile is filed as OPENNLP-1953. It is related to the BOM point for config.json in my review.

@krickert krickert changed the title OPENNLP-1931: Scan the opennlp-dl JSON vocabulary and id2label without patterns OPENNLP-1931: Reject malformed JSON vocabulary and config files in opennlp-dl; read the vocabulary of tokenizer.json Sep 16, 2026
@krickert
krickert force-pushed the OPENNLP-1931-dl-json-scan branch 2 times, most recently from 8699305 to a4ed75a Compare September 16, 2026 06:09
@rzo1

rzo1 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

CI is red.

@rzo1 rzo1 changed the title OPENNLP-1931: Reject malformed JSON vocabulary and config files in opennlp-dl; read the vocabulary of tokenizer.json OPENNLP-1931: Regex removal (3/8): Read opennlp-dl vocab and config JSON with a strict scanner Sep 18, 2026
@rzo1 rzo1 changed the title OPENNLP-1931: Regex removal (3/8): Read opennlp-dl vocab and config JSON with a strict scanner OPENNLP-1931: Regex removal (3/10): Read opennlp-dl vocab and config JSON with a strict scanner Sep 18, 2026
@krickert
krickert force-pushed the OPENNLP-1931-dl-json-scan branch 2 times, most recently from f632f55 to ed9dd7e Compare September 18, 2026 21:16
@rzo1

rzo1 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for the update. CI is green now, and the description is refreshed. I have three points on the code.

1. Please move the tokenizer.json support to its own issue and PR

Doing it properly needs more than this PR should carry. AbstractDL always builds a WordpieceEncoder (AbstractDL.java:111, :142), yet the Javadoc (:541) and the Unigram error message (:592) say BPE models are supported, and LoadVocabTest:259 loads a "type": "BPE" file. A RoBERTa tokenizer.json loads without error and then gets WordPiece over byte-level BPE tokens, so predictions go wrong without any error. The support needs a model.type == "WordPiece" gate, and possibly continuing_subword_prefix and the normalizer's lowercase flag as well. That's a feature of its own, not regex removal.

For this PR, please:

  • drop the tokenizer.json commits (912b434d, c1f98f5f) and the tokenizer.json cases from 16c40928;
  • remove the tokenizer.json sentences from doccat.xml and from the description;
  • have the rejection of a tokenizer.json say what went wrong. At the moment it would read like "version is not a non-negative integer". Something like "expected one object mapping tokens to integer ids, as in vocab.json" would be clearer.

Nothing is released between the two PRs, since main is 3.0.0-SNAPSHOT, so if the follow-up lands before 3.0.0 no user ever sees the gap. Please file an issue for it.

2. Mark JsonScan as @Internal

JsonScan is public only because DocumentCategorizerConfig in opennlp.dl.doccat calls stringObject. opennlp-dl has no module-info, so the class becomes visible API as soon as it ships. Please annotate it with @Internal(since = "3.0.0") (opennlp.tools.commons.Internal).

3. Reference OPENNLP-1953

The byte order mark fix in loadVocabFile is OPENNLP-1953. Please mention it in those commits or in the squash message so the issue can be closed when this merges.

The eval build (DocumentCategorizerDLEval) is still needed before merge.

@krickert
krickert marked this pull request as ready for review September 19, 2026 17:55
@krickert krickert changed the title OPENNLP-1931: Regex removal (3/10): Read opennlp-dl vocab and config JSON with a strict scanner OPENNLP-1931: Regex removal (3a/10): Read opennlp-dl vocab and config JSON with a strict scanner Sep 19, 2026
@krickert
krickert force-pushed the OPENNLP-1931-dl-json-scan branch from 3c7012f to 399e50d Compare September 20, 2026 20:19

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking

  1. AbstractDL.java loadJsonVocab: behavior change that isn't documented. The old regex picked up every "token": id pair anywhere in the file, so a tokenizer.json passed as the vocabulary happened to load. It is now rejected, and so is any vocab.json with non-integer members. That's the right call, but users will only see it as a new InvalidFormatException. Please say in doccat.xml that tokenizer.json isn't accepted as a vocabulary yet, with a pointer to OPENNLP-1988.

  2. Eval build. This changes how DL vocabularies and labels are loaded. Please trigger an eval build on the final head before merge. The local DocumentCategorizerDLEval run is good, but not enough.

Minor

  • AbstractDL.java:56, :196 and JsonScan.java:59, :286. Byte-order-mark handling is duplicated: AbstractDL strips the BOM, and then JsonScan checks for it again. FrequencyDictionaryLoader (#1320) and HunspellDictionary do the same. Please add one StringUtil.stripByteOrderMark and use it everywhere.
  • JsonScan.java:98, :124, :200. "key must not be null" is repeated three times, while TEXT_MUST_NOT_BE_NULL is already a constant. Declare KEY_MUST_NOT_BE_NULL.
  • JsonScan.java:350-351. indexOf(BACKSLASH, start, end) is already bounded, so firstBackslash >= end can never be true. Drop it.
  • JsonScan.java:48-49. @Internal doesn't change that this ships as a public type. It's public only because DocumentCategorizerConfig sits in opennlp.dl.doccat. Move the id2label lookup into opennlp.dl and make JsonScan package-private, or say why it has to be public.
  • JsonScan.java:34-46. The class Javadoc explains the design ("in one pass", "Nesting is bounded by memory, not by the call stack", "the only API of this class"). Cut it down to what the class does, RFC 8259 plus the documented deviations, and move the rationale to the JIRA ticket.
  • doccat.xml:176-199. Both paragraphs are too detailed ("Thus a BOM-prefixed…", "U+FEFF inside a JSON string is content…", "so a damaged vocabulary is reported instead of read in part", Python's NaN). Condense each to two or three sentences. :185 is also much longer than its neighbours; please rewrap it.
  • LoadVocabTest.java. The File.createTempFile + Files.writeString setup is repeated in several tests, and DocumentCategorizerDLTest:211 already has a configFile(...) helper. Use @TempDir with one shared helper.
  • Description. "Configuration whitespace handling is independent of opennlp.whitespace.mode" only makes sense next to the other regex-removal PRs. Drop it.

claude Bot added a commit to ai-pipestream/opennlp that referenced this pull request Sep 24, 2026
OPENNLP-1931: Strip byte order marks through StringUtil (for apache apache#1277)
krickert added a commit to ai-pipestream/opennlp that referenced this pull request Sep 24, 2026
…inors

OPENNLP-1931: Address rzo1's remaining review asks (for apache apache#1277)

Human sign-off
… JSON with a strict scanner

Replaces the regular expressions in `AbstractDL.loadJsonVocab` and `DocumentCategorizerConfig.fromJson` with `JsonScan`, a cursor-based JSON
scanner that follows RFC 8259 for structure, whitespace, and string escapes, and reports the offset of a malformed document.

I've followed all the points made from the review, hand-reviewed with full comments public and in the upstream fork.

The suggestions were all good - and the implementation was checked on every level.  All of it is well tested, and approved in 3 PRs on the ai-pipestream fork.

Edited AI summary below:

- A JSON vocabulary is one object mapping tokens to non-negative integer
  ids. Fractional, negative, and overflowing ids are rejected with the
  token named. Any other layout, including tokenizer.json, is rejected
  with an error describing the expected format; tokenizer.json support
  is deferred to OPENNLP-1988.
- id2label is read from the top-level configuration object. Keys and
  labels are decoded from their escapes, a brace inside a label does not
  end the object, and a value that is not a string is rejected.
- Malformed token and configuration files surface as
  InvalidFormatException from loadVocabFile and readCategories.
- A leading byte order mark is skipped in JSON and plain-text
  vocabularies and in config.json, as RFC 8259 section 8.1 allows. This
  fixes OPENNLP-1953. A mark at any later offset is malformed, and a
  configuration that is only the mark and whitespace has no labels.
  StringUtil gains startsWithByteOrderMark and stripByteOrderMark, which
  AbstractDL and JsonScan share.
- Nested values are skipped with an explicit stack, so a deeply nested
  or truncated file is rejected at its offset instead of overflowing the
  stack. Escape searches are bounded to the current string. NaN,
  Infinity, and -Infinity are accepted in skipped values, as Python's
  json module writes them, and rejected where a value is read.
- JsonScan is annotated @internal(since = "3.0.0"); it is public only
  because DocumentCategorizerConfig uses stringObject.
- The document categorizer chapter states the accepted layouts, the
  integer id rule, the byte order mark rule, the non-finite values, and
  the InvalidFormatException.

Tests cover offsets and reasons for text cut off after each token,
content after the object, separators and non-JSON whitespace, bad
escapes at any depth, values nested 500 levels deep, CR and CRLF inside
and outside strings, keys written with escapes, ids that do not fit an
int, leading zeros, tokenizer.json rejection, and the byte order mark in
both file formats. The vocabulary and categorizer tests write their
files under a JUnit temporary directory.

Assisted-by: Claude Code 2.1.281
Co-authored-by: Kristian Rickert <kristian@apache.org>
Co-authored-by: Claude <noreply@anthropic.com>
Reviewed-by: Kristian Rickert <kristian@apache.org>
Signed-off-by: Kristian Rickert <kristian@apache.org>
@krickert
krickert force-pushed the OPENNLP-1931-dl-json-scan branch from 399e50d to 16148bb Compare September 24, 2026 11:02
@krickert

Copy link
Copy Markdown
Contributor Author

Once green, going to run the eval build

@rzo1

rzo1 commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

@krickert Please link the eval build here.

@rzo1

rzo1 commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for the update. Maintainer edits don't work on the ai-pipestream fork, so here is a patch with the remaining mechanical fixes:

  • JsonScan: class Javadoc reduced to what it reads and the three deviations from RFC 8259. Dropped "in one pass", the call-stack sentence, and "the only API of this class".
  • DocumentCategorizerDL.readCategories: a non-integer id2label key now fails with "id2label key must be an integer: " instead of the bare "For input string: ..." from NumberFormatException.
  • StringUtilByteOrderMarkTest:  / � escapes instead of invisible characters in the source.
  • doccat.xml: condensed both paragraphs, dropped the Python NaN/Infinity sentence, and made OPENNLP-1988 a <ulink>.

Apply it with git am 1277.patch, and please don't force-push over it without applying it first.

1277.patch
From fb41615b1b87be53ed9398367978f17be16f26c0 Mon Sep 17 00:00:00 2001
From: Richard Zowalla <rzo1@apache.org>
Date: Fri, 25 Sep 2026 21:10:17 +0200
Subject: [PATCH] OPENNLP-1931: Trim commentary and name the bad id2label key

- JsonScan: reduce the class Javadoc to what is read and the three
  deviations from RFC 8259.
- DocumentCategorizerDL: report a non-integer id2label key by name
  instead of the bare NumberFormatException message.
- StringUtilByteOrderMarkTest: use escapes instead of invisible
  characters.
- doccat.xml: condense the vocabulary and configuration paragraphs and
  link OPENNLP-1988.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 .../util/StringUtilByteOrderMarkTest.java     |  4 +--
 .../src/main/java/opennlp/dl/JsonScan.java    | 26 +++++++++----------
 .../dl/doccat/DocumentCategorizerDL.java      | 17 +++++++++++-
 opennlp-docs/src/docbkx/doccat.xml            | 23 ++++++----------
 4 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/opennlp-api/src/test/java/opennlp/tools/util/StringUtilByteOrderMarkTest.java b/opennlp-api/src/test/java/opennlp/tools/util/StringUtilByteOrderMarkTest.java
index 0914c7673..8f34a0a90 100644
--- a/opennlp-api/src/test/java/opennlp/tools/util/StringUtilByteOrderMarkTest.java
+++ b/opennlp-api/src/test/java/opennlp/tools/util/StringUtilByteOrderMarkTest.java
@@ -25,7 +25,7 @@ import org.junit.jupiter.api.Test;
  */
 public class StringUtilByteOrderMarkTest {
 
-  private static final String BOM = "";
+  private static final String BOM = "\uFEFF";
 
   @Test
   void testStripsOneLeadingMark() {
@@ -49,7 +49,7 @@ public class StringUtilByteOrderMarkTest {
     Assertions.assertFalse(StringUtil.startsWithByteOrderMark(""));
     Assertions.assertFalse(StringUtil.startsWithByteOrderMark("{}" + BOM));
     // U+FFFE is the byte-swapped mark, not a mark itself
-    Assertions.assertFalse(StringUtil.startsWithByteOrderMark("�{}"));
+    Assertions.assertFalse(StringUtil.startsWithByteOrderMark("\uFFFE{}"));
   }
 
   @Test
diff --git a/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/JsonScan.java b/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/JsonScan.java
index a5363c93f..4a0246a86 100644
--- a/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/JsonScan.java
+++ b/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/JsonScan.java
@@ -30,20 +30,18 @@ import opennlp.tools.commons.Internal;
 import opennlp.tools.util.StringUtil;
 
 /**
- * Reads the JSON files of the deep-learning components, vocabularies and model configurations,
- * in one pass over the text. Structure, whitespace, numbers, and string escapes follow
- * <a href="https://www.rfc-editor.org/rfc/rfc8259">RFC 8259</a>, with three additions: a byte
- * order mark as the first character is skipped, as
- * <a href="https://www.rfc-editor.org/rfc/rfc8259#section-8.1">section 8.1</a> allows; the
- * control characters {@code U+0000} to {@code U+001F} are accepted as content inside a string,
- * in keys as well as in values; and the values {@code NaN}, {@code Infinity}, and
- * {@code -Infinity}, which Python's {@code json} module writes by default, are accepted where a
- * value is skipped, never where one is read. Nesting is bounded by memory, not by the call
- * stack. Malformed text is reported as an {@link IllegalArgumentException} whose message names
- * the offset at which reading stopped.
- *
- * <p>{@link #stringObject(String, String)} is the only API of this class; its other members
- * serve the classes of this package.
+ * Reads the JSON files of the deep-learning components, vocabularies and model configurations.
+ * Structure, whitespace, numbers, and string escapes follow
+ * <a href="https://www.rfc-editor.org/rfc/rfc8259">RFC 8259</a>, with three additions:
+ * <ul>
+ *   <li>a byte order mark as the first character is skipped, as
+ *   <a href="https://www.rfc-editor.org/rfc/rfc8259#section-8.1">section 8.1</a> allows;</li>
+ *   <li>the control characters {@code U+0000} to {@code U+001F} are accepted inside a string;</li>
+ *   <li>the values {@code NaN}, {@code Infinity}, and {@code -Infinity} are accepted where a
+ *   value is skipped, never where one is read.</li>
+ * </ul>
+ * Malformed text is reported as an {@link IllegalArgumentException} whose message names the
+ * offset at which reading stopped.
  */
 @Internal(since = "3.0.0")
 public final class JsonScan {
diff --git a/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/doccat/DocumentCategorizerDL.java b/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/doccat/DocumentCategorizerDL.java
index ca9fd0667..208cb573e 100644
--- a/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/doccat/DocumentCategorizerDL.java
+++ b/opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/doccat/DocumentCategorizerDL.java
@@ -453,7 +453,7 @@ public class DocumentCategorizerDL extends AbstractDL implements DocumentCategor
     final Map<Integer, String> categories = new HashMap<>();
     try {
       for (Map.Entry<String, String> label : DocumentCategorizerConfig.fromJson(json).id2label().entrySet()) {
-        categories.put(Integer.valueOf(label.getKey()), label.getValue());
+        categories.put(parseIndex(label.getKey()), label.getValue());
       }
     } catch (IllegalArgumentException e) {
       throw new InvalidFormatException(
@@ -462,4 +462,19 @@ public class DocumentCategorizerDL extends AbstractDL implements DocumentCategor
     return categories;
   }
 
+  /**
+   * Parses an {@code id2label} key as an output index.
+   *
+   * @param key The key to parse.
+   * @return The output index.
+   * @throws IllegalArgumentException Thrown if {@code key} is not an integer.
+   */
+  private static int parseIndex(String key) {
+    try {
+      return Integer.parseInt(key);
+    } catch (NumberFormatException e) {
+      throw new IllegalArgumentException("id2label key must be an integer: " + key, e);
+    }
+  }
+
 }
diff --git a/opennlp-docs/src/docbkx/doccat.xml b/opennlp-docs/src/docbkx/doccat.xml
index 7b74274c1..b41729334 100644
--- a/opennlp-docs/src/docbkx/doccat.xml
+++ b/opennlp-docs/src/docbkx/doccat.xml
@@ -173,24 +173,17 @@ String category = myCategorizer.getBestCategory(outcomes);]]>
 				For additional examples, refer to the <code>DocumentCategorizerDLEval</code> class.
 			</para>
 			<para>
-				The vocabulary file is either plain text, with one token per line and the line number as
-				the token ID, as in <code>vocab.txt</code>, or a JSON object that maps each token to a
-				non-negative integer ID, as in <code>vocab.json</code>. A file is read as JSON when its
-				first non-whitespace character is an opening brace, and a leading byte order mark is
-				ignored. A Hugging Face <code>tokenizer.json</code> is not a vocabulary file and is
-				rejected; support for it is tracked in OPENNLP-1988. A vocabulary that cannot be read is
-				rejected with an <code>InvalidFormatException</code> that names the file and the position
-				of the problem.
+				The vocabulary file is either plain text with one token per line, as in
+				<code>vocab.txt</code>, or a JSON object mapping each token to an integer ID, as in
+				<code>vocab.json</code>. A Hugging Face <code>tokenizer.json</code> is not supported yet
+				(<ulink url="https://issues.apache.org/jira/browse/OPENNLP-1988">OPENNLP-1988</ulink>).
+				An unreadable vocabulary is rejected with an <code>InvalidFormatException</code>.
 			</para>
 			<para>
 				When a configuration file is given in place of the categories map, its top-level
-				<code>id2label</code> object supplies the categories, mapping each output index to a
-				label. A configuration without a top-level <code>id2label</code> gives no categories.
-				A configuration that is not well-formed JSON, or whose <code>id2label</code> is not an
-				object of strings keyed by integers, is rejected with an
-				<code>InvalidFormatException</code>. The values <code>NaN</code> and
-				<code>Infinity</code>, which Python writes into some configurations, are accepted
-				outside <code>id2label</code>.
+				<code>id2label</code> object maps each output index to a category label. A configuration
+				that is not well-formed JSON, or whose <code>id2label</code> is not an object of strings
+				keyed by integers, is rejected with an <code>InvalidFormatException</code>.
 			</para>
 			<para>
 				Like <code>NameFinderDL</code>, long input is split into overlapping chunks on the full
-- 
2.55.0

Still open on your side:

  • Eval build: this changes how DocumentCategorizerDLEval reads labels, so please run it and link the result. The description still has the "(link)" placeholder.
  • The commit message should not mention "approved in 3 PRs on the ai-pipestream fork" or "I've followed all the points". Please drop that when squashing.
  • The byte order mark is stripped in AbstractDL.loadVocabFile and checked again in JsonScan.document. Keep one, or add a short comment explaining why both are needed.
  • Add a test pinning that a lone \uD83D escape is accepted as is.
  • FrequencyDictionaryLoader still has its own BOM constant. It can move to StringUtil.stripByteOrderMark in OPENNLP-1933: Regex removal (5a/10): Simplify dictionary and filename parsing #1320.

As mentioned on #1278: pushing your branches to apache/opennlp instead of the ai-pipestream fork would let committers push small fixes like this directly.

@krickert

Copy link
Copy Markdown
Contributor Author

Thanks. I'll push all of them to Apache branches tonight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants