feat(): Support context document import from local filesystem and GitHub, Notion, Confluence#16903
Conversation
Bundle ReportChanges will increase total bundle size by 535.74kB (1.8%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
|
🔴 Meticulous spotted visual differences in 5 of 1554 screens tested: view and approve differences detected. Meticulous evaluated ~10 hours of user flows against your PR. Last updated for commit |
alexsku
left a comment
There was a problem hiding this comment.
PR Review
Type: behavior change (new feature)
Size: +4432 / -60 across 49 files
This PR adds the ability to import context documents into DataHub from two sources: local file upload (via browser-side text extraction) and GitHub repositories (via the GitHub REST API). It introduces a new DocumentImportService backend, three new GraphQL resolvers (import from files, import from GitHub, preview from GitHub), a React modal UI with source selection, and parent document hierarchy support. It also adds ignore_above to keyword mappings in ES to prevent indexing failures on long text values.
Key invariants:
- Authorization: All import operations require
MANAGE_DOCUMENTSplatform privilege - Idempotency: Documents are created with deterministic IDs derived from source IDs, so re-imports update rather than duplicate
- Hierarchy: GitHub folder structure is preserved as parent-child document relationships; candidates must be ordered parents-first
- The GitHub token is passed per-request and not stored server-side
Risk Assessment
| Risk | Medium |
| Blast radius | Document entities only; ES mapping change affects all entities with TEXT/TEXT_PARTIAL fields |
| Rollback | Safe for import feature (new code, no migrations). ES mapping change (ignore_above) is additive and backward-compatible |
| Rollout | Ship it — import resolvers are null-guarded if factory bean is absent |
| CI | Failing: build-and-test (except_metadata_ingestion, UTC) |
Blocking Issues
None found.
High-Priority Issues
[HIGH] GitHub token passed through GraphQL — potential for logging/persistence
Category: security | Confidence: medium
Location: datahub-graphql-core/src/main/resources/documents.graphql:871 — githubToken: String!
The GitHub PAT is passed as a plain String! in the GraphQL input. While it's used only server-side and not stored, GraphQL query logging, APM tracing, or error serialization could inadvertently capture the token in server logs. The same ImportDocumentsFromGitHubInput type is reused for both the previewDocumentsFromGitHub query and the importDocumentsFromGitHub mutation, meaning the token is sent even for read-only preview operations.
Suggested fix: Ensure GraphQL request logging is configured to redact githubToken. Consider whether the preview query truly needs the token in the same input type (it does for private repos, so this is likely acceptable). Document the logging risk.
How to validate: Check if the DataHub GMS logging configuration logs full GraphQL variables. Search for query logging middleware.
[HIGH] No limit on number of files fetched from GitHub — unbounded server-side work
Category: performance | Confidence: high
Location: metadata-service/services/src/main/java/com/linkedin/metadata/service/docimport/GitHubDocumentSource.java:166-187 — fetchDocuments()
The fetchDocuments method iterates over all matching files from the GitHub tree API and fetches each file's content one-by-one via individual HTTP requests (line 167: fetchFileContent). For a large repo with thousands of matching files, this creates an unbounded number of sequential HTTP requests from the GMS server, tying up a GraphQL worker thread for potentially minutes. There's no configurable limit or pagination.
Suggested fix: Add a maxFiles parameter (defaulting to something like 500) and stop fetching after the limit. Log a warning when truncated.
How to validate: Point the import at a large repo (e.g., a docs monorepo with 1000+ .md files) and observe GMS behavior.
Other Issues
-
[MEDIUM / edge-case]
GitHubDocumentSource.java:391—matchesFiltersusesstartsWith(pathPrefix)without a/boundary. Prefix"doc"would match"document/README.md". Fix: append/to non-empty prefix before the startsWith check, or usefilePath.startsWith(pathPrefix + "/"). -
[MEDIUM / edge-case]
GitHubDocumentSource.java:90-92— The GitHub tree API returns atruncated: truefield when the tree is too large (>100K entries). This is not checked. If truncated, the user silently gets an incomplete file list. Fix: checktree.path("truncated").asBoolean()and warn the user. -
[MEDIUM / performance]
DocumentImportService.java:190—entityClient.exists()is called for every candidate inside a loop. For a batch import of N documents, this makes N individual existence checks. Fix: consider batching existence checks or accepting that the first import will always be "create". -
[MEDIUM / correctness]
DocumentImportService.java:275-283—makeDocumentIdcan produce collisions. Two different source IDs that differ only in characters replaced by-(e.g.,"docs/a b"and"docs/a-b") will produce the same document ID. File-upload source IDs likeupload.my fileandupload.my-filewould collide. -
[MEDIUM / dx]
ImportDocumentsModal.tsx:210-212—<Text color="gray" colorLevel={1700}>passes color directly to alchemy<Text>components. Per the project's theming guidance, colors should come from theme tokens rather than being passed as props to alchemy components. -
[LOW / operability]
GitHubDocumentSource.java:362-386—executeGitHubGetreturnsnullon non-200 responses but doesn't distinguish between 401 (bad token), 403 (rate limit), and 404 (repo not found). A more specific error message would help users troubleshoot.
What's Missing
- No limit on imported file count: The GitHub import has no cap on the number of files processed. Both the frontend and backend should enforce a maximum.
- No cancellation support: The GitHub import is synchronous on the GraphQL thread. Consider async execution with progress tracking for large imports.
- Bundle size increase: Codecov reports +528KB (2.33%) from the
mammothDOCX library. Consider lazy-loading it only when the import modal is opened.
Test Plan
| Invariant | Covered? | Suggested test |
|---|---|---|
| Auth check on all import endpoints | Yes — resolver tests verify AuthorizationException |
— |
| Idempotent re-import (same URN) | Yes — smoke test test_import_file_upload_idempotent |
— |
| Parent-child hierarchy | Yes — smoke test test_import_file_upload_with_parent |
— |
makeDocumentId collision |
No | Unit test: verify distinct source IDs don't collide |
| Large repo truncation handling | No | Integration test: mock truncated tree response |
ES ignore_above behavior |
Partial — FieldTypeMapperTest covers mapping generation |
Could add integration test indexing a >8191 char keyword |
Questions for Author
- Is the CI build failure (
build-and-test) unrelated to this PR, or does it need to be fixed before merge? - Was a file count limit for GitHub imports intentionally omitted, or is it planned for a follow-up?
- Should the
mammothdependency be lazy-loaded to reduce the bundle impact for users who don't use the import feature? - The ES
ignore_above/ keyword mapping changes seem orthogonal to the document import feature — was this bundled intentionally due to document text fields hitting the limit, or could it be split into a separate PR for cleaner rollback?
4484385 to
d8a61f3
Compare
50b3b11 to
9b1ba47
Compare
Connector Tests ResultsAll connector tests passed for commit To skip connector tests, add the Autogenerated by the connector-tests CI pipeline. |
Rebase document-import work onto latest oss/master, merging OSS sidebar filters/i18n with ImportDocumentsButton and adapting github-documents stale-removal to the current StaleEntityRemovalHandler API. Co-authored-by: Cursor <cursoragent@cursor.com>
Force the patched version pulled in transitively by mammoth for docx import. Co-authored-by: Cursor <cursoragent@cursor.com>
674f412 to
2961e5f
Compare
shirshanka
left a comment
There was a problem hiding this comment.
Approving - please check CI!
…Hub, Notion, Confluence (#16903) Co-authored-by: John Joyce <john@ip-192-168-1-212.us-west-2.compute.internal> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: John Joyce <john@Mac-5837.lan> Co-authored-by: John Joyce <john@Mac-5917.lan> Co-authored-by: John Joyce <john@Mac-6389.lan>
Summary
Adding a PR to introduce support for importing context inside the Documents section from local filesystem (one-time), GitHub (ingestion), Notion (ingestion), and Confluence (ingestion).
Also, extend the Notion + Confluence sources to support configuring the document source type, so you can ingest as a native document if you want to import into the editable experience under documents.
See the video below for the full experience.
Video Walkthrough
import-docs-flow.mov
Screenshots
Status
Ready for review.