From 902d05b22b022b156234515693f2b8866248f482 Mon Sep 17 00:00:00 2001 From: Jack Date: Thu, 23 Jul 2026 22:06:22 +0800 Subject: [PATCH] feat: parser component param check and family mapping fixes (#17312) ## Summary Adds construction-time parameter validation to the ingestion `ParserComponent` (mirroring the applicable subset of Python `ParserParam.check()`), fixes a family-mapping mismatch that silently skipped `output_format` validation and setup configuration for image/audio files, and aligns the no-CGO parser stubs with the CGO variants by threading `context.Context` through `ParseWithResult`. --- internal/ingestion/component/parser.go | 77 +++++++- .../ingestion/component/parser_check_test.go | 174 ++++++++++++++++++ .../ingestion/component/parser_dispatch.go | 8 +- .../component/parser_dispatch_test.go | 80 ++++++++ .../parser/parser/office_parsers_no_cgo.go | 9 +- .../parser/office_parsers_nocgo_test.go | 10 +- internal/parser/parser/pdf_parser_nocgo.go | 9 +- .../parser/parser/pdf_parser_nocgo_test.go | 6 +- 8 files changed, 349 insertions(+), 24 deletions(-) create mode 100644 internal/ingestion/component/parser_check_test.go diff --git a/internal/ingestion/component/parser.go b/internal/ingestion/component/parser.go index 9b8e24cdf9..6288455c3e 100644 --- a/internal/ingestion/component/parser.go +++ b/internal/ingestion/component/parser.go @@ -64,10 +64,12 @@ // side-effect component (out of scope for Phase 2.2). // // - The Python _param.check() business validation -// (parse_method whitelist, output_format whitelist, etc.) is -// not replicated. The component trusts the param block -// passed in at construction time; invalid values surface as -// runtime errors in the chosen parser branch. +// (parse_method whitelist, conditional lang checks) is mirrored +// by (*ParserComponent).Check() below, which NewParserComponent +// runs at construction time. The Python flow check() also +// validates audio/video vlm.llm_id, but Go media_dispatch uses +// tenant default models (resolveTenantModelByType) rather than +// setup["vlm"]["llm_id"], so that check is intentionally omitted. // // - NO PERSISTENCE: parsed pages live only in the per-run // output map, exactly as the schema.Page type is intended. @@ -166,7 +168,70 @@ func NewParserComponent(params map[string]any) (runtime.Component, error) { } p.AllowedOutputFormat = allowed } - return &ParserComponent{Setups: s, Param: p}, nil + pc := &ParserComponent{Setups: s, Param: p} + if err := pc.Check(); err != nil { + return nil, fmt.Errorf("Parser: %w", err) + } + return pc, nil +} + +// Check mirrors the applicable subset of Python ParserParam.check() +// (rag/flow/parser/parser.py:251-321). Runs at construction time so +// a malformed DSL surfaces as a canvas compile failure rather than a +// mid-run error. Returns the first validation error encountered +// (Python raises ValueError on the first failure). +// +// NOT covered here (intentional): +// - output_format whitelist: already enforced at Invoke time by +// resolveOutputFormat (parser_dispatch.go:100-122). +// - audio/video vlm.llm_id: Go media_dispatch uses tenant default +// models (resolveTenantModelByType), not setup["vlm"]["llm_id"]. +// The Python flow check() for vlm.llm_id does not apply — Go +// never reads that field, and validating it would block every +// valid audio/video pipeline (see ingestion_pipeline_audio.json). +func (c *ParserComponent) Check() error { + // PDF family (parser.py:252-261). + if pdf, ok := c.Setups["pdf"]; ok { + pm, _ := pdf["parse_method"].(string) + if pm == "" { + return errors.New("Parse method abnormal. does not support empty value.") + } + pmLower := strings.ToLower(pm) + pdfWhitelist := []string{ + "deepdoc", "plain_text", "mineru", "docling", + "opendataloader", "tcadp parser", "paddleocr", "somark", + } + if !containsString(pdfWhitelist, pmLower) { + // Non-whitelist parse_method is treated as a VLM method, + // which requires lang (Python parser.py:257-258). + if lang, _ := pdf["lang"].(string); lang == "" { + return errors.New("PDF VLM language does not support empty value.") + } + } + } + // image family (parser.py:283-287). + if img, ok := c.Setups["image"]; ok { + pm, _ := img["parse_method"].(string) + // OCR mode does not need a VLM language; any other value does. + if pm != "ocr" { + if lang, _ := img["lang"].(string); lang == "" { + return errors.New("Image VLM language does not support empty value.") + } + } + } + return nil +} + +// containsString reports whether s is in list. Used by Check() for +// whitelist membership tests; kept unexported and local to this file +// to avoid polluting the package namespace. +func containsString(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false } func defaultSetups() map[string]schema.ParserSetup { @@ -245,7 +310,7 @@ func defaultSetups() map[string]schema.ParserSetup { "aiff", "au", "midi", "wma", "realaudio", "vqf", "oggvorbis", "ape", }, - "output_format": "text", + "output_format": "json", }, "video": { "suffix": []string{"mp4", "avi", "mkv"}, diff --git a/internal/ingestion/component/parser_check_test.go b/internal/ingestion/component/parser_check_test.go new file mode 100644 index 0000000000..47c134483f --- /dev/null +++ b/internal/ingestion/component/parser_check_test.go @@ -0,0 +1,174 @@ +// +// Copyright 2026 The InfiniFlow Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package component + +import ( + "strings" + "testing" + + "ragflow/internal/ingestion/component/schema" +) + +// TestParserComponent_Check covers the construction-time business +// validation that mirrors the applicable subset of Python +// ParserParam.check() (rag/flow/parser/parser.py:251-321). +// +// Go does NOT validate audio/video vlm.llm_id because media_dispatch +// resolves tenant default models via resolveTenantModelByType, not +// setup["vlm"]["llm_id"]. See plan: quantum-forging-curie-sZ_7zRZb. +func TestParserComponent_Check(t *testing.T) { + cases := []struct { + name string + setups map[string]schema.ParserSetup + wantErr string // non-empty substring expected in error; empty means no error + }{ + // --- PDF family (parser.py:252-261) --- + { + name: "pdf: parse_method empty → error", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": ""}}, + wantErr: "Parse method abnormal", + }, + { + name: "pdf: parse_method missing → error", + setups: map[string]schema.ParserSetup{"pdf": {}}, + wantErr: "Parse method abnormal", + }, + { + name: "pdf: deepdoc (whitelist) without lang → pass", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "deepdoc"}}, + }, + { + name: "pdf: plain_text (whitelist, case-insensitive) without lang → pass", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "PLAIN_TEXT"}}, + }, + { + name: "pdf: tcadp parser (whitelist with space) without lang → pass", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "tcadp parser"}}, + }, + { + name: "pdf: unknown VLM method without lang → error", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "some_vlm", "lang": ""}}, + wantErr: "PDF VLM language", + }, + { + name: "pdf: unknown VLM method with lang → pass", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "some_vlm", "lang": "English"}}, + }, + { + name: "pdf: paddleocr (whitelist) without lang → pass", + setups: map[string]schema.ParserSetup{"pdf": {"parse_method": "paddleocr"}}, + }, + + // --- image family (parser.py:283-287) --- + { + name: "image: ocr without lang → pass (no lang check for OCR)", + setups: map[string]schema.ParserSetup{"image": {"parse_method": "ocr"}}, + }, + { + name: "image: ocr with empty lang → pass (OCR skips lang)", + setups: map[string]schema.ParserSetup{"image": {"parse_method": "ocr", "lang": ""}}, + }, + { + name: "image: non-ocr without lang → error", + setups: map[string]schema.ParserSetup{"image": {"parse_method": "vlm_xyz", "lang": ""}}, + wantErr: "Image VLM language", + }, + { + name: "image: non-ocr with lang → pass", + setups: map[string]schema.ParserSetup{"image": {"parse_method": "vlm_xyz", "lang": "English"}}, + }, + { + name: "image: missing parse_method → pass (treated as non-ocr, but lang defaults empty in DSL)", + setups: map[string]schema.ParserSetup{"image": {"lang": "English"}}, + }, + + // --- audio/video: vlm.llm_id NOT validated in Go --- + { + name: "audio: no vlm field → pass (Go uses tenant default ASR)", + setups: map[string]schema.ParserSetup{"audio": {"output_format": "text"}}, + }, + { + name: "video: no vlm field → pass (Go uses tenant default VISION)", + setups: map[string]schema.ParserSetup{"video": {"output_format": "text"}}, + }, + { + name: "audio: vlm.llm_id empty → pass (Go ignores vlm.llm_id)", + setups: map[string]schema.ParserSetup{"audio": {"vlm": map[string]any{"llm_id": ""}}}, + }, + + // --- empty / default configurations --- + { + name: "empty setups → pass", + setups: map[string]schema.ParserSetup{}, + }, + { + name: "nil setups → pass", + setups: nil, + }, + { + name: "only unrelated family → pass", + setups: map[string]schema.ParserSetup{"markdown": {"output_format": "json"}}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := &ParserComponent{Setups: tc.setups, Param: schema.ParserParam{}.Defaults()} + err := c.Check() + if tc.wantErr != "" { + if err == nil { + t.Fatalf("want error containing %q, got nil", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error = %q, want substring %q", err.Error(), tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("want nil error, got: %v", err) + } + }) + } +} + +// TestParserComponent_New_RunsCheck asserts NewParserComponent +// invokes Check() at construction time so a malformed DSL surfaces +// as a canvas compile failure rather than a mid-run error. +func TestParserComponent_New_RunsCheck(t *testing.T) { + // Default config must pass (audio/video vlm not required in Go). + c, err := NewParserComponent(nil) + if err != nil { + t.Fatalf("NewParserComponent(nil): %v", err) + } + if c == nil { + t.Fatal("NewParserComponent(nil) returned nil component") + } + + // Malformed PDF parse_method must fail at construction. + c, err = NewParserComponent(map[string]any{ + "pdf": map[string]any{"parse_method": "", "lang": ""}, + }) + if err == nil { + t.Fatal("NewParserComponent with empty pdf.parse_method: want error, got nil") + } + if !strings.Contains(err.Error(), "Parse method abnormal") { + t.Errorf("error = %q, want substring %q", err.Error(), "Parse method abnormal") + } + if c != nil { + t.Errorf("want nil component on error, got %T", c) + } +} diff --git a/internal/ingestion/component/parser_dispatch.go b/internal/ingestion/component/parser_dispatch.go index 981eb87f9b..8eea5e131c 100644 --- a/internal/ingestion/component/parser_dispatch.go +++ b/internal/ingestion/component/parser_dispatch.go @@ -305,17 +305,17 @@ func pythonFamilyName(raw string) string { "go", "ts", "sh", "cs", "kt", "sql": return "text&code" case "mp4", "avi", "mkv", "mov", "webm", "flv", - "mpeg", "mpg", "wmv", "3gp", "3gpp": + "mpeg", "mpg", "wmv", "3gp", "3gpp", "video": return "video" - case "eml", "msg": + case "eml", "msg", "email": return "email" case "da", "wave", "wav", "mp3", "aac", "flac", "ogg", - "aiff", "au", "midi", "wma", "ape", "alac", "wv", "opus": + "aiff", "au", "midi", "wma", "ape", "alac", "wv", "opus", "aural": return "audio" case "visual", "picture", "image", "png", "jpg", "jpeg", "gif", "bmp", "tiff", "tif", "webp", "svg", "ico", "avif", "heic", "apng": - return "picture" + return "image" } return "" } diff --git a/internal/ingestion/component/parser_dispatch_test.go b/internal/ingestion/component/parser_dispatch_test.go index 197ad40692..dbd2cba282 100644 --- a/internal/ingestion/component/parser_dispatch_test.go +++ b/internal/ingestion/component/parser_dispatch_test.go @@ -849,3 +849,83 @@ func tcadpZipFixtureForComponent(t *testing.T) []byte { } return buf.Bytes() } + +// TestPythonFamilyName_FileTypeConstants pins the contract that every +// utility.FileType semantic constant resolves to the setups key used by +// defaultSetups / AllowedOutputFormat. Before the fix, FileTypeVISUAL mapped +// to "picture" (mismatching the "image" setups key) and FileTypeAURAL matched +// no case at all (returning ""), so output_format validation and +// configureParserFromSetups were silently skipped for image/audio files. +func TestPythonFamilyName_FileTypeConstants(t *testing.T) { + cases := []struct { + ft utility.FileType + want string + }{ + {utility.FileTypePDF, "pdf"}, + {utility.FileTypeDOC, "doc"}, + {utility.FileTypeDOCX, "docx"}, + {utility.FileTypePPT, "slides"}, + {utility.FileTypePPTX, "slides"}, + {utility.FileTypeXLS, "spreadsheet"}, + {utility.FileTypeXLSX, "spreadsheet"}, + {utility.FileTypeCSV, "spreadsheet"}, + {utility.FileTypeHTML, "html"}, + {utility.FileTypeMarkdown, "markdown"}, + {utility.FileTypeTXT, "text&code"}, + {utility.FileTypeEPUB, "epub"}, + {utility.FileTypeJSON, "json"}, + {utility.FileTypeVISUAL, "image"}, + {utility.FileTypeAURAL, "audio"}, + {utility.FileTypeVIDEO, "video"}, + {utility.FileTypeEMAIL, "email"}, + } + for _, c := range cases { + got := pythonFamilyName(string(c.ft)) + if got != c.want { + t.Errorf("pythonFamilyName(%q) = %q, want %q", c.ft, got, c.want) + } + // Every mapped family must have a matching setups key and + // allowed_output_format entry, otherwise output_format validation + // is silently skipped. + if _, ok := defaultSetups()[got]; !ok { + t.Errorf("pythonFamilyName(%q) → %q has no defaultSetups entry", c.ft, got) + } + allowed := schema.ParserParam{}.Defaults().AllowedOutputFormat + if _, ok := allowed[got]; !ok { + t.Errorf("pythonFamilyName(%q) → %q has no allowed_output_format entry", c.ft, got) + } + } +} + +// TestConfigureParserFromSetups_VisualAural pins that image and audio +// files pick up their setup (parse_method / lang / vlm) via the family +// mapping. Before the fix, configureParserFromSetups silently skipped +// configuration because resolveParserFamily returned "picture" / "aural", +// neither of which existed in defaultSetups. +func TestConfigureParserFromSetups_VisualAural(t *testing.T) { + setups := defaultSetups() + + t.Run("visual resolves to image setup", func(t *testing.T) { + got := &captureSetupConfigurer{} + configureParserFromSetups(got, utility.FileTypeVISUAL, setups) + want := map[string]any(setups["image"]) + if got.setup == nil { + t.Fatal("ConfigureFromSetup not called for FileTypeVISUAL") + } + if v, _ := got.setup["parse_method"].(string); v != want["parse_method"] { + t.Errorf("FileTypeVISUAL parse_method = %v, want %v", v, want["parse_method"]) + } + }) + + t.Run("aural resolves to audio setup", func(t *testing.T) { + got := &captureSetupConfigurer{} + configureParserFromSetups(got, utility.FileTypeAURAL, setups) + want := map[string]any(setups["audio"]) + if got.setup == nil { + t.Fatal("ConfigureFromSetup not called for FileTypeAURAL") + } + if v, _ := got.setup["output_format"].(string); v != want["output_format"] { + t.Errorf("FileTypeAURAL output_format = %v, want %v", v, want["output_format"]) + } + }) +} diff --git a/internal/parser/parser/office_parsers_no_cgo.go b/internal/parser/parser/office_parsers_no_cgo.go index ce1e3a1604..8f1443fde2 100644 --- a/internal/parser/parser/office_parsers_no_cgo.go +++ b/internal/parser/parser/office_parsers_no_cgo.go @@ -3,6 +3,7 @@ package parser import ( + "context" "errors" "fmt" ) @@ -17,28 +18,28 @@ import ( // shape used by the rest of the package. var ErrOfficeCGORequired = errors.New("parser: office family requires CGO (office_oxide)") -func (p *DOCXParser) ParseWithResult(filename string, _ []byte) ParseResult { +func (p *DOCXParser) ParseWithResult(_ context.Context, filename string, _ []byte) ParseResult { return ParseResult{ File: map[string]any{"name": filename}, Err: fmt.Errorf("%w: docx", ErrOfficeCGORequired), } } -func (p *DOCParser) ParseWithResult(filename string, _ []byte) ParseResult { +func (p *DOCParser) ParseWithResult(_ context.Context, filename string, _ []byte) ParseResult { return ParseResult{ File: map[string]any{"name": filename}, Err: fmt.Errorf("%w: doc", ErrOfficeCGORequired), } } -func (p *PPTParser) ParseWithResult(filename string, _ []byte) ParseResult { +func (p *PPTParser) ParseWithResult(_ context.Context, filename string, _ []byte) ParseResult { return ParseResult{ File: map[string]any{"name": filename}, Err: fmt.Errorf("%w: ppt", ErrOfficeCGORequired), } } -func (p *PPTXParser) ParseWithResult(filename string, _ []byte) ParseResult { +func (p *PPTXParser) ParseWithResult(_ context.Context, filename string, _ []byte) ParseResult { return ParseResult{ File: map[string]any{"name": filename}, Err: fmt.Errorf("%w: pptx", ErrOfficeCGORequired), diff --git a/internal/parser/parser/office_parsers_nocgo_test.go b/internal/parser/parser/office_parsers_nocgo_test.go index 3da84b6ce1..c3cacbf690 100644 --- a/internal/parser/parser/office_parsers_nocgo_test.go +++ b/internal/parser/parser/office_parsers_nocgo_test.go @@ -3,19 +3,21 @@ package parser import ( + "context" "errors" "testing" ) func TestOfficeParsers_ParseWithResult_NoCGO(t *testing.T) { + ctx := context.Background() cases := []struct { name string res ParseResult }{ - {name: "docx", res: (&DOCXParser{}).ParseWithResult("a.docx", nil)}, - {name: "doc", res: (&DOCParser{}).ParseWithResult("a.doc", nil)}, - {name: "pptx", res: (&PPTXParser{}).ParseWithResult("a.pptx", nil)}, - {name: "ppt", res: (&PPTParser{}).ParseWithResult("a.ppt", nil)}, + {name: "docx", res: (&DOCXParser{}).ParseWithResult(ctx, "a.docx", nil)}, + {name: "doc", res: (&DOCParser{}).ParseWithResult(ctx, "a.doc", nil)}, + {name: "pptx", res: (&PPTXParser{}).ParseWithResult(ctx, "a.pptx", nil)}, + {name: "ppt", res: (&PPTParser{}).ParseWithResult(ctx, "a.ppt", nil)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/parser/parser/pdf_parser_nocgo.go b/internal/parser/parser/pdf_parser_nocgo.go index 1a4f808abe..ad3a67d0ec 100644 --- a/internal/parser/parser/pdf_parser_nocgo.go +++ b/internal/parser/parser/pdf_parser_nocgo.go @@ -3,6 +3,7 @@ package parser import ( + "context" "fmt" ) @@ -14,13 +15,13 @@ func (p *PDFParser) ParseWithResult(ctx context.Context, filename string, data [ case "plain_text": return parsePDFWithPlainText(filename, data, p) case "mineru": - return parsePDFWithMinerU(filename, data, p) + return parsePDFWithMinerU(ctx, filename, data, p) case "paddleocr": - return parsePDFWithPaddleOCR(filename, data, p) + return parsePDFWithPaddleOCR(ctx, filename, data, p) case "docling": - return parsePDFWithDocling(filename, data, p) + return parsePDFWithDocling(ctx, filename, data, p) case "opendataloader": - return parsePDFWithOpenDataLoader(filename, data, p) + return parsePDFWithOpenDataLoader(ctx, filename, data, p) case "somark": return parsePDFWithSoMark(filename, data, p) case "tcadp": diff --git a/internal/parser/parser/pdf_parser_nocgo_test.go b/internal/parser/parser/pdf_parser_nocgo_test.go index 97efbe4c90..a1b00d3724 100644 --- a/internal/parser/parser/pdf_parser_nocgo_test.go +++ b/internal/parser/parser/pdf_parser_nocgo_test.go @@ -3,14 +3,16 @@ package parser import ( + "context" "errors" "testing" ) func TestPDFParser_ParseWithResult_NoCGO(t *testing.T) { + ctx := context.Background() pdf := NewPDFParser() - empty := pdf.ParseWithResult("empty.pdf", nil) + empty := pdf.ParseWithResult(ctx, "empty.pdf", nil) if empty.Err != nil { t.Fatalf("empty input: want nil err, got %v", empty.Err) } @@ -21,7 +23,7 @@ func TestPDFParser_ParseWithResult_NoCGO(t *testing.T) { t.Fatalf("empty input JSON len = %d, want 1", len(empty.JSON)) } - res := pdf.ParseWithResult("a.pdf", []byte("%PDF-1.4")) + res := pdf.ParseWithResult(ctx, "a.pdf", []byte("%PDF-1.4")) if res.Err == nil { t.Fatal("want ErrPDFEngineUnavailable, got nil") }