fix: make metadata key collisions and skipped documents visible during document loading (#5542)

## Issue

Related to #5541. **This PR intentionally does not close #5541** — the
Kotlin `parseAsync` divergence stays open and is addressed in 2.0 (see
"Follow-up" below).

  ## Change

`DocumentLoader.load()` forwards the `DocumentSource` metadata onto the
parsed `Document` with `putAll()`. When the parser and the source define
the same key (e.g. a custom `DocumentParser` that sets
`file_name`, or `ApachePdfBoxDocumentParser` /
`ApacheTikaDocumentParser` with `includeMetadata = true`, which copy
keys straight out of the file), the document value is dropped and
nothing is reported.

This PR keeps the existing "source wins" resolution — no behaviour
change, no breaking change — but stops it being silent, and stops batch
loaders losing documents invisibly:

  **`langchain4j-core`**
- `DocumentLoader.load()` logs a warning naming the conflicting key
**and both values**, and points at the workaround:
    ```
Metadata key "file_name" is set both by the document ("report.pdf") and
by the source ("2024-report.pdf").
Keeping the source value and discarding the document value. To control
this, remove the key in your
    DocumentParser before returning the Document.
    ```
- Javadoc on `DocumentLoader.load()` now states the collision behaviour
explicitly instead of just "forwards the source Metadata".
- `Metadata.merge()` reports the conflicting **values**, not only the
key names:
`Metadata keys are not unique. Common keys and their values:
{key2=("value2", "value3")}`

**Batch loaders** (`FileSystemDocumentLoader`,
`ClassPathDocumentLoader`, `AmazonS3DocumentLoader`,
`AzureBlobStorageDocumentLoader`, `GoogleCloudStorageDocumentLoader`,
`TencentCosDocumentLoader`,
`GitHubDocumentLoader`)

All seven swallow per-document exceptions and continue, so a caller can
silently receive a shorter `List<Document>` than expected. Each now logs
a summary when anything was lost:
  ```
Loaded 9997 of 10000 documents from '/docs'. Skipped 3 that failed to
load and 0 that were blank.
  ```
`FileSystemDocumentLoader` and `ClassPathDocumentLoader` additionally
pass the throwable to the logger instead of only its message, so
failures get a stack trace like the cloud loaders already did.

  ## Why not throw on collisions

An earlier revision of this PR made both `DocumentLoader.load()` and the
Kotlin `parseAsync` throw on collisions via `Metadata.merge()`. That
turns out to be the wrong fix for 1.x: every batch loader treats an
exception as "this file is broken, skip it". A perfectly readable
document would then be dropped from the result list purely because of a
metadata key name — replacing a silently discarded *value* with a
silently discarded *document*. The trigger is user data (keys authored
by whoever produced the PDF), not user code, so it is not something the
caller can reliably avoid.

A workaround exists today and needs no new API — decorate the parser to
drop the key:

  ```java
  DocumentParser stripped = inputStream -> {
      Document document = myParser.parse(inputStream);
      document.metadata().remove(Document.FILE_NAME);
      return document;
  };
  ```

  ## Follow-up (2.0)

Collisions should not be resolved silently, but the fix needs two pieces
that only fit in a major release, and they have to land together:

- **Collisions fail by default**, with an explicit resolution strategy
to opt out (`sourceWins()`, `documentWins()`, or a custom resolver), so
the library never guesses which value was meant.
- **Bulk loading gets a real failure policy**: fail-fast by default,
explicit opt-in to tolerance, and failures returned as **data** rather
than only logged — so "throw" can never mean "document silently
  vanishes".

Until then, the Kotlin `parseAsync` extension keeps throwing where the
Java path warns. That divergence is deliberate, and is why #5541 stays
open — better one behaviour change in 2.0 than two in a row.

  ## Notes for reviewers

  - No API changes and no new dependencies; `revapi` is unaffected.
- The only `langchain4j-kotlin` change is a test assertion updated for
the new `Metadata.merge()` message. The Kotlin `parseAsync` main source
is back to `main`.
- `Metadata.merge()`'s exception **message** changed. The exception type
and the conditions that trigger it are unchanged; only assertions on the
exact message text are affected.
- `GoogleCloudStorageDocumentLoader` shows a larger diff than its change
warrants: its imports were already unformatted on `main` and only hit
the spotless ratchet now that the file is touched.

  ## General checklist
  - [X] There are no breaking changes (API, behaviour)
  - [X] I have added unit and/or integration tests for my change
  - [X] The tests cover both positive and negative cases
- [X] I have manually run all the unit and integration tests in the
module I have added/changed, and they are all green
- [X] I have manually run all the unit and integration tests in the
[core](https://github.com/langchain4j/langchain4j/tree/main/langchain4j-core)
and

[main](https://github.com/langchain4j/langchain4j/tree/main/langchain4j)
modules, and they are all green
- [ ] I have added/updated the
[documentation](https://github.com/langchain4j/langchain4j/tree/main/docs/docs)
<!-- N/A — behaviour is documented in the Javadoc of
DocumentLoader.load() -->
- [ ] I have added an example in the [examples
repo](https://github.com/langchain4j/langchain4j-examples) (only for
"big" features) <!-- N/A -->
- [ ] I have added/updated [Spring Boot
starter(s)](https://github.com/langchain4j/langchain4j-spring) (if
applicable) <!-- N/A -->

---------

Co-authored-by: Dmytro Liubarskyi <ljubarskij@gmail.com>
This commit is contained in:
Eunbin Son 2026-07-31 18:06:19 +09:00 committed by GitHub
parent 319246b468
commit f11c7a78d2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 171 additions and 27 deletions

View File

@ -96,16 +96,27 @@ public class AmazonS3DocumentLoader {
.filter(s3Object -> !s3Object.key().endsWith("/") && s3Object.size() > 0)
.collect(toList());
int failed = 0;
for (S3Object s3Object : filteredS3Objects) {
String key = s3Object.key();
try {
Document document = loadDocument(bucket, key, parser);
documents.add(document);
} catch (Exception e) {
failed++;
log.warn("Failed to load an object with key '{}' from bucket '{}', skipping it.", key, bucket, e);
}
}
if (failed > 0) {
log.warn(
"Loaded {} of {} documents from bucket '{}'. Skipped {} that failed to load.",
documents.size(),
documents.size() + failed,
bucket,
failed);
}
return documents;
}

View File

@ -13,6 +13,7 @@ import dev.langchain4j.data.document.DocumentParser;
import dev.langchain4j.data.document.source.azure.storage.blob.AzureBlobStorageSource;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@ -59,6 +60,7 @@ public class AzureBlobStorageDocumentLoader {
*/
public List<Document> loadDocuments(String containerName, String prefix, DocumentParser parser) {
List<Document> documents = new ArrayList<>();
AtomicInteger failed = new AtomicInteger();
ListBlobsOptions options = new ListBlobsOptions().setPrefix(prefix);
@ -69,6 +71,7 @@ public class AzureBlobStorageDocumentLoader {
try {
documents.add(loadDocument(containerName, blob.getName(), parser));
} catch (Exception e) {
failed.incrementAndGet();
log.warn(
"Failed to load blob '{}' from container '{}', skipping it.",
blob.getName(),
@ -77,6 +80,15 @@ public class AzureBlobStorageDocumentLoader {
}
});
if (failed.get() > 0) {
log.warn(
"Loaded {} of {} documents from container '{}'. Skipped {} that failed to load.",
documents.size(),
documents.size() + failed.get(),
containerName,
failed.get());
}
return documents;
}
}

View File

@ -10,6 +10,7 @@ import dev.langchain4j.data.document.source.github.GitHubSource;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import org.kohsuke.github.GHContent;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
@ -161,14 +162,24 @@ public class GitHubDocumentLoader {
ensureNotBlank(owner, "owner");
ensureNotBlank(repo, "repo");
List<Document> documents = new ArrayList<>();
AtomicInteger failed = new AtomicInteger();
try {
gitHub.getRepository(owner + "/" + repo)
.getDirectoryContent(path, branch)
.forEach(ghDirectoryContent ->
GitHubDocumentLoader.scanDirectory(ghDirectoryContent, documents, parser));
GitHubDocumentLoader.scanDirectory(ghDirectoryContent, documents, failed, parser));
} catch (IOException ioException) {
throw new RuntimeException(ioException);
}
if (failed.get() > 0) {
logger.warn(
"Loaded {} of {} documents from '{}/{}'. Skipped {} that failed to load.",
documents.size(),
documents.size() + failed.get(),
owner,
repo,
failed.get());
}
return documents;
}
@ -176,13 +187,14 @@ public class GitHubDocumentLoader {
return loadDocuments(owner, repo, branch, "", parser);
}
private static void scanDirectory(GHContent ghContent, List<Document> documents, DocumentParser parser) {
private static void scanDirectory(
GHContent ghContent, List<Document> documents, AtomicInteger failed, DocumentParser parser) {
if (ghContent.isDirectory()) {
try {
ghContent
.listDirectoryContent()
.forEach(ghDirectoryContent ->
GitHubDocumentLoader.scanDirectory(ghDirectoryContent, documents, parser));
GitHubDocumentLoader.scanDirectory(ghDirectoryContent, documents, failed, parser));
} catch (IOException ioException) {
logger.error("Failed to read directory from GitHub: {}", ghContent.getHtmlUrl(), ioException);
}
@ -191,6 +203,7 @@ public class GitHubDocumentLoader {
try {
document = withRetry(() -> fromGitHub(parser, ghContent), 2);
} catch (RuntimeException runtimeException) {
failed.incrementAndGet();
logger.error("Failed to read document from GitHub: {}", ghContent.getHtmlUrl(), runtimeException);
}
if (document != null) {

View File

@ -1,23 +1,21 @@
package dev.langchain4j.data.document.loader.gcs;
import static dev.langchain4j.internal.ValidationUtils.ensureNotBlank;
import static dev.langchain4j.internal.ValidationUtils.ensureNotNull;
import com.google.api.gax.paging.Page;
import com.google.auth.Credentials;
import com.google.cloud.storage.Blob;
import com.google.cloud.storage.Storage;
import com.google.cloud.storage.StorageOptions;
import com.google.api.gax.paging.Page;
import dev.langchain4j.data.document.Document;
import dev.langchain4j.data.document.DocumentLoader;
import dev.langchain4j.data.document.DocumentParser;
import dev.langchain4j.data.document.source.gcs.GcsSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.List;
import static dev.langchain4j.internal.ValidationUtils.ensureNotBlank;
import static dev.langchain4j.internal.ValidationUtils.ensureNotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Google Cloud Storage Document Loader to load documents from Google Cloud Storage buckets.
@ -70,21 +68,35 @@ public class GoogleCloudStorageDocumentLoader {
* @return A list of documents from the bucket that match the glob pattern.
*/
public List<Document> loadDocuments(String bucket, String globPattern, DocumentParser parser) {
Page<Blob> blobs = globPattern != null ?
storage.list(bucket, Storage.BlobListOption.currentDirectory(), Storage.BlobListOption.matchGlob(globPattern)) :
storage.list(bucket, Storage.BlobListOption.currentDirectory());
Page<Blob> blobs = globPattern != null
? storage.list(
bucket,
Storage.BlobListOption.currentDirectory(),
Storage.BlobListOption.matchGlob(globPattern))
: storage.list(bucket, Storage.BlobListOption.currentDirectory());
List<Document> documents = new ArrayList<>();
int failed = 0;
for (Blob blob : blobs.iterateAll()) {
try {
GcsSource gcsSource = new GcsSource(blob);
documents.add(DocumentLoader.load(gcsSource, ensureNotNull(parser, "parser")));
} catch (Exception e) {
failed++;
log.warn("Failed to load blob '{}' from bucket '{}', skipping it.", blob.getName(), bucket, e);
}
}
if (failed > 0) {
log.warn(
"Loaded {} of {} documents from bucket '{}'. Skipped {} that failed to load.",
documents.size(),
documents.size() + failed,
bucket,
failed);
}
return documents;
}

View File

@ -74,6 +74,7 @@ public class TencentCosDocumentLoader {
ObjectListing objectListing = cosClient.listObjects(listObjectsRequest);
int failed = 0;
while (true) {
for (COSObjectSummary object : objectListing.getObjectSummaries()) {
if (object.getKey().endsWith("/") || object.getSize() == 0) {
@ -84,6 +85,7 @@ public class TencentCosDocumentLoader {
Document document = loadDocument(bucket, key, parser);
documents.add(document);
} catch (Exception e) {
failed++;
log.warn("Failed to load an object with key '{}' from bucket '{}', skipping it.", key, bucket, e);
}
}
@ -93,6 +95,15 @@ public class TencentCosDocumentLoader {
objectListing = cosClient.listNextBatchOfObjects(objectListing);
}
if (failed > 0) {
log.warn(
"Loaded {} of {} documents from bucket '{}'. Skipped {} that failed to load.",
documents.size(),
documents.size() + failed,
bucket,
failed);
}
return documents;
}

View File

@ -1,19 +1,26 @@
package dev.langchain4j.data.document;
import java.io.InputStream;
import java.util.Map;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Utility class for loading documents.
*/
public class DocumentLoader {
private DocumentLoader() {
}
private static final Logger log = LoggerFactory.getLogger(DocumentLoader.class);
private DocumentLoader() {}
/**
* Loads a document from the given source using the given parser.
*
* <p>Forwards the source Metadata to the parsed Document.
* <p>Forwards the source Metadata to the parsed Document. If both define the same metadata key,
* the source value is kept, the document value is discarded, and a warning is logged.
* To control which value is kept, remove the key in your {@link DocumentParser}
* before returning the {@link Document}.
*
* @param source The source from which the document will be loaded.
* @param parser The parser that will be used to parse the document.
@ -23,7 +30,9 @@ public class DocumentLoader {
public static Document load(DocumentSource source, DocumentParser parser) {
try (InputStream inputStream = source.inputStream()) {
Document document = parser.parse(inputStream);
document.metadata().putAll(source.metadata().toMap());
Map<String, Object> sourceMetadata = source.metadata().toMap();
warnAboutDiscardedValues(document.metadata(), sourceMetadata);
document.metadata().putAll(sourceMetadata);
return document;
} catch (BlankDocumentException e) {
throw e;
@ -31,4 +40,19 @@ public class DocumentLoader {
throw new RuntimeException("Failed to load document", e);
}
}
private static void warnAboutDiscardedValues(Metadata documentMetadata, Map<String, Object> sourceMetadata) {
sourceMetadata.forEach((key, sourceValue) -> {
if (documentMetadata.containsKey(key)) {
log.warn(
"Metadata key \"{}\" is set both by the document (\"{}\") and by the source (\"{}\"). "
+ "Keeping the source value and discarding the document value. "
+ "To control this, remove the key in your DocumentParser "
+ "before returning the Document.",
key,
documentMetadata.toMap().get(key),
sourceValue);
}
});
}
}

View File

@ -4,6 +4,7 @@ import static dev.langchain4j.internal.Exceptions.illegalArgument;
import static dev.langchain4j.internal.Exceptions.runtime;
import static dev.langchain4j.internal.ValidationUtils.ensureNotBlank;
import static dev.langchain4j.internal.ValidationUtils.ensureNotNull;
import static java.util.stream.Collectors.joining;
import dev.langchain4j.data.segment.TextSegment;
import dev.langchain4j.store.embedding.EmbeddingStore;
@ -471,7 +472,11 @@ public class Metadata {
final var commonKeys = new HashSet<>(thisMap.keySet());
commonKeys.retainAll(anotherMap.keySet());
if (!commonKeys.isEmpty()) {
throw illegalArgument("Metadata keys are not unique. Common keys: %s", commonKeys);
final var conflicts = commonKeys.stream()
.sorted()
.map(key -> "%s=(\"%s\", \"%s\")".formatted(key, thisMap.get(key), anotherMap.get(key)))
.collect(joining(", ", "{", "}"));
throw illegalArgument("Metadata keys are not unique. Common keys and their values: %s", conflicts);
}
final var mergedMap = new HashMap<>(thisMap);
mergedMap.putAll(anotherMap);

View File

@ -9,6 +9,10 @@ import org.assertj.core.api.WithAssertions;
import org.junit.jupiter.api.Test;
class DocumentLoaderTest implements WithAssertions {
private static final DocumentParser COLLIDING_PARSER = inputStream ->
Document.from("Hello, world!", new Metadata().put("foo", "baz").put("title", "Bar"));
public static final class StringSource implements DocumentSource {
private final String content;
private final Metadata metadata;
@ -97,4 +101,26 @@ class DocumentLoaderTest implements WithAssertions {
}))
.withMessageContaining("Failed to load document");
}
@Test
void load_sourceWinsWhenDocumentAndSourceMetadataShareKeys() {
StringSource source = new StringSource("Hello, world!", new Metadata().put("foo", "bar"));
Document document = DocumentLoader.load(source, COLLIDING_PARSER);
assertThat(document.metadata().toMap()).containsOnly(entry("foo", "bar"), entry("title", "Bar"));
}
@Test
void load_wrapsIllegalArgumentExceptionThrownByParser() {
StringSource source = new StringSource("Hello, world!", new Metadata().put("foo", "bar"));
DocumentParser failingParser = inputStream -> {
throw new IllegalArgumentException("Failed to parse document");
};
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> DocumentLoader.load(source, failingParser))
.withMessageContaining("Failed to load document")
.withCauseInstanceOf(IllegalArgumentException.class);
}
}

View File

@ -86,6 +86,7 @@ internal class MergeMetadataTest {
metadata1.merge(metadata2)
}
exception.message shouldBe "Metadata keys are not unique. Common keys: [key2]"
exception.message shouldBe
"Metadata keys are not unique. Common keys and their values: {key2=(\"value2\", \"value3\")}"
}
}

View File

@ -21,6 +21,7 @@ import java.nio.file.PathMatcher;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.util.stream.IntStream;
import java.util.stream.Stream;
@ -364,7 +365,10 @@ public class ClassPathDocumentLoader {
Path pathMatcherRoot,
PathMatcher pathMatcher,
DocumentParser documentParser) {
return pathStream
AtomicInteger blank = new AtomicInteger();
AtomicInteger failed = new AtomicInteger();
List<Document> documents = pathStream
.filter(Files::isRegularFile)
// converting absolute pathMatcherRoot into relative before using pathMatcher
// because patterns defined in pathMatcher are relative to pathMatcherRoot (directoryPath)
@ -379,16 +383,28 @@ public class ClassPathDocumentLoader {
p,
documentParser);
} catch (BlankDocumentException ignored) {
// blank/empty documents are ignored
blank.incrementAndGet();
return null;
} catch (Exception e) {
String message = (e.getCause() != null) ? e.getCause().getMessage() : e.getMessage();
LOG.warn("Failed to load '{}': {}", p, message);
failed.incrementAndGet();
LOG.warn("Failed to load '{}'", p, e);
return null;
}
})
.filter(Objects::nonNull)
.toList();
if (blank.get() > 0 || failed.get() > 0) {
LOG.warn(
"Loaded {} of {} documents from '{}'. Skipped {} that failed to load and {} that were blank.",
documents.size(),
documents.size() + blank.get() + failed.get(),
directoryOnClasspath,
failed.get(),
blank.get());
}
return documents;
}
private static String getRelativePath(

View File

@ -19,6 +19,7 @@ import java.nio.file.PathMatcher;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@ -485,6 +486,8 @@ public class FileSystemDocumentLoader {
private static List<Document> loadDocuments(
Stream<Path> pathStream, PathMatcher pathMatcher, Path pathMatcherRoot, DocumentParser documentParser) {
List<Document> documents = new ArrayList<>();
AtomicInteger blank = new AtomicInteger();
AtomicInteger failed = new AtomicInteger();
pathStream
.filter(Files::isRegularFile)
@ -499,13 +502,23 @@ public class FileSystemDocumentLoader {
Document document = loadDocument(file, documentParser);
documents.add(document);
} catch (BlankDocumentException ignored) {
// blank/empty documents are ignored
blank.incrementAndGet();
} catch (Exception e) {
String message = e.getCause() != null ? e.getCause().getMessage() : e.getMessage();
log.warn("Failed to load '{}': {}", file, message);
failed.incrementAndGet();
log.warn("Failed to load '{}'", file, e);
}
});
if (blank.get() > 0 || failed.get() > 0) {
log.warn(
"Loaded {} of {} documents from '{}'. Skipped {} that failed to load and {} that were blank.",
documents.size(),
documents.size() + blank.get() + failed.get(),
pathMatcherRoot,
failed.get(),
blank.get());
}
return documents;
}
}