diff --git a/internal/dao/ingestion_parser_config_runtime_test.go b/internal/dao/ingestion_parser_config_runtime_test.go index 4b529fcada..b4b1625f15 100644 --- a/internal/dao/ingestion_parser_config_runtime_test.go +++ b/internal/dao/ingestion_parser_config_runtime_test.go @@ -221,7 +221,10 @@ func TestMigratedMinerUControlsReachRequest(t *testing.T) { if err := dao.MigrateIngestionParserConfigsForTest(t.Context(), db); err != nil { t.Fatal(err) } - config := pipeline.BuildParserConfig(dsl, migratedConfig(t, db, "kb-mineru")) + config, err := pipeline.BuildParserConfig(dsl, migratedConfig(t, db, "kb-mineru")) + if err != nil { + t.Fatal(err) + } effective := effectiveComponentParams(t, "general", config) parser, err := component.NewParserComponent(effective["Parser:HipSignsRhyme"]) if err != nil { @@ -400,9 +403,17 @@ func TestIngestionMigrationMatchesBuiltinTemplates(t *testing.T) { } // Snapshot before cleaning so mutations cannot hide a lost parameter. expected := jsonNormalize(t, config) - cleaned := pipeline.CleanComponentParams(rawDSL, config) - if !reflect.DeepEqual(jsonNormalize(t, cleaned), expected) { - t.Fatalf("API cleaning changed migrated params: got %v; want %v", cleaned, expected) + cleaned, err := pipeline.BuildParserConfig(rawDSL, config) + if err != nil { + t.Fatal(err) + } + for cpnID, raw := range expected.(map[string]any) { + params, _ := cleaned[cpnID].(map[string]any) + for key, value := range raw.(map[string]any) { + if !reflect.DeepEqual(jsonNormalize(t, params[key]), value) { + t.Fatalf("API config changed migrated %s.%s: got %v; want %v", cpnID, key, params[key], value) + } + } } // Parser: exactly the template's families the config targets, @@ -873,7 +884,11 @@ func extractorFromMigratedConfig(t *testing.T, config map[string]any) *component if err != nil { t.Fatal(err) } - cleaned := service.ApplyComponentScopedParserConfig(pipeline.BuildParserConfig(dsl, config), "tenant-llm") + cleaned, err := pipeline.BuildParserConfig(dsl, config) + if err != nil { + t.Fatal(err) + } + cleaned = service.ApplyComponentScopedParserConfig(cleaned, "tenant-llm") c, err := component.NewExtractorComponent(cleaned["Extractor:AutoExtractDefault"].(map[string]any)) if err != nil { t.Fatal(err) diff --git a/internal/ingestion/component/parser_dispatch.go b/internal/ingestion/component/parser_dispatch.go index 0d5370cb1d..23fffb6039 100644 --- a/internal/ingestion/component/parser_dispatch.go +++ b/internal/ingestion/component/parser_dispatch.go @@ -258,7 +258,7 @@ func pythonFamilyName(raw string) string { return "doc" case "docx": return "docx" - case "ppt", "pptx", "slides": + case "ppt", "pptx", "pages", "dps", "dpt", "slides": return "slides" case "xls", "xlsx", "spreadsheet": return "spreadsheet" diff --git a/internal/ingestion/component/parser_dispatch_test.go b/internal/ingestion/component/parser_dispatch_test.go index 0ef41e37a9..587bcc97b5 100644 --- a/internal/ingestion/component/parser_dispatch_test.go +++ b/internal/ingestion/component/parser_dispatch_test.go @@ -1961,6 +1961,14 @@ func tcadpZipFixtureForComponent(t *testing.T) []byte { return buf.Bytes() } +func TestParserFileFamilyPresentationExtensions(t *testing.T) { + for _, ext := range []string{"ppt", "pptx", "pages", "dps", "dpt"} { + if got := ParserFileFamily(ext); got != "slides" { + t.Errorf("ParserFileFamily(%q) = %q, want slides", ext, got) + } + } +} + // TestPythonFamilyName_FileTypeConstants pins the contract that every // utility.FileType semantic constant resolves to the setups key used by // defaultSetups. Before the fix, FileTypeVISUAL mapped diff --git a/internal/ingestion/pipeline/agent_template_extractor_contract_test.go b/internal/ingestion/pipeline/agent_template_extractor_contract_test.go index 56e6c965e4..8269adaa42 100644 --- a/internal/ingestion/pipeline/agent_template_extractor_contract_test.go +++ b/internal/ingestion/pipeline/agent_template_extractor_contract_test.go @@ -138,19 +138,19 @@ func TestAgentTemplateExtractorsUseModularParams(t *testing.T) { t.Errorf("%s: Extractor %s has every modular sub-task disabled; the template's purpose requires at least one active", f, cpnID) } - // The params must also survive CleanComponentParams: no + // The params must also survive BuildParserConfig: no // dropped keys and any active sub-task left active with its // prompt intact. dslRaw, _ := json.Marshal(doc["dsl"]) - cleaned := CleanComponentParams(dslRaw, map[string]any{cpnID: params}) + cleaned := buildTestParserConfig(t, dslRaw, map[string]any{cpnID: params}) got, _ := cleaned[cpnID].(map[string]any) if got == nil { - t.Errorf("%s: Extractor %s dropped by CleanComponentParams", f, cpnID) + t.Errorf("%s: Extractor %s dropped by BuildParserConfig", f, cpnID) continue } for k := range params { if _, kept := got[k]; !kept { - t.Errorf("%s: Extractor %s param %q dropped by CleanComponentParams", f, cpnID, k) + t.Errorf("%s: Extractor %s param %q dropped by BuildParserConfig", f, cpnID, k) } } for _, k := range modularKeys { @@ -159,14 +159,14 @@ func TestAgentTemplateExtractorsUseModularParams(t *testing.T) { continue } if !subTaskActive(k, got[k]) { - t.Errorf("%s: Extractor %s sub-task %q no longer active after CleanComponentParams: %#v", f, cpnID, k, got[k]) + t.Errorf("%s: Extractor %s sub-task %q no longer active after BuildParserConfig: %#v", f, cpnID, k, got[k]) continue } if k == "summary" { cleanedSummary, _ := got[k].(map[string]any) prompt, _ := cleanedSummary["system_prompt"].(string) if prompt == "" { - t.Errorf("%s: Extractor %s summary.system_prompt empty after CleanComponentParams", f, cpnID) + t.Errorf("%s: Extractor %s summary.system_prompt empty after BuildParserConfig", f, cpnID) } } } diff --git a/internal/ingestion/pipeline/image_ocr_test.go b/internal/ingestion/pipeline/image_ocr_test.go index 0d039510d9..8b4646e65c 100644 --- a/internal/ingestion/pipeline/image_ocr_test.go +++ b/internal/ingestion/pipeline/image_ocr_test.go @@ -114,7 +114,7 @@ func TestImageOCRSavedConfigToSearchableChunk(t *testing.T) { t.Fatal(err) } const parserID = "Parser:ViewsCaptureLight" - config := BuildParserConfig(dsl, map[string]any{parserID: map[string]any{ + config := buildTestParserConfig(t, dsl, map[string]any{parserID: map[string]any{ "image": map[string]any{"ocr_enabled": true, "ocr_model_id": "ocr-model"}, "enable_vision_enhancement": false, }}) diff --git a/internal/ingestion/pipeline/mineru_config_test.go b/internal/ingestion/pipeline/mineru_config_test.go index 21fd73b038..1f04f15dab 100644 --- a/internal/ingestion/pipeline/mineru_config_test.go +++ b/internal/ingestion/pipeline/mineru_config_test.go @@ -41,7 +41,7 @@ func TestBuildParserConfigPreservesMinerUExtractionSwitches(t *testing.T) { if nested { params = map[string]any{"setups": params} } - config := BuildParserConfig(generalDSL(t), map[string]any{ + config := buildTestParserConfig(t, generalDSL(t), map[string]any{ "Parser:HipSignsRhyme": params, }) // Exercise the serialized shape stored in parser_config, not diff --git a/internal/ingestion/pipeline/parser_tenant_reference_test.go b/internal/ingestion/pipeline/parser_tenant_reference_test.go index 2cc2fcae00..48aec5f526 100644 --- a/internal/ingestion/pipeline/parser_tenant_reference_test.go +++ b/internal/ingestion/pipeline/parser_tenant_reference_test.go @@ -27,7 +27,7 @@ func TestTenantPDFParserReferenceCleaning(t *testing.T) { for _, ref := range []string{"model-b", "SoMark-model@b@SoMark", "OpenDataLoader-model@b@OpenDataLoader"} { for _, field := range []string{"parse_method", "layout_recognizer"} { t.Run(ref+"/"+field, func(t *testing.T) { - cleaned := CleanComponentParams(generalDSL(t), map[string]any{ + cleaned := buildTestParserConfig(t, generalDSL(t), map[string]any{ "Parser:HipSignsRhyme": map[string]any{ "pdf": map[string]any{"parse_method": "deepdoc", field: ref}, }, diff --git a/internal/ingestion/pipeline/parser_vision_config_test.go b/internal/ingestion/pipeline/parser_vision_config_test.go index 5e2666d3e9..2701b8a27e 100644 --- a/internal/ingestion/pipeline/parser_vision_config_test.go +++ b/internal/ingestion/pipeline/parser_vision_config_test.go @@ -11,7 +11,7 @@ func TestBuildParserConfigPreservesGlobalVisionSettings(t *testing.T) { for _, enabled := range []bool{true, false} { for _, model := range []string{"model-B", ""} { want := map[string]any{"enable_vision_enhancement": enabled, "vlm": map[string]any{"llm_id": model}} - params := BuildParserConfig(generalDSL(t), map[string]any{id: want})[id].(map[string]any) + params := buildTestParserConfig(t, generalDSL(t), map[string]any{id: want})[id].(map[string]any) for key, value := range want { if !reflect.DeepEqual(params[key], value) { t.Fatalf("existing canvas saved %s = %v, want %v", key, params[key], value) @@ -68,7 +68,7 @@ func TestBuiltinParserVisionSettings(t *testing.T) { } for _, enabled := range []bool{true, false} { want := map[string]any{"enable_vision_enhancement": enabled, "vlm": map[string]any{"llm_id": "model-B"}} - params := BuildParserConfig(dsl, map[string]any{id: want})[id].(map[string]any) + params := buildTestParserConfig(t, dsl, map[string]any{id: want})[id].(map[string]any) for key, value := range want { if !reflect.DeepEqual(params[key], value) { t.Fatalf("saved %s = %v, want %v", key, params[key], value) diff --git a/internal/ingestion/pipeline/pipeline_params.go b/internal/ingestion/pipeline/pipeline_params.go index 304019b75e..000d44016e 100644 --- a/internal/ingestion/pipeline/pipeline_params.go +++ b/internal/ingestion/pipeline/pipeline_params.go @@ -40,7 +40,7 @@ var extractorValidParamKeys = func() map[string]struct{} { }() // componentParamSchemaKeys maps a DSL component_name (lower-cased) to the -// param keys its param schema declares. CleanComponentParams unions these with +// param keys its param schema declares. cleanComponentParams unions these with // the keys a template already bakes, so a param the component can read is // never dropped just because a template — or a canvas snapshot saved before // the param existed — does not carry it. @@ -186,21 +186,14 @@ func getComponentParamWhitelist(cpnID string) (map[string]struct{}, bool) { return nil, false } -// CleanComponentParams filters rawConfig against the DSL schema given by dslJSON. +// cleanComponentParams filters rawConfig against the extracted DSL schemas. // Keys containing ':' are treated as component IDs; they are kept only when both // the cpnID AND the param name exist in one of the accepted-key sources: the DSL // schema (the params the template bakes), the schema-derived per-component table // (componentParamSchemaKeys), or the component's dynamic parameter schema (e.g. // Extractor modular features). Keys without ':' (legacy flat fields) are dropped // with a warning. -func CleanComponentParams(dslJSON []byte, rawConfig map[string]interface{}) map[string]interface{} { - schemas, err := ExtractAllComponentParams(dslJSON) - if err != nil { - common.Warn("CleanComponentParams: failed to extract DSL schema, returning input as-is", - zap.Error(err)) - return rawConfig - } - +func cleanComponentParams(schemas []ComponentParamsSchema, rawConfig map[string]interface{}) map[string]interface{} { validCPNs := make(map[string]map[string]struct{}, len(schemas)) componentNames := make(map[string]string, len(schemas)) for _, s := range schemas { @@ -233,13 +226,13 @@ func CleanComponentParams(dslJSON []byte, rawConfig map[string]interface{}) map[ result := make(map[string]interface{}, len(rawConfig)) for key, val := range rawConfig { if !strings.Contains(key, ":") { - common.Warn("CleanComponentParams: dropping legacy flat field", + common.Warn("cleanComponentParams: dropping legacy flat field", zap.String("key", key)) continue } validKeys, ok := validCPNs[key] if !ok { - common.Warn("CleanComponentParams: dropping unknown cpnID", + common.Warn("cleanComponentParams: dropping unknown cpnID", zap.String("cpnID", key)) continue } @@ -274,7 +267,7 @@ func CleanComponentParams(dslJSON []byte, rawConfig map[string]interface{}) map[ continue } } - common.Warn("CleanComponentParams: dropping unknown param", + common.Warn("cleanComponentParams: dropping unknown param", zap.String("cpnID", key), zap.String("param", pk)) } if len(cleaned) > 0 { @@ -374,17 +367,16 @@ func NormalizeExtractorParams(raw map[string]any) map[string]any { // BuildParserConfig builds the final parser_config by starting from the DSL // defaults for every component, then overlaying the cleaned incoming overrides. // This ensures all components from the current pipeline are present while -// stripping stale params from other pipelines. -func BuildParserConfig(dslJSON []byte, rawConfig map[string]interface{}) entity.JSONMap { - cleaned := CleanComponentParams(dslJSON, rawConfig) - defaults, err := ComponentParamsDefaults(dslJSON) +// stripping stale params from other pipelines. Invalid DSL returns an error. +func BuildParserConfig(dslJSON []byte, rawConfig map[string]interface{}) (entity.JSONMap, error) { + schemas, err := ExtractAllComponentParams(dslJSON) if err != nil { - common.Warn("BuildParserConfig: failed to extract DSL defaults, using cleaned only", - zap.Error(err)) - return entity.JSONMap(cleaned) + return nil, err } - result := make(entity.JSONMap, len(defaults)) - for cpnID, params := range defaults { + cleaned := cleanComponentParams(schemas, rawConfig) + result := make(entity.JSONMap, len(schemas)) + for _, component := range schemas { + cpnID, params := component.CpnID, component.ParamsDefaults base := make(map[string]interface{}, len(params)) for k, v := range params { base[k] = v @@ -398,7 +390,7 @@ func BuildParserConfig(dslJSON []byte, rawConfig map[string]interface{}) entity. } result[cpnID] = base } - return result + return result, nil } // ApplyParentChildChunkerConfig derives runtime children_delimiters from the diff --git a/internal/ingestion/pipeline/pipeline_params_test.go b/internal/ingestion/pipeline/pipeline_params_test.go index c56b3d2208..95f0eb9165 100644 --- a/internal/ingestion/pipeline/pipeline_params_test.go +++ b/internal/ingestion/pipeline/pipeline_params_test.go @@ -9,6 +9,15 @@ import ( "ragflow/internal/entity" ) +func buildTestParserConfig(t *testing.T, dsl []byte, overrides map[string]any) entity.JSONMap { + t.Helper() + config, err := BuildParserConfig(dsl, overrides) + if err != nil { + t.Fatal(err) + } + return config +} + // generalDSL returns a minimal DSL resembling the "general" template's component // structure, with one Parser and one Chunker component. func generalDSL(t *testing.T) []byte { @@ -44,7 +53,7 @@ func generalDSL(t *testing.T) []byte { return raw } -func TestCleanComponentParamsNormalizesGeneralLegacyDelimiter(t *testing.T) { +func TestBuildParserConfigNormalizesGeneralLegacyDelimiter(t *testing.T) { dsl := map[string]any{ "components": map[string]any{ "GeneralChunker:Legacy": map[string]any{ @@ -61,7 +70,7 @@ func TestCleanComponentParamsNormalizesGeneralLegacyDelimiter(t *testing.T) { if err != nil { t.Fatalf("marshal dsl: %v", err) } - result := CleanComponentParams(dslJSON, map[string]any{ + result := buildTestParserConfig(t, dslJSON, map[string]any{ "GeneralChunker:Legacy": map[string]any{ "delimiter": "\n!?;。；！？", }, @@ -96,7 +105,11 @@ func TestCleanComponentParamsNormalizesDelimiterWhenDSLDeclaresOnlyLegacyKey(t * if err != nil { t.Fatalf("marshal DSL: %v", err) } - result := CleanComponentParams(dslJSON, map[string]any{ + schemas, err := ExtractAllComponentParams(dslJSON) + if err != nil { + t.Fatal(err) + } + result := cleanComponentParams(schemas, map[string]any{ "GeneralChunker:Custom": map[string]any{ "delimiter": "\n!?", }, @@ -114,30 +127,21 @@ func TestCleanComponentParamsNormalizesDelimiterWhenDSLDeclaresOnlyLegacyKey(t * } } -func TestCleanComponentParams_DropsLegacyFlatFields(t *testing.T) { +func TestBuildParserConfig_DropsLegacyFlatFields(t *testing.T) { dslJSON := generalDSL(t) raw := map[string]any{ "chunk_token_num": 256, "image_context_size": 10, } - result := CleanComponentParams(dslJSON, raw) - if len(result) != 0 { - t.Errorf("expected empty result, got %v", result) + result := buildTestParserConfig(t, dslJSON, raw) + for key := range raw { + if _, exists := result[key]; exists { + t.Errorf("flat field %q survived config building: %v", key, result) + } } } -func TestCleanComponentParams_DropsUnknownCPNID(t *testing.T) { - dslJSON := generalDSL(t) - raw := map[string]any{ - "Parser:NoSuch": map[string]any{"chunk_size": float64(256)}, - } - result := CleanComponentParams(dslJSON, raw) - if _, ok := result["Parser:NoSuch"]; ok { - t.Error("expected unknown cpnID to be dropped") - } -} - -func TestCleanComponentParams_DropsUnknownParamKey(t *testing.T) { +func TestBuildParserConfig_DropsUnknownParamKey(t *testing.T) { dslJSON := generalDSL(t) raw := map[string]any{ "Parser:HipSignsRhyme": map[string]any{ @@ -145,7 +149,7 @@ func TestCleanComponentParams_DropsUnknownParamKey(t *testing.T) { "pdf": map[string]any{"parse_method": "deepdoc"}, }, } - result := CleanComponentParams(dslJSON, raw) + result := buildTestParserConfig(t, dslJSON, raw) params := result["Parser:HipSignsRhyme"].(map[string]any) if _, ok := params["no_such_param"]; ok { t.Error("expected unknown param key to be dropped") @@ -155,32 +159,6 @@ func TestCleanComponentParams_DropsUnknownParamKey(t *testing.T) { } } -func TestCleanComponentParams_ReturnsInputOnDSLError(t *testing.T) { - result := CleanComponentParams([]byte("not json"), map[string]any{"key": "val"}) - if result["key"] != "val" { - t.Error("expected input returned as-is on DSL error") - } -} - -func TestCleanComponentParams_ValidCPNIDPassesThrough(t *testing.T) { - dslJSON := generalDSL(t) - raw := map[string]any{ - "Parser:HipSignsRhyme": map[string]any{ - "pdf": map[string]any{"parse_method": "deepdoc"}, - }, - "Chunker:LegalReadersDecide": map[string]any{ - "chunk_size": float64(256), - }, - } - result := CleanComponentParams(dslJSON, raw) - if _, ok := result["Parser:HipSignsRhyme"]; !ok { - t.Error("expected Parser:HipSignsRhyme to pass through") - } - if _, ok := result["Chunker:LegalReadersDecide"]; !ok { - t.Error("expected Chunker:LegalReadersDecide to pass through") - } -} - // --- BuildParserConfig --- func TestBuildParserConfig_ShallowMerge_NestedParam(t *testing.T) { @@ -213,7 +191,7 @@ func TestBuildParserConfig_ShallowMerge_NestedParam(t *testing.T) { }, } - result := BuildParserConfig(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) chunker, ok := result["Chunker:xyz"].(map[string]any) if !ok { t.Fatal("expected Chunker:xyz in result") @@ -266,7 +244,7 @@ func TestBuildParserConfig_FlattensNestedParserSetups(t *testing.T) { }, } - result := BuildParserConfig(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) parser, ok := result["Parser:HipSignsRhyme"].(map[string]any) if !ok { t.Fatal("expected Parser:HipSignsRhyme in result") @@ -297,54 +275,6 @@ func TestBuildParserConfig_FlattensNestedParserSetups(t *testing.T) { } } -// CleanComponentParams must flatten a legacy nested "setups" group before -// filtering, otherwise the lifted families are dropped as unknown params -// against the (flat) DSL schema keys. -func TestCleanComponentParams_FlattensNestedParserSetups(t *testing.T) { - dsl := map[string]any{ - "components": map[string]any{ - "Parser:HipSignsRhyme": map[string]any{ - "obj": map[string]any{ - "component_name": "Parser", - "params": map[string]any{ - "setups": map[string]any{ - "pdf": map[string]any{"parse_method": "deepdoc"}, - }, - }, - }, - }, - }, - } - dslJSON, err := json.Marshal(dsl) - if err != nil { - t.Fatalf("marshal dsl: %v", err) - } - - rawConfig := map[string]any{ - "Parser:HipSignsRhyme": map[string]any{ - "setups": map[string]any{ - "pdf": map[string]any{"parse_method": "vision"}, - }, - }, - } - - result := CleanComponentParams(dslJSON, rawConfig) - parser, ok := result["Parser:HipSignsRhyme"].(map[string]any) - if !ok { - t.Fatal("expected Parser:HipSignsRhyme in result") - } - if _, ok := parser["setups"]; ok { - t.Error("nested \"setups\" key must be flattened away") - } - pdf, ok := parser["pdf"].(map[string]any) - if !ok { - t.Fatalf("expected flattened pdf family, got %T", parser["pdf"]) - } - if pdf["parse_method"] != "vision" { - t.Errorf("pdf parse_method = %v, want vision", pdf["parse_method"]) - } -} - // The runtime passes Doc.ParserConfig as override_params, where top-level // keys from the override replace the DSL-baked ones. A legacy nested Parser // entry must be lifted BEFORE that merge — otherwise the DSL's flat families @@ -422,7 +352,7 @@ func TestBuildParserConfig_ScalarOverridePreservesOtherDefaults(t *testing.T) { "chunk_size": float64(1024), }, } - result := BuildParserConfig(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) chunker, ok := result["Chunker:LegalReadersDecide"].(map[string]any) if !ok { t.Fatal("expected Chunker:LegalReadersDecide in result") @@ -441,8 +371,8 @@ func TestBuildParserConfig_UnknownCPNIDNotPresentInResult(t *testing.T) { overrides := map[string]any{ "Parser:Unknown": map[string]any{"chunk_size": float64(256)}, } - result := BuildParserConfig(dslJSON, overrides) - // Unknown cpnID should be dropped by CleanComponentParams; the result should + result := buildTestParserConfig(t, dslJSON, overrides) + // Unknown cpnID should be dropped by BuildParserConfig; the result should // still contain the DSL-defined components with their defaults. if _, ok := result["Parser:Unknown"]; ok { t.Error("expected unknown cpnID to be absent from result") @@ -452,16 +382,18 @@ func TestBuildParserConfig_UnknownCPNIDNotPresentInResult(t *testing.T) { } } -func TestBuildParserConfig_FallbackOnDSLError(t *testing.T) { - result := BuildParserConfig([]byte("not json"), map[string]any{"key": "val"}) - if result["key"] != "val" { - t.Error("expected fallback to return raw config on DSL error") +func TestBuildParserConfigRejectsInvalidDSL(t *testing.T) { + for _, dsl := range []string{"not json", `{}`, `{"components":{"Extractor:first":{},"Extractor:second":{}}}`} { + result, err := BuildParserConfig([]byte(dsl), map[string]any{"key": "val"}) + if err == nil || result != nil { + t.Fatalf("invalid DSL must return an error without config: config=%#v err=%v", result, err) + } } } func TestBuildParserConfig_AllComponentsPresent(t *testing.T) { dslJSON := generalDSL(t) - result := BuildParserConfig(dslJSON, nil) + result := buildTestParserConfig(t, dslJSON, nil) // Both components from the DSL fixture should be present. if _, ok := result["Parser:HipSignsRhyme"]; !ok { t.Error("expected Parser:HipSignsRhyme") @@ -500,7 +432,7 @@ func TestBuildParserConfig_BuiltinExtractorKeepsTagFileID(t *testing.T) { "tags": map[string]any{"top_n": 5, "tag_file_id": "file-123"}, }, } - result := BuildParserConfig(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) params, ok := result[s.CpnID].(map[string]any) if !ok { t.Fatalf("template %q: expected component %q in result", ref, s.CpnID) @@ -606,13 +538,13 @@ func TestBuildParserConfig_BuiltinExtractorKeepsBuiltInMetadata(t *testing.T) { }, }, } - result := BuildParserConfig(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) params, ok := result[s.CpnID].(map[string]any) if !ok { t.Fatalf("template %q: expected component %q in result", ref, s.CpnID) } if _, ok := params["metadata"]; !ok { - t.Errorf("template %q: metadata dropped by CleanComponentParams", ref) + t.Errorf("template %q: metadata dropped by BuildParserConfig", ref) } } } @@ -642,7 +574,7 @@ func TestApplyParentChildChunkerConfig(t *testing.T) { } } -func TestCleanComponentParams_KeepsModularExtractorParams(t *testing.T) { +func TestBuildParserConfig_KeepsModularExtractorParams(t *testing.T) { // Builtin template with empty Extractor params: {} dsl := map[string]any{ "components": map[string]any{ @@ -680,7 +612,7 @@ func TestCleanComponentParams_KeepsModularExtractorParams(t *testing.T) { }, } - result := CleanComponentParams(dslJSON, overrides) + result := buildTestParserConfig(t, dslJSON, overrides) ext, ok := result["Extractor:AutoExtractDefault"].(map[string]any) if !ok { t.Fatalf("expected Extractor:AutoExtractDefault in result, got: %v", result) @@ -703,7 +635,7 @@ func TestCleanComponentParams_KeepsModularExtractorParams(t *testing.T) { } } -func TestCleanComponentParams_ExtractorBuiltInMetadataRetained(t *testing.T) { +func TestBuildParserConfig_ExtractorBuiltInMetadataRetained(t *testing.T) { dslJSON := []byte(`{ "components": { "Extractor:AutoExtractDefault": { @@ -732,7 +664,7 @@ func TestCleanComponentParams_ExtractorBuiltInMetadataRetained(t *testing.T) { }, } - result := CleanComponentParams(dslJSON, rawConfig) + result := buildTestParserConfig(t, dslJSON, rawConfig) ext, ok := result["Extractor:AutoExtractDefault"].(map[string]any) if !ok { t.Fatalf("expected Extractor:AutoExtractDefault in result, got: %v", result) @@ -893,7 +825,7 @@ func compilerDSL(t *testing.T) []byte { return raw } -func TestCleanComponentParams_CompilerKeepsLLMRuntimeParams(t *testing.T) { +func TestBuildParserConfig_CompilerKeepsLLMRuntimeParams(t *testing.T) { dslJSON := compilerDSL(t) raw := map[string]any{ "Compiler:NewBoxesLove": map[string]any{ @@ -914,7 +846,7 @@ func TestCleanComponentParams_CompilerKeepsLLMRuntimeParams(t *testing.T) { "mode": "structure", }, } - result := CleanComponentParams(dslJSON, raw) + result := buildTestParserConfig(t, dslJSON, raw) params, ok := result["Compiler:NewBoxesLove"].(map[string]any) if !ok { t.Fatalf("expected Compiler params to survive, got %#v", result) @@ -953,7 +885,7 @@ func TestBuildParserConfig_CompilerRuntimeParamsSurvive(t *testing.T) { "thinking": "disabled", }, } - result := BuildParserConfig(dslJSON, raw) + result := buildTestParserConfig(t, dslJSON, raw) params, ok := result["Compiler:NewBoxesLove"].(map[string]any) if !ok { t.Fatalf("expected Compiler entry in built parser_config, got %#v", result) @@ -1001,7 +933,7 @@ func titleFamilyDSL(t *testing.T) []byte { return raw } -func TestCleanComponentParams_TitleFamilyKeepsSchemaParamsWithoutBakedKeys(t *testing.T) { +func TestBuildParserConfig_TitleFamilyKeepsSchemaParamsWithoutBakedKeys(t *testing.T) { dslJSON := titleFamilyDSL(t) raw := map[string]any{ "TitleChunker:StaleSnapshot": map[string]any{ @@ -1009,7 +941,7 @@ func TestCleanComponentParams_TitleFamilyKeepsSchemaParamsWithoutBakedKeys(t *te "root_chunk_as_heading": true, }, } - result := CleanComponentParams(dslJSON, raw) + result := buildTestParserConfig(t, dslJSON, raw) params, ok := result["TitleChunker:StaleSnapshot"].(map[string]any) if !ok { t.Fatalf("expected TitleChunker params to survive, got %#v", result) @@ -1025,7 +957,7 @@ func TestCleanComponentParams_TitleFamilyKeepsSchemaParamsWithoutBakedKeys(t *te // TestBuildParserConfig_KeepsTitleChunkerPerMethodKeys locks the persistence // contract for the title-chunker family: the frontend stores each method's // rules, hierarchy value and token cap under its own key so the dataset -// config keeps Hierarchy and Group independent; CleanComponentParams must not +// config keeps Hierarchy and Group independent; BuildParserConfig must not // drop them. func TestBuildParserConfig_KeepsTitleChunkerPerMethodKeys(t *testing.T) { dslJSON := titleFamilyDSL(t) @@ -1042,7 +974,7 @@ func TestBuildParserConfig_KeepsTitleChunkerPerMethodKeys(t *testing.T) { "groupChunkTokenCap": 2048, }, } - params, ok := BuildParserConfig(dslJSON, overrides)["TitleChunker:StaleSnapshot"].(map[string]any) + params, ok := buildTestParserConfig(t, dslJSON, overrides)["TitleChunker:StaleSnapshot"].(map[string]any) if !ok { t.Fatalf("expected TitleChunker params to survive, got %#v", params) } @@ -1091,7 +1023,7 @@ func TestBuildParserConfig_TitleFamilyParamsSurviveBuiltinTemplates(t *testing.T "root_chunk_as_heading": true, }, } - params, ok := BuildParserConfig(dslJSON, overrides)[s.CpnID].(map[string]any) + params, ok := buildTestParserConfig(t, dslJSON, overrides)[s.CpnID].(map[string]any) if !ok { t.Fatalf("template %q: component %q missing from built parser_config", ref, s.CpnID) } @@ -1111,7 +1043,7 @@ func TestBuildParserConfig_TitleFamilyParamsSurviveBuiltinTemplates(t *testing.T // operatorFormParamKeys mirrors the params the dataset/document pipeline // operator forms submit (transformFormConfigToApi in // web/src/utils/pipeline-operator.ts). Every key listed for a component must be -// accepted by CleanComponentParams for that component in every builtin +// accepted by BuildParserConfig for that component in every builtin // template; otherwise a user edit is silently dropped. var operatorFormParamKeys = map[string][]string{ "TitleChunker": { @@ -1140,7 +1072,7 @@ var operatorFormParamKeys = map[string][]string{ "Extractor": {"llm_id", "keywords", "questions", "tags", "summary", "metadata"}, } -func TestCleanComponentParams_AcceptsOperatorFormParams(t *testing.T) { +func TestBuildParserConfig_AcceptsOperatorFormParams(t *testing.T) { registry, err := DefaultRegistry() if err != nil { t.Fatalf("DefaultRegistry: %v", err) @@ -1169,7 +1101,7 @@ func TestCleanComponentParams_AcceptsOperatorFormParams(t *testing.T) { for _, key := range expected { params[key] = "probe" } - cleaned := CleanComponentParams(dslJSON, map[string]interface{}{s.CpnID: params}) + cleaned := buildTestParserConfig(t, dslJSON, map[string]interface{}{s.CpnID: params}) got, ok := cleaned[s.CpnID].(map[string]any) if !ok { t.Fatalf("template %q: component %q missing from cleaned result", ref, s.CpnID) @@ -1245,9 +1177,9 @@ func TestBuiltinTemplateComponentsHaveWhitelistCoverage(t *testing.T) { } } -// TestCleanComponentParams_TitleFamilyStillDropsUnknownKeys keeps the union +// TestBuildParserConfig_TitleFamilyStillDropsUnknownKeys keeps the union // honest: a key the component schema does not declare is still dropped. -func TestCleanComponentParams_TitleFamilyStillDropsUnknownKeys(t *testing.T) { +func TestBuildParserConfig_TitleFamilyStillDropsUnknownKeys(t *testing.T) { dslJSON := titleFamilyDSL(t) raw := map[string]any{ "TitleChunker:StaleSnapshot": map[string]any{ @@ -1255,7 +1187,7 @@ func TestCleanComponentParams_TitleFamilyStillDropsUnknownKeys(t *testing.T) { "method": "hierarchy", }, } - result := CleanComponentParams(dslJSON, raw) + result := buildTestParserConfig(t, dslJSON, raw) params, ok := result["TitleChunker:StaleSnapshot"].(map[string]any) if !ok { t.Fatalf("expected TitleChunker params to survive, got %#v", result) diff --git a/internal/service/dataset/crud.go b/internal/service/dataset/crud.go index 77326334cf..ef732b2b0b 100644 --- a/internal/service/dataset/crud.go +++ b/internal/service/dataset/crud.go @@ -144,7 +144,11 @@ func (d *DatasetService) CreateDataset(ctx context.Context, req *service.CreateD common.Warn("failed to load pipeline DSL for building parser_config", zap.String("parserID", parserID), zap.Error(dslErr)) } else { - parserConfig = pipelinepkg.BuildParserConfig(dslJSON, req.ParserConfig) + var buildErr error + parserConfig, buildErr = pipelinepkg.BuildParserConfig(dslJSON, req.ParserConfig) + if buildErr != nil { + return nil, common.CodeArgumentError, buildErr + } } // Preserve the public default shape without overriding pipeline delimiters diff --git a/internal/service/dataset/helpers.go b/internal/service/dataset/helpers.go index afc34ce5e3..fbc30d6ce3 100644 --- a/internal/service/dataset/helpers.go +++ b/internal/service/dataset/helpers.go @@ -206,7 +206,7 @@ func validateDatasetParserConfigSize(parserConfig map[string]interface{}) error // not component-scoped (i.e. do not contain ":"), since the Go backend only // consumes keys keyed by a component id (e.g. "Parser:abc123" or // "Extractor:AutoExtractDefault"). Flat keys are legacy transport artifacts -// that downstream consumers (CleanComponentParams) never read, so they are +// that downstream consumers (BuildParserConfig) never read, so they are // dropped rather than rejected. Only top-level keys are affected; a key nested // inside a component node (e.g. "Extractor:AutoExtractDefault.metadata") keeps // its existing name. It mutates the provided map in place and returns the names diff --git a/internal/service/dataset/update.go b/internal/service/dataset/update.go index f1b9fb77fd..938a1c21f2 100644 --- a/internal/service/dataset/update.go +++ b/internal/service/dataset/update.go @@ -301,7 +301,11 @@ func (d *DatasetService) UpdateDataset(ctx context.Context, datasetID, userID st zap.String("parserID", effParserID), zap.Error(dslErr)) } if dslJSON != nil { - parserConfig := pipelinepkg.BuildParserConfig(dslJSON, map[string]interface{}(req.ParserConfig)) + parserConfig, buildErr := pipelinepkg.BuildParserConfig(dslJSON, map[string]interface{}(req.ParserConfig)) + if buildErr != nil { + txCode = common.CodeDataError + return buildErr + } updates["parser_config"] = preserveDatasetParserConfigState(parserConfig, lockedKB.ParserConfig, req.ParserConfig) } } diff --git a/internal/service/document/document_dataset_update.go b/internal/service/document/document_dataset_update.go index 83e256c786..2e59553522 100644 --- a/internal/service/document/document_dataset_update.go +++ b/internal/service/document/document_dataset_update.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "maps" "math" "path/filepath" "ragflow/internal/service" @@ -13,12 +14,12 @@ import ( "ragflow/internal/common" "ragflow/internal/dao" "ragflow/internal/entity" + componentpkg "ragflow/internal/ingestion/component" pipelinepkg "ragflow/internal/ingestion/pipeline" "ragflow/internal/permission" permissionresponse "ragflow/internal/permission/response" "ragflow/internal/tokenizer" - - "go.uber.org/zap" + "ragflow/internal/utility" ) func (s *DocumentService) BatchUpdateDocumentStatus(ctx context.Context, userID, datasetID, status string, documentIDs []string) (map[string]interface{}, common.ErrorCode, error) { @@ -155,35 +156,40 @@ func (s *DocumentService) UpdateDatasetDocument(ctx context.Context, userID, dat } return nil, common.CodeServerError, err } - if code, err := s.validateDatasetDocumentUpdate(ctx, datasetID, documentID, userID, doc, req, present); err != nil { + if code, err := s.validateDatasetDocumentUpdate(ctx, doc, req, present); err != nil { return nil, code, err } - if present["meta_fields"] { - if err = s.replaceDocumentMetadata(ctx, documentID, req.MetaFields); err != nil { - return nil, common.CodeDataError, err - } + // Normalize chunk_method into the same selection used to clean and save + // parser_config. An omitted selection keeps the document's own template. + parseType, parserID := req.ParseType, req.ParserID + hasSelection := parseType != nil || + (parserID != nil && strings.TrimSpace(*parserID) != "") || + (req.PipelineID != nil && strings.TrimSpace(*req.PipelineID) != "") + if !hasSelection && present["chunk_method"] { + builtin := int(service.ParseModeBuiltIn) + parseType, parserID = &builtin, req.ChunkMethod } - - if present["name"] && req.Name != nil && (doc.Name == nil || *req.Name != *doc.Name) { - if err = s.updateDocumentNameOnly(ctx, doc, kb, *req.Name); err != nil { - return nil, common.CodeDataError, err - } - } - - // Resolve the effective parse mode once via the shared selection value - // object. sel is the request selection (nil when the whole selection block - // was omitted); current is the dataset's persisted selection. Resolve applies - // PATCH semantics: an omitted block inherits the current mode. Both the - // parser_config cleaning and the parser selection derive from this single - // resolution so they can never disagree. - sel, selErr := service.FromRequest(req.ParseType, req.ParserID, req.PipelineID) + sel, selErr := service.FromRequest(parseType, parserID, req.PipelineID) if selErr != nil { return nil, common.CodeDataError, selErr } - current := service.CurrentSelection(kb.ParserID, kb.PipelineID) + current := service.CurrentSelection(doc.ParserID, doc.PipelineID) eff, _ := service.Resolve(current, sel) isPipeline, effParserID, effPipelineID := eff.Effective() + selectionChanged := current.Mode != eff.Mode || + (isPipeline && current.PipelineID != eff.PipelineID) || + (!isPipeline && current.ParserID != effParserID) + if selectionChanged && current.IsBuiltIn() && eff.IsBuiltIn() { + registry, err := pipelinepkg.DefaultRegistry() + if err != nil { + return nil, common.CodeDataError, fmt.Errorf("builtin pipeline registry: %w", err) + } + previous, previousOK := registry.Get(current.ParserID) + target, targetOK := registry.Get(effParserID) + selectionChanged = !previousOK || !targetOK || previous.ParserID != target.ParserID + } + hasConfig := present["parser_config"] && req.ParserConfig != nil if (sel != nil || present["parser_config"]) && isPipeline && effPipelineID != nil && strings.TrimSpace(*effPipelineID) != "" { accessErr := permission.NewDatabaseChecker(dao.DB).CheckDependency( ctx, @@ -199,56 +205,66 @@ func (s *DocumentService) UpdateDatasetDocument(ctx context.Context, userID, dat } } - if present["parser_config"] && req.ParserConfig != nil { - var dslJSON []byte - dslJSON, err = service.LoadPipelineDSL(ctx, isPipeline, effParserID, effPipelineID) + parserUpdates := map[string]interface{}{} + if sel != nil || hasConfig { + // Validate the target before any document, file, or metadata writes. + dslJSON, err := service.LoadPipelineDSL(ctx, isPipeline, effParserID, effPipelineID) if err != nil { - common.Warn("cleanAndUpdateDocumentParserConfig: failed to load DSL, falling back to merge", - zap.Error(err)) - if err = s.updateDocumentParserConfig(ctx, doc.ID, req.ParserConfig); err != nil { - return nil, common.CodeDataError, err + return nil, common.CodeDataError, err + } + rawConfig := req.ParserConfig + if !selectionChanged { + rawConfig = common.DeepMergeMaps(doc.ParserConfig, req.ParserConfig) + } + cleaned, err := pipelinepkg.BuildParserConfig(dslJSON, rawConfig) + if err != nil { + return nil, common.CodeDataError, fmt.Errorf("invalid pipeline DSL: %w", err) + } + if doc.Name != nil { + family := componentpkg.ParserFileFamily(utility.GetFileExtension(*doc.Name)) + if family != "" { + if declared, found := configDeclaredFamily(cleaned, family); found && !declared { + return nil, common.CodeDataError, fmt.Errorf("selected pipeline does not support file type %q", family) + } } - } else { - cleaned := pipelinepkg.BuildParserConfig(dslJSON, req.ParserConfig) - pipelinepkg.ApplyParentChildChunkerConfig(cleaned, req.ParserConfig) + } + if selectionChanged || hasConfig { + // Inherited runtime delimiters are not an explicit override when + // this request changes parent_child. + parentChildOverrides := maps.Clone(rawConfig) + for componentID, raw := range req.ParserConfig { + if requested, ok := raw.(map[string]any); ok { + if _, provided := requested["parent_child"]; provided { + parentChildOverrides[componentID] = requested + } + } + } + pipelinepkg.ApplyParentChildChunkerConfig(cleaned, parentChildOverrides) llmID, _, _ := service.NewTenantService().GetDefaultModelRefsForUser(ctx, userID, kb.TenantID, entity.ModelTypeChat) cleaned = service.ApplyComponentScopedParserConfig(cleaned, llmID) - if err = s.documentDAO.UpdateByID(ctx, dao.DB, doc.ID, map[string]interface{}{ - "parser_config": cleaned, - }); err != nil { - return nil, common.CodeDataError, err - } + parserUpdates["parser_config"] = cleaned } } - // Save the selection without changing existing parse results. Cleanup is - // owned by the explicit reparse request. - parserUpdates := map[string]interface{}{} - if req.ParseType != nil { - switch { - case !isPipeline: // BuiltIn - if present["parser_id"] && req.ParserID != nil { - if p := strings.TrimSpace(*req.ParserID); p != "" { - parserUpdates["parser_id"] = p - } - } - // Drop any prior canvas so the worker falls back to the builtin template. - parserUpdates["pipeline_id"] = "" - case isPipeline: // Pipeline - if present["pipeline_id"] && req.PipelineID != nil { - parserUpdates["pipeline_id"] = *req.PipelineID - } - } - } else if present["chunk_method"] && req.ChunkMethod != nil { - // chunk_method is the public alias for a direct parser change; it does - // not require parse_type (mirrors Python's update_chunk_method). - if p := strings.TrimSpace(*req.ChunkMethod); !strings.EqualFold(p, doc.ParserID) { - parserUpdates["parser_id"] = p - parserUpdates["pipeline_id"] = "" - } else if doc.PipelineID != nil && *doc.PipelineID != "" { + if sel != nil { + if isPipeline { + parserUpdates["pipeline_id"] = *effPipelineID + } else { + parserUpdates["parser_id"] = effParserID parserUpdates["pipeline_id"] = "" } } + if present["meta_fields"] { + if err = s.replaceDocumentMetadata(ctx, documentID, req.MetaFields); err != nil { + return nil, common.CodeDataError, err + } + } + if present["name"] && req.Name != nil && (doc.Name == nil || *req.Name != *doc.Name) { + if err = s.updateDocumentNameOnly(ctx, doc, kb, *req.Name); err != nil { + return nil, common.CodeDataError, err + } + } + // Save selection and config together; parsed content changes only on reparse. if len(parserUpdates) > 0 { if err = s.documentDAO.UpdateByID(ctx, dao.DB, doc.ID, parserUpdates); err != nil { return nil, common.CodeDataError, err @@ -308,7 +324,7 @@ func (s *DocumentService) validateDocumentModifiable(ctx context.Context, doc *e } } -func (s *DocumentService) validateDatasetDocumentUpdate(ctx context.Context, datasetID, documentID, userID string, doc *entity.Document, req *UpdateDatasetDocumentRequest, present map[string]bool) (common.ErrorCode, error) { +func (s *DocumentService) validateDatasetDocumentUpdate(ctx context.Context, doc *entity.Document, req *UpdateDatasetDocumentRequest, present map[string]bool) (common.ErrorCode, error) { if req == nil { return common.CodeDataError, errors.New("invalid request payload") } @@ -347,29 +363,15 @@ func (s *DocumentService) validateDatasetDocumentUpdate(ctx context.Context, dat if req.ChunkMethod == nil || strings.TrimSpace(*req.ChunkMethod) == "" { return common.CodeDataError, errors.New("`chunk_method` (empty string) is not valid") } - cm := strings.TrimSpace(*req.ChunkMethod) - if !validDocumentChunkMethods[cm] { - return common.CodeDataError, fmt.Errorf("Field: - Message: <`chunk_method` %s doesn't exist> - Value: <%s>", cm, cm) + registry, err := pipelinepkg.DefaultRegistry() + if err != nil { + return common.CodeDataError, fmt.Errorf("builtin pipeline registry: %w", err) } - if (doc.Type == "visual" && cm != "picture") || (isPresentationFile(doc.Name) && cm != "presentation") { - return common.CodeDataError, errors.New("not supported yet") + if !registry.IsValid(*req.ChunkMethod) { + return common.CodeDataError, fmt.Errorf("unknown builtin parser_id: %s", strings.TrimSpace(*req.ChunkMethod)) } } - if present["parse_type"] || present["parser_id"] || present["pipeline_id"] { - sel, err := service.FromRequest(req.ParseType, req.ParserID, req.PipelineID) - if err != nil { - return common.CodeDataError, err - } - // The parser_id type constraint (visual/presentation) only applies in - // builtin mode — in pipeline mode parser_id is not applicable. - if sel != nil && sel.IsBuiltIn() && present["parser_id"] && req.ParserID != nil { - parserID := strings.TrimSpace(*req.ParserID) - if (doc.Type == "visual" && parserID != "picture") || (isPresentationFile(doc.Name) && parserID != "presentation") { - return common.CodeDataError, errors.New("not supported yet") - } - } - } if present["name"] { // An explicit null name is a type error, not an unset field. if req.Name == nil { @@ -431,14 +433,6 @@ func (s *DocumentService) validateDocumentName(ctx context.Context, doc *entity. return common.CodeSuccess, nil } -func isPresentationFile(name *string) bool { - if name == nil { - return false - } - ext := strings.ToLower(filepath.Ext(*name)) - return ext == ".ppt" || ext == ".pptx" || ext == ".pages" || ext == ".dps" || ext == ".dpt" -} - func validateMetaFields(meta map[string]any) error { if meta == nil { return nil @@ -505,24 +499,6 @@ func (s *DocumentService) updateDocumentNameOnly(ctx context.Context, doc *entit ) } -func (s *DocumentService) updateDocumentParserConfig(ctx context.Context, documentID string, config map[string]any) error { - if len(config) == 0 { - return nil - } - - doc, err := s.documentDAO.GetByID(ctx, dao.DB, documentID) - if err != nil { - return fmt.Errorf("document(%s) not found", documentID) - } - - merged := common.DeepMergeMaps(doc.ParserConfig, config) - pipelinepkg.ApplyParentChildChunkerConfig(merged, config) - - return s.documentDAO.UpdateByID(ctx, dao.DB, documentID, map[string]interface{}{ - "parser_config": entity.JSONMap(merged), - }) -} - func (s *DocumentService) toUpdateDatasetDocumentResponse(ctx context.Context, doc *entity.Document, metaFields map[string]interface{}) (*UpdateDatasetDocumentResponse, error) { if metaFields == nil { metaFields = map[string]interface{}{} @@ -576,13 +552,6 @@ func (s *DocumentService) toUpdateDatasetDocumentResponse(ctx context.Context, d }, nil } -// validDocumentChunkMethods mirrors Python's UpdateDocumentReq chunk_method set. -var validDocumentChunkMethods = map[string]bool{ - "naive": true, "manual": true, "qa": true, "table": true, "paper": true, - "book": true, "laws": true, "presentation": true, "picture": true, - "one": true, "knowledge_graph": true, "email": true, "tag": true, -} - // pythonFloatRepr formats a float the way Python's str()/repr() does: // integral floats keep a trailing ".0". func pythonFloatRepr(v float64) string { diff --git a/internal/service/document/document_dataset_update_test.go b/internal/service/document/document_dataset_update_test.go index 5cda45b506..0f6be9a995 100644 --- a/internal/service/document/document_dataset_update_test.go +++ b/internal/service/document/document_dataset_update_test.go @@ -1,40 +1,476 @@ package document import ( + "fmt" + "reflect" + "strings" "testing" "ragflow/internal/common" + "ragflow/internal/dao" "ragflow/internal/entity" + "ragflow/internal/service" ) -// TestIsPresentationFile pins the name-based presentation guard used by the -// parser_id / chunk_method switch in document_dataset_update.go. A WPS -// presentation (dps/dpt) must be treated as a presentation on a par with ppt/ -// pptx/pages, so the API allows switching its chunk_method to "presentation". -func TestIsPresentationFile(t *testing.T) { - p := func(s string) *string { return &s } - - cases := []struct { - name string - in *string - want bool +func TestUpdateDatasetDocumentParentChildPatchUsesRequestedFields(t *testing.T) { + for _, tc := range []struct { + name string + enabled bool + children []any + patch map[string]any + want []any }{ - {"nil name is not presentation", nil, false}, - {"ppt is presentation", p("deck.ppt"), true}, - {"pptx is presentation", p("deck.pptx"), true}, - {"pages is presentation", p("deck.pages"), true}, - {"wps presentation dps is presentation", p("deck.dps"), true}, - {"wps presentation dpt is presentation", p("deck.dpt"), true}, - {"wps presentation dps is case-insensitive", p("DECK.DPS"), true}, - {"pdf is not presentation", p("report.pdf"), false}, - {"wps doc is not presentation", p("note.wps"), false}, - {"wps sheet et is not presentation", p("sheet.et"), false}, - } - - for _, tc := range cases { + {"enable", false, []any{}, map[string]any{"parent_child": map[string]any{"use_parent_child": true}}, []any{"|"}}, + {"disable", true, []any{"|"}, map[string]any{"parent_child": map[string]any{"use_parent_child": false}}, []any{}}, + {"delimiter", true, []any{"|"}, map[string]any{"parent_child": map[string]any{"children_delimiter": "!"}}, []any{"!"}}, + {"explicit children", true, []any{"|"}, map[string]any{"parent_child": map[string]any{"children_delimiter": "!"}, "children_delimiters": []any{"?"}}, []any{"?"}}, + {"unrelated patch", false, []any{"|"}, map[string]any{"chunk_token_size": 128}, []any{"|"}}, + } { t.Run(tc.name, func(t *testing.T) { - if got := isPresentationFile(tc.in); got != tc.want { - t.Fatalf("isPresentationFile(%v) = %v, want %v", tc.in, got, tc.want) + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.txt", 0, 0) + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("parser_config", entity.JSONMap{ + "GeneralChunker:SixApplesFall": map[string]any{ + "children_delimiters": tc.children, + "parent_child": map[string]any{"use_parent_child": tc.enabled, "children_delimiter": "|"}, + }, + }).Error; err != nil { + t.Fatal(err) + } + resp, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", &UpdateDatasetDocumentRequest{ + ParserConfig: map[string]any{"GeneralChunker:SixApplesFall": tc.patch}, + }, map[string]bool{"parser_config": true}) + if code != common.CodeSuccess || err != nil { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + for _, config := range []map[string]any{resp.ParserConfig, doc.ParserConfig} { + chunker, _ := config["GeneralChunker:SixApplesFall"].(map[string]any) + if !reflect.DeepEqual(chunker["children_delimiters"], tc.want) { + t.Fatalf("children_delimiters=%v, want=%v", chunker["children_delimiters"], tc.want) + } + } + }) + } +} + +func TestUpdateDatasetDocumentConfigPatchPreservesOmittedSettings(t *testing.T) { + for _, selection := range []string{"omitted", "same builtin", "same canvas"} { + t.Run(selection, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.docx", 0, 0) + config := entity.JSONMap{ + "Parser:HipSignsRhyme": map[string]any{ + "enable_vision_enhancement": true, + "vlm": map[string]any{"llm_id": "old-model", "system_prompt": "keep this prompt"}, + }, + "GeneralChunker:SixApplesFall": map[string]any{"chunk_token_size": 123}, + "Parser:Removed": map[string]any{"pdf": map[string]any{}}, + } + updates := map[string]any{"parser_id": "general", "parser_config": config} + req := &UpdateDatasetDocumentRequest{ParserConfig: map[string]any{ + "Parser:HipSignsRhyme": map[string]any{"vlm": map[string]any{"llm_id": "new-model"}}, + }} + present := map[string]bool{"parser_config": true} + switch selection { + case "same builtin": + req.ParseType, req.ParserID = new(1), new("general") + present["parse_type"], present["parser_id"] = true, true + case "same canvas": + const pipelineID = "1234567890abcdef1234567890abcdef" + insertTestCanvas(t, pipelineID, "user-1") + updates["pipeline_id"] = pipelineID + req.ParseType, req.PipelineID = new(2), new(pipelineID) + present["parse_type"], present["pipeline_id"] = true, true + } + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Updates(updates).Error; err != nil { + t.Fatal(err) + } + resp, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if err != nil || code != common.CodeSuccess { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + for _, saved := range []map[string]any{resp.ParserConfig, doc.ParserConfig} { + parser, _ := saved["Parser:HipSignsRhyme"].(map[string]any) + vlm, _ := parser["vlm"].(map[string]any) + chunker, _ := saved["GeneralChunker:SixApplesFall"].(map[string]any) + if parser["enable_vision_enhancement"] != true || vlm["llm_id"] != "new-model" || vlm["system_prompt"] != "keep this prompt" || chunker["chunk_token_size"] != float64(123) { + t.Fatalf("partial update lost settings: %#v", saved) + } + if _, exists := saved["Parser:Removed"]; exists { + t.Fatalf("stale component survived cleaning: %#v", saved) + } + } + }) + } +} + +func TestUpdateDatasetDocumentRejectsUnsupportedFileFamilyBeforeWrites(t *testing.T) { + for _, filename := range []string{"image.png", "deck.pptx", "deck.pages", "DECK.DPS", "deck.dpt"} { + for _, selection := range []string{"chunk_method", "parser_id"} { + for _, withConfig := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/%s/config=%t", filename, selection, withConfig), func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", filename, 0, 0) + insertTestFile(t, "file-1", "folder-1", filename, &filename) + insertTestFile2Document(t, "f2d-1", "file-1", "doc-1") + if filename == "image.png" { + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Updates(map[string]any{ + "type": "visual", "parser_id": "picture", "parser_config": pictureParserConfig(), + }).Error; err != nil { + t.Fatal(err) + } + } + before, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + req := &UpdateDatasetDocumentRequest{Name: new("renamed." + strings.SplitN(filename, ".", 2)[1]), MetaFields: map[string]any{"author": "changed"}} + present := map[string]bool{"name": true, "meta_fields": true, selection: true} + if selection == "parser_id" { + req.ParseType, req.ParserID = new(1), new("manual") + present["parse_type"] = true + } else { + req.ChunkMethod = new("manual") + } + if withConfig { + req.ParserConfig = map[string]any{"Parser:HipSignsRhyme": map[string]any{"enable_vision_enhancement": true}} + present["parser_config"] = true + } + engine := newMetadataDocEngine(map[string]map[string]any{"doc-1": {"author": "original"}}, map[string]string{"doc-1": "kb-1"}) + svc := testDocumentService(t) + svc.docEngine = engine + svc.metadataSvc = service.NewMetadataServiceForTest(dao.NewKnowledgebaseDAO(), engine) + _, code, err := svc.UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if code != common.CodeDataError || err == nil || !strings.Contains(err.Error(), "does not support") { + t.Fatalf("expected unsupported file rejection: code=%d err=%v", code, err) + } + after, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil || !reflect.DeepEqual(before, after) { + t.Fatalf("rejected request changed document: before=%#v after=%#v err=%v", before, after, err) + } + file, err := dao.NewFileDAO().GetByID(t.Context(), db, "file-1") + if err != nil || file.Name != filename { + t.Fatalf("rejected request changed file: file=%#v err=%v", file, err) + } + if !reflect.DeepEqual(engine.records["doc-1"], map[string]any{"author": "original"}) { + t.Fatalf("rejected request changed metadata: %#v", engine.records) + } + }) + } + } + } +} + +func TestUpdateDatasetDocumentParserAliasPreservesConfig(t *testing.T) { + for _, current := range []string{"naive", "general"} { + for _, selection := range []string{"chunk_method", "parser_id"} { + for _, withConfig := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/%s/config=%t", current, selection, withConfig), func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.docx", 0, 0) + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Updates(map[string]any{ + "parser_id": current, + "parser_config": entity.JSONMap{"GeneralChunker:SixApplesFall": map[string]any{"chunk_token_size": 123}}, + }).Error; err != nil { + t.Fatal(err) + } + target := "general" + if current == "general" { + target = "naive" + } + req := &UpdateDatasetDocumentRequest{} + present := map[string]bool{selection: true} + if selection == "parser_id" { + req.ParseType, req.ParserID = new(1), &target + present["parse_type"] = true + } else { + req.ChunkMethod = &target + } + if withConfig { + req.ParserConfig = map[string]any{"Parser:HipSignsRhyme": map[string]any{"enable_vision_enhancement": true}} + present["parser_config"] = true + } + resp, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if err != nil || code != common.CodeSuccess { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + if resp.ParserID != target || doc.ParserID != target { + t.Fatalf("parser ID not saved: response=%q stored=%q want=%q", resp.ParserID, doc.ParserID, target) + } + for _, config := range []map[string]any{resp.ParserConfig, doc.ParserConfig} { + chunker, _ := config["GeneralChunker:SixApplesFall"].(map[string]any) + if chunker["chunk_token_size"] != float64(123) { + t.Fatalf("alias switch reset chunk size: %#v", config) + } + if withConfig { + parser, _ := config["Parser:HipSignsRhyme"].(map[string]any) + if parser["enable_vision_enhancement"] != true { + t.Fatalf("alias switch lost config patch: %#v", config) + } + } + } + }) + } + } + } +} + +func TestUpdateDatasetDocumentSelectionWithoutConfig(t *testing.T) { + for _, selection := range []string{"chunk_method", "parser_id"} { + for _, current := range []string{"general", "manual"} { + t.Run(selection+"/"+current, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.docx", 0, 0) + config := entity.JSONMap{"ManualChunker:NineInsectsFind": map[string]any{"hierarchy": float64(7)}} + if current == "general" { + config = entity.JSONMap{"GeneralChunker:SixApplesFall": map[string]any{"chunk_token_size": 123}} + } + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Updates(map[string]any{"parser_id": current, "parser_config": config}).Error; err != nil { + t.Fatal(err) + } + req := &UpdateDatasetDocumentRequest{} + present := map[string]bool{selection: true} + if selection == "parser_id" { + req.ParseType, req.ParserID = new(1), new("manual") + present["parse_type"] = true + } else { + req.ChunkMethod = new("manual") + } + _, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if err != nil || code != common.CodeSuccess { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + if doc.ParserID != "manual" { + t.Fatalf("parser=%q", doc.ParserID) + } + if current == "manual" { + if !reflect.DeepEqual(doc.ParserConfig, config) { + t.Fatalf("unchanged selection reset config: %#v", doc.ParserConfig) + } + return + } + if _, ok := doc.ParserConfig["ManualChunker:NineInsectsFind"]; !ok { + t.Fatalf("new template config missing: %#v", doc.ParserConfig) + } + if _, ok := doc.ParserConfig["GeneralChunker:SixApplesFall"]; ok { + t.Fatalf("old template config retained: %#v", doc.ParserConfig) + } + parser, ok := doc.ParserConfig["Parser:HipSignsRhyme"].(map[string]any) + if !ok || parser["docx"] == nil { + t.Fatalf("target file family missing: %#v", doc.ParserConfig) + } + }) + } + } +} + +func TestUpdateDatasetDocumentChunkMethodAcceptsCurrentTemplates(t *testing.T) { + for _, file := range []struct{ method, name string }{{"general", "report.docx"}, {"audio", "recording.mp3"}} { + t.Run(file.method, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", file.name, 0, 0) + resp, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", &UpdateDatasetDocumentRequest{ + ChunkMethod: &file.method, + }, map[string]bool{"chunk_method": true}) + if err != nil || code != common.CodeSuccess { + t.Fatalf("select %s: code=%d err=%v", file.method, code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + if resp.ParserID != file.method || doc.ParserID != file.method { + t.Fatalf("selection not saved: response=%q stored=%q want=%q", resp.ParserID, doc.ParserID, file.method) + } + }) + } +} + +func TestUpdateDatasetDocumentUsesEffectiveParserForConfig(t *testing.T) { + for _, changeMethod := range []bool{false, true} { + name := "document selection" + if changeMethod { + name = "chunk method switch" + } + t.Run(name, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.docx", 0, 0) + req := &UpdateDatasetDocumentRequest{ParserConfig: map[string]any{ + "ManualChunker:NineInsectsFind": map[string]any{"hierarchy": 2}, + "GeneralChunker:SixApplesFall": map[string]any{"chunk_token_size": 123}, + }} + present := map[string]bool{"parser_config": true} + if changeMethod { + req.ChunkMethod = new("manual") + req.ParserID, req.PipelineID = new(""), new("") + present["chunk_method"] = true + present["parser_id"], present["pipeline_id"] = true, true + } else if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("parser_id", "manual").Error; err != nil { + t.Fatal(err) + } + _, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if err != nil || code != common.CodeSuccess { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + chunker, ok := doc.ParserConfig["ManualChunker:NineInsectsFind"].(map[string]any) + if doc.ParserID != "manual" || !ok || chunker["hierarchy"] != float64(2) { + t.Fatalf("target template config lost: parser=%q config=%#v", doc.ParserID, doc.ParserConfig) + } + if _, ok := doc.ParserConfig["GeneralChunker:SixApplesFall"]; ok { + t.Fatalf("config retained a node from the dataset template: %#v", doc.ParserConfig) + } + }) + } +} + +func TestUpdateDatasetDocumentKeepsDocumentCanvasForConfigPatch(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "report.docx", 0, 0) + const pipelineID = "1234567890abcdef1234567890abcdef" + if err := db.Create(&entity.UserCanvas{ + ID: pipelineID, UserID: "user-1", TenantID: new("tenant-1"), Permission: string(entity.TenantPermissionMe), + DSL: entity.JSONMap{ + "components": map[string]any{ + "Parser:custom": map[string]any{"obj": map[string]any{ + "component_name": "Parser", "params": map[string]any{"docx": map[string]any{}}, + }}, + }, + }, + }).Error; err != nil { + t.Fatal(err) + } + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("pipeline_id", pipelineID).Error; err != nil { + t.Fatal(err) + } + _, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", &UpdateDatasetDocumentRequest{ + ParserConfig: map[string]any{"Parser:custom": map[string]any{"vlm": map[string]any{"llm_id": "vision-model"}}}, + }, map[string]bool{"parser_config": true}) + if err != nil || code != common.CodeSuccess { + t.Fatalf("update: code=%d err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + parser, ok := doc.ParserConfig["Parser:custom"].(map[string]any) + if !ok || !reflect.DeepEqual(parser["vlm"], map[string]any{"llm_id": "vision-model"}) || doc.PipelineID == nil || *doc.PipelineID != pipelineID { + t.Fatalf("document canvas config lost: %#v", doc) + } +} + +func TestUpdateDatasetDocumentRejectsParserErrorsBeforeWrites(t *testing.T) { + for _, failure := range []string{"unknown builtin", "unknown builtin without config", "empty canvas", "malformed canvas", "invalid selection", "knowledge_graph", "tag", "invalid chunk with builtin", "invalid chunk with canvas"} { + t.Run(failure, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", "old.docx", 0, 0) + insertTestFile(t, "file-1", "folder-1", "old.docx", new("old.docx")) + insertTestFile2Document(t, "f2d-1", "file-1", "doc-1") + req := &UpdateDatasetDocumentRequest{ + Name: new("new.docx"), ParseType: new(1), ParserID: new("missing-parser"), + MetaFields: map[string]any{"author": "must-not-save"}, + ParserConfig: map[string]any{"Parser:HipSignsRhyme": map[string]any{"vlm": map[string]any{"llm_id": "must-not-save"}}}, + } + present := map[string]bool{"name": true, "parse_type": true, "parser_id": true, "parser_config": true, "meta_fields": true} + wantError := "unknown builtin parser_id" + switch failure { + case "invalid chunk with builtin", "invalid chunk with canvas": + req.ParserID, req.ChunkMethod = new("general"), new("does-not-exist") + present["chunk_method"] = true + wantError = "unknown builtin parser_id: does-not-exist" + if failure == "invalid chunk with canvas" { + const pipelineID = "1234567890abcdef1234567890abcdef" + insertTestCanvas(t, pipelineID, "user-1") + req.ParseType, req.ParserID, req.PipelineID = new(2), nil, new(pipelineID) + delete(present, "parser_id") + present["pipeline_id"] = true + } + case "knowledge_graph", "tag": + req.ParseType, req.ParserID, req.ChunkMethod = nil, nil, &failure + delete(present, "parse_type") + delete(present, "parser_id") + present["chunk_method"] = true + case "unknown builtin without config": + req.ParserConfig = nil + delete(present, "parser_config") + case "empty canvas", "malformed canvas": + const pipelineID = "1234567890abcdef1234567890abcdef" + dsl := entity.JSONMap{} + wantError = "has no DSL" + if failure == "malformed canvas" { + dsl["graph"] = map[string]any{} + wantError = "DSL has no components" + } + if err := db.Create(&entity.UserCanvas{ + ID: pipelineID, UserID: "user-1", TenantID: new("tenant-1"), Permission: string(entity.TenantPermissionMe), DSL: dsl, + }).Error; err != nil { + t.Fatal(err) + } + req.ParseType, req.ParserID, req.PipelineID = new(2), nil, new(pipelineID) + delete(present, "parser_id") + present["pipeline_id"] = true + case "invalid selection": + req.ParseType = new(2) + wantError = "parser_id must not be set" + } + engine := newMetadataDocEngine(map[string]map[string]any{"doc-1": {"author": "original"}}, map[string]string{"doc-1": "kb-1"}) + svc := testDocumentService(t) + svc.docEngine = engine + svc.metadataSvc = service.NewMetadataServiceForTest(dao.NewKnowledgebaseDAO(), engine) + _, code, err := svc.UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if code != common.CodeDataError || err == nil || !strings.Contains(err.Error(), wantError) { + t.Fatalf("expected %q, got code=%d err=%v", wantError, code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + if doc.Name == nil || *doc.Name != "old.docx" || doc.ParserID != "naive" || (doc.PipelineID != nil && *doc.PipelineID != "") || len(doc.ParserConfig) != 0 { + t.Fatalf("rejected request changed document: %#v", doc) + } + file, err := dao.NewFileDAO().GetByID(t.Context(), db, "file-1") + if err != nil || file.Name != "old.docx" { + t.Fatalf("rejected request changed file: file=%#v err=%v", file, err) + } + if !reflect.DeepEqual(engine.records["doc-1"], map[string]any{"author": "original"}) { + t.Fatalf("rejected request changed metadata: %#v", engine.records) } }) } diff --git a/internal/service/document/document_parser_selection.go b/internal/service/document/document_parser_selection.go index 95de880490..ee4786f543 100644 --- a/internal/service/document/document_parser_selection.go +++ b/internal/service/document/document_parser_selection.go @@ -35,8 +35,7 @@ func dedicatedDocumentParserID(filetype utility.FileType, filename string) strin } // The presentation and email extensions all classify as FileTypeDOC, so the - // name decides (same name-based rule as Python's get_parser and the - // document parser_id guard in document_dataset_update.go). + // name decides which dedicated pipeline owns them. name := strings.ToLower(filename) switch { case strings.HasSuffix(name, ".ppt"), @@ -189,7 +188,12 @@ func rebuildParserConfig(ctx context.Context, kb *entity.Knowledgebase, target, return nil, false } raw := map[string]interface{}(base) - cleaned := pipelinepkg.BuildParserConfig(dslJSON, raw) + cleaned, err := pipelinepkg.BuildParserConfig(dslJSON, raw) + if err != nil { + common.Warn("document: invalid target pipeline DSL", + zap.String("parser_id", target), zap.Error(err)) + return nil, false + } // The dataset-level metadata group is parser-independent; keep it so the // target pipeline's Extractor node inherits the dataset setting instead of // falling back to disabled. diff --git a/internal/service/document/document_parser_selection_test.go b/internal/service/document/document_parser_selection_test.go index 0d53875ba2..ac8ca8d103 100644 --- a/internal/service/document/document_parser_selection_test.go +++ b/internal/service/document/document_parser_selection_test.go @@ -350,7 +350,7 @@ func TestResolveDocumentParser_Priority(t *testing.T) { wantFamilyIn: "text&code", }, { - name: "an unmappable extension still reaches its owner pipeline", + name: "pages reaches its owner pipeline", kbParser: "general", kbConfig: generalParserConfig(), filename: "deck.pages", diff --git a/internal/service/document/document_test.go b/internal/service/document/document_test.go index e44968be02..9cac648586 100644 --- a/internal/service/document/document_test.go +++ b/internal/service/document/document_test.go @@ -688,11 +688,20 @@ func ensureTestTenantOwner(t *testing.T, userID, tenantID string) { func insertTestCanvas(t *testing.T, canvasID, ownerID string) { t.Helper() tenantID := "tenant-1" + dslJSON, err := service.LoadPipelineDSL(t.Context(), false, "general", nil) + if err != nil { + t.Fatal(err) + } + var dsl entity.JSONMap + if err := json.Unmarshal(dslJSON, &dsl); err != nil { + t.Fatal(err) + } if err := dao.DB.Create(&entity.UserCanvas{ ID: canvasID, UserID: ownerID, TenantID: &tenantID, Permission: string(entity.TenantPermissionMe), + DSL: dsl, }).Error; err != nil { t.Fatalf("insert test canvas: %v", err) } @@ -3025,31 +3034,71 @@ func float64Ptr(value float64) *float64 { return &value } func int64Ptr(value int64) *int64 { return &value } -func TestUpdateDatasetDocumentRejectsUnsupportedParserIDForVisualDoc(t *testing.T) { - db := setupServiceTestDB(t) - pushServiceDB(t, db) - insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) - insertNamedTestDoc(t, "doc-1", "kb-1", "image.png", 0, 0) - if err := dao.DB.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("type", "visual").Error; err != nil { - t.Fatalf("update doc type: %v", err) - } +func TestUpdateDatasetDocumentSavesVisionModelAcrossFileTypes(t *testing.T) { + for _, file := range []struct { + name string + docType string + parserID string + }{ + {"image.png", "visual", "general"}, + {"deck.pptx", "ppt", "presentation"}, + {"deck.pages", "doc", "presentation"}, + {"DECK.DPS", "ppt", "presentation"}, + {"deck.dpt", "ppt", "presentation"}, + {"report.docx", "doc", "manual"}, + } { + for _, selection := range []string{"parser_id", "chunk_method"} { + t.Run(file.name+"/"+selection, func(t *testing.T) { + db := setupServiceTestDB(t) + pushServiceDB(t, db) + insertTestKB(t, "kb-1", "tenant-1", 1, 0, 0) + insertNamedTestDoc(t, "doc-1", "kb-1", file.name, 0, 0) + if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("type", file.docType).Error; err != nil { + t.Fatalf("update doc type: %v", err) + } - parserID := "naive" - parseType := 1 - svc := testDocumentService(t) - ctx := documentTestContext(t.Context()) - _, code, err := svc.UpdateDatasetDocument(ctx, "user-1", "kb-1", "doc-1", &UpdateDatasetDocumentRequest{ - ParserID: &parserID, - ParseType: &parseType, - }, map[string]bool{"parser_id": true, "parse_type": true}) - if err == nil { - t.Fatal("expected parser_id visual error") - } - if code != common.CodeDataError { - t.Fatalf("code = %v, want %v", code, common.CodeDataError) - } - if err.Error() != "not supported yet" { - t.Fatalf("err = %q", err.Error()) + req := &UpdateDatasetDocumentRequest{ + ParserConfig: map[string]any{ + "Parser:HipSignsRhyme": map[string]any{ + "enable_vision_enhancement": true, + "vlm": map[string]any{"llm_id": "b17886e8ff3f4000bcf46f5b58c76803"}, + }, + }, + } + present := map[string]bool{"parser_config": true, selection: true} + wantParser := file.parserID + if selection == "parser_id" { + req.ParseType = new(1) + req.ParserID = &file.parserID + req.PipelineID = new("") + present["parse_type"] = true + present["pipeline_id"] = true + } else { + req.ChunkMethod = &file.parserID + } + resp, code, err := testDocumentService(t).UpdateDatasetDocument(documentTestContext(t.Context()), "user-1", "kb-1", "doc-1", req, present) + if err != nil || code != common.CodeSuccess { + t.Fatalf("save vision model: code=%v err=%v", code, err) + } + doc, err := dao.NewDocumentDAO().GetByID(t.Context(), db, "doc-1") + if err != nil { + t.Fatal(err) + } + if resp.ParserID != wantParser || doc.ParserID != wantParser { + t.Fatalf("parser selection: response=%q stored=%q, want %q", resp.ParserID, doc.ParserID, wantParser) + } + for _, config := range []map[string]any{resp.ParserConfig, doc.ParserConfig} { + parser, ok := config["Parser:HipSignsRhyme"].(map[string]any) + if !ok || parser["enable_vision_enhancement"] != true { + t.Fatalf("vision enhancement not saved: %#v", config) + } + vlm, ok := parser["vlm"].(map[string]any) + if !ok || vlm["llm_id"] != "b17886e8ff3f4000bcf46f5b58c76803" { + t.Fatalf("vision model not saved: %#v", parser) + } + } + }) + } } } @@ -3143,7 +3192,7 @@ func TestUpdateDatasetDocumentParserConfigPreservesParseResults(t *testing.T) { db := setupServiceTestDB(t) pushServiceDB(t, db) insertTestKB(t, "kb-1", "tenant-1", 1, 10, 5) - insertNamedTestDoc(t, "doc-1", "kb-1", "doc.txt", 10, 5) + insertNamedTestDoc(t, "doc-1", "kb-1", "doc.docx", 10, 5) insertTestCanvas(t, pipelineID, "user-1") insertTestIngestionTaskWithStatus(t, "task-1", "user-1", "doc-1", "kb-1", common.COMPLETED) if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("process_duration", 42).Error; err != nil { @@ -4091,7 +4140,7 @@ func TestUpdateDatasetDocumentParseTypeBuiltin(t *testing.T) { db := setupServiceTestDB(t) pushServiceDB(t, db) insertTestKB(t, "kb-1", "tenant-1", 1, 10, 5) - insertNamedTestDoc(t, "doc-1", "kb-1", "doc.txt", 10, 5) + insertNamedTestDoc(t, "doc-1", "kb-1", "doc.docx", 10, 5) // Seed a prior canvas-backed document so the builtin transition must // actively clear the persisted pipeline_id. Without this, a regression in @@ -4189,19 +4238,11 @@ func TestUpdateDatasetDocumentParseTypePipelineRejectsParserID(t *testing.T) { } } -func TestUpdateDatasetDocumentParentChildConfigSurvivesDSLFailure(t *testing.T) { +func TestUpdateDatasetDocumentParentChildConfigReachesCanvasChunker(t *testing.T) { db := setupServiceTestDB(t) pushServiceDB(t, db) insertTestKB(t, "kb-1", "tenant-1", 1, 10, 5) insertNamedTestDoc(t, "doc-1", "kb-1", "doc.txt", 10, 5) - if err := db.Model(&entity.Document{}).Where("id = ?", "doc-1").Update("parser_config", entity.JSONMap{ - "GeneralChunker:SixApplesFall": map[string]any{ - "children_delimiters": []any{}, - }, - }).Error; err != nil { - t.Fatalf("seed document parser config: %v", err) - } - parseType := 2 pipelineID := "1234567890abcdef1234567890abcdef" insertTestCanvas(t, pipelineID, "user-1") diff --git a/test/api/document_api_test.go b/test/api/document_api_test.go index 0b03443ed2..6f9bf72284 100644 --- a/test/api/document_api_test.go +++ b/test/api/document_api_test.go @@ -290,6 +290,8 @@ func TestRESTDocumentUpdatesPersistAndRejectAtomically(t *testing.T) { {"nested_meta", apiObject{"meta_fields": apiObject{"nested": apiObject{"x": 1}}}, 102}, {"nested_meta_list", apiObject{"meta_fields": apiObject{"nested": []any{apiObject{"x": 1}}}}, 102}, {"invalid_chunk_method", apiObject{"chunk_method": "unknown"}, 102}, {"empty_chunk_method", apiObject{"chunk_method": ""}, 102}, + {"removed_knowledge_graph", apiObject{"name": "must_not_persist.txt", "chunk_method": "knowledge_graph"}, 102}, + {"removed_tag", apiObject{"name": "must_not_persist.txt", "chunk_method": "tag"}, 102}, {"parser_not_object", apiObject{"parser_config": []any{}}, 102}, } { t.Run(tc.name, func(t *testing.T) { @@ -303,7 +305,29 @@ func TestRESTDocumentUpdatesPersistAndRejectAtomically(t *testing.T) { } }) } - for _, method := range []string{"naive", "manual", "qa", "table", "paper", "book", "laws", "presentation", "picture", "one", "knowledge_graph", "email", "tag"} { + catalog := object(t, restOK(t, restCall(t, TestConfig, http.MethodGet, "/pipelines", nil, apiObject{"type": "builtin"}))["data"]) + pipelines := array(t, catalog["canvas"]) + if len(pipelines) == 0 { + t.Fatal("builtin pipeline catalog is empty") + } + methods := []string{"naive"} + for _, pipeline := range pipelines { + methods = append(methods, object(t, pipeline)["id"].(string)) + } + for _, method := range methods { + // This document is text: registry membership alone does not mean a + // template supports its file family. + if !slices.Contains([]string{"naive", "general", "book", "laws", "one", "qa", "table"}, method) { + before := getRESTDocument(t, id, docID) + restCode(t, restCall(t, TestConfig, http.MethodPatch, path, apiObject{"chunk_method": method, "name": "must_not_persist.txt"}, nil), 200, 102) + after := getRESTDocument(t, id, docID) + for _, key := range []string{"name", "parser_id", "pipeline_id", "parser_config"} { + if !reflect.DeepEqual(before[key], after[key]) { + t.Fatalf("unsupported template %s changed %s: before=%v after=%v", method, key, before[key], after[key]) + } + } + continue + } restOK(t, restCall(t, TestConfig, http.MethodPatch, path, apiObject{"chunk_method": method}, nil)) persisted := getRESTDocument(t, id, docID) if persisted["chunk_method"] != method || persisted["parser_id"] != method { @@ -375,6 +399,7 @@ func TestRESTDocumentParserConfigurationAndMetadataConfig(t *testing.T) { } } // Flat Python-era options are discarded by the component-scoped Go contract. + beforeFlatConfig := persisted["parser_config"] restOK(t, restCall(t, TestConfig, http.MethodPatch, path, apiObject{"parser_config": apiObject{"chunk_token_num": -1, "raptor": apiObject{"use_raptor": true}}}, nil)) persisted = getRESTDocument(t, id, docID) pc := object(t, persisted["parser_config"]) @@ -383,8 +408,8 @@ func TestRESTDocumentParserConfigurationAndMetadataConfig(t *testing.T) { t.Fatalf("flat parser key retained: %s", key) } } - if object(t, pc[component])["chunk_token_size"] != object(t, config[component])["chunk_token_size"] { - t.Fatalf("flat options did not restore component defaults: got=%v defaults=%v", pc[component], config[component]) + if !reflect.DeepEqual(pc, beforeFlatConfig) { + t.Fatalf("ignored flat options changed existing config: got=%v want=%v", pc, beforeFlatConfig) } metadata := apiObject{"author": "alice", "tags": []any{"one", "two"}} metadataPath := path + "/metadata/config"