fix(knowledge): copy embedded chunk images into exports namespace

Cloned images were unreachable via GET /knowledge-bases/:id/files:
copyOwnedObject used CopyFile, which writes to the knowledge-scoped upload
layout ({tenant}/{knowledgeID}/...), but the KB proxy only serves objects
under {tenant}/exports/ (ValidateKBScopedStoragePath). Read the source
bytes and re-save via SaveBytes instead, mirroring how images are
persisted during ingestion, so copies land in the servable exports
namespace and remain independent of the source object.
This commit is contained in:
wizardchen
2026-07-22 15:27:11 +08:00
committed by lyingbug
parent 0f422f172d
commit 554d273cf5
2 changed files with 85 additions and 23 deletions
@@ -3,22 +3,24 @@ package service
import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"mime/multipart"
"strings"
"testing"
"github.com/Tencent/WeKnora/internal/types"
)
// countingFileService is a minimal FileService stub for cloneChunkImageInfo tests.
// CopyFile records each invocation and returns a deterministic destination path
// derived from (knowledgeID, srcPath) so dedup and rewrite behaviour are verifiable.
// copyOwnedObject copies by GetFile-then-SaveBytes (so extracted images land in
// the servable exports/ namespace), so this stub streams the source path back as
// the file bytes from GetFile and reconstructs a deterministic exports-style
// destination path in SaveBytes, making dedup and rewrite behaviour verifiable.
type countingFileService struct {
copyCalls int
copiedFrom []string
failOnURL string // when non-empty, CopyFile returns an error for this srcPath
failOnURL string // when non-empty, GetFile returns an error for this srcPath
deleteCalls int
}
@@ -28,12 +30,22 @@ func (c *countingFileService) SaveFile(ctx context.Context, file *multipart.File
return "", nil
}
// SaveBytes records each copy and returns a deterministic exports-namespace path
// derived from the streamed source bytes (which GetFile set to the source path).
func (c *countingFileService) SaveBytes(ctx context.Context, data []byte, tenantID uint64, fileName string, temp bool) (string, error) {
return "", nil
src := string(data)
c.copyCalls++
c.copiedFrom = append(c.copiedFrom, src)
return fmt.Sprintf("local://%d/exports/copy-of-%s", tenantID, src), nil
}
// GetFile streams the requested path back as the file content so SaveBytes can
// build a deterministic destination. failOnURL simulates an unreadable source.
func (c *countingFileService) GetFile(ctx context.Context, filePath string) (io.ReadCloser, error) {
return nil, errors.New("not implemented")
if c.failOnURL != "" && filePath == c.failOnURL {
return nil, fmt.Errorf("simulated read failure for %s", filePath)
}
return io.NopCloser(strings.NewReader(filePath)), nil
}
func (c *countingFileService) GetFileURL(ctx context.Context, filePath string) (string, error) {
@@ -46,12 +58,7 @@ func (c *countingFileService) DeleteFile(ctx context.Context, filePath string) e
}
func (c *countingFileService) CopyFile(ctx context.Context, srcPath string, tenantID uint64, knowledgeID string) (string, error) {
if c.failOnURL != "" && srcPath == c.failOnURL {
return "", fmt.Errorf("simulated copy failure for %s", srcPath)
}
c.copyCalls++
c.copiedFrom = append(c.copiedFrom, srcPath)
return fmt.Sprintf("local://%d/%s/copy-of-%s", tenantID, knowledgeID, srcPath), nil
return "", fmt.Errorf("CopyFile must not be used for embedded image copies")
}
func mustImageInfoJSON(t *testing.T, imgs []types.ImageInfo) string {
@@ -93,7 +100,7 @@ func TestCloneChunkImageInfo_RewritesURLAndMatchedOriginal(t *testing.T) {
if err := json.Unmarshal([]byte(out), &got); err != nil {
t.Fatalf("unmarshal out: %v", err)
}
want := "local://7/k-dst/copy-of-local://1/k0/a.png"
want := "local://7/exports/copy-of-local://1/k0/a.png"
if got[0].URL != want {
t.Errorf("URL not rewritten: got %q want %q", got[0].URL, want)
}
@@ -125,7 +132,7 @@ func TestRewriteContentImageURLs_ParentTextChunk(t *testing.T) {
// Parent text chunk has NO image_info but embeds the markdown reference.
parentContent := "See ![diagram](local://1/k0/a.png) here."
got := rewriteContentImageURLs(parentContent, urlCache)
want := "See ![diagram](local://7/k-dst/copy-of-local://1/k0/a.png) here."
want := "See ![diagram](local://7/exports/copy-of-local://1/k0/a.png) here."
if got != want {
t.Errorf("parent content image URL not rewritten:\n got %q\nwant %q", got, want)
}
@@ -5,6 +5,9 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"path/filepath"
"slices"
"strings"
"time"
@@ -21,13 +24,18 @@ import (
"golang.org/x/sync/errgroup"
)
// copyOwnedObject performs a real copy of srcPath into a NEW object owned by
// (tenantID, knowledgeID) using the destination FileService, returning the new
// provider:// path. The same-backend check lives inside dstSvc.CopyFile, which
// returns file.ErrCrossBackendCopy when srcPath belongs to a different provider;
// that error is propagated unchanged so callers can fail the clone explicitly.
// srcSvc is accepted for symmetry with the read side but is not used directly:
// server-side copies are issued by the destination service.
// copyOwnedObject copies srcPath into a NEW object owned by the destination
// tenant, returning the new provider:// (resource) path.
//
// Extracted/embedded chunk images MUST land in the tenant's exports/ namespace,
// because GET /knowledge-bases/:id/files only serves objects that pass
// ValidateKBScopedStoragePath (i.e. {tenant}/exports/...). CopyFile writes to
// the knowledge-scoped upload layout ({tenant}/{knowledgeID}/...) used for raw
// source files, which the KB proxy rejects — so a clone that used CopyFile
// produced images that could no longer be rendered. Instead, read the source
// bytes and re-save them via SaveBytes, exactly mirroring how the original
// images were persisted during ingestion (see image_resolver.saveReferencedImage),
// so the copy is a genuine independent object in the servable namespace.
func copyOwnedObject(
ctx context.Context,
srcSvc, dstSvc interfaces.FileService,
@@ -35,8 +43,55 @@ func copyOwnedObject(
tenantID uint64,
knowledgeID string,
) (string, error) {
_ = srcSvc // reserved for future cross-backend streaming fallback
return dstSvc.CopyFile(ctx, srcPath, tenantID, knowledgeID)
_ = knowledgeID // exports objects are tenant-scoped, not knowledge-scoped
rc, err := srcSvc.GetFile(ctx, srcPath)
if err != nil {
return "", fmt.Errorf("read source image %q: %w", srcPath, err)
}
defer rc.Close()
data, err := io.ReadAll(rc)
if err != nil {
return "", fmt.Errorf("buffer source image %q: %w", srcPath, err)
}
fileName := uuid.New().String() + imageExtForCopy(srcPath, data)
newPath, err := dstSvc.SaveBytes(ctx, data, tenantID, fileName, false)
if err != nil {
return "", fmt.Errorf("save copied image for %q: %w", srcPath, err)
}
return newPath, nil
}
// imageExtForCopy resolves the file extension to use for a copied image. It
// prefers an image extension already present on the source path, then falls
// back to sniffing the content bytes, and finally defaults to ".png" (matching
// image_resolver's default) so the object is always served with a sane type.
func imageExtForCopy(srcPath string, data []byte) string {
if ext := strings.ToLower(filepath.Ext(srcPath)); isImageExt(ext) {
return ext
}
switch http.DetectContentType(data) {
case "image/png":
return ".png"
case "image/jpeg":
return ".jpg"
case "image/gif":
return ".gif"
case "image/webp":
return ".webp"
case "image/bmp":
return ".bmp"
}
return ".png"
}
func isImageExt(ext string) bool {
switch ext {
case ".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp", ".svg":
return true
default:
return false
}
}
// cloneChunkImageInfo parses a chunk's image_info JSON, copies every referenced