From 839ba312a612312a58649bb3bae0568f044ae5ec Mon Sep 17 00:00:00 2001 From: Oliver Mee <102673257+Omee11@users.noreply.github.com> Date: Thu, 2 Jul 2026 21:43:08 +0800 Subject: [PATCH] fix(api): skip unsupported block-children errors instead of aborting sync GET /blocks/{id}/children returns a 400 validation_error when a page contains a block type the integration cannot receive (seen with Notion AI blocks: "Block type ai_block is not supported via the API for your bot type."). This aborted the entire sync --source api run, regardless of how many other pages were left to sync. Classify this specific 400 as non-fatal: skip the unfetchable children batch, record a warning, and continue. The page_blocks "complete" sync marker and RetireSourcePageBlocksNotSyncedAt are both now gated on zero warnings, so a page that synced cleanly before does not have its previously-synced blocks retired the next time it also contains one unfetchable block. Adds isUnsupportedBlockChildrenError and threads a []string warnings return through walkBlocksAt -> walkBlocks -> ingestPage -> Sync, reusing the existing Summary.Warnings / writeAPIWarnings stderr path already used for the forbidden-/users and restricted-resource cases. Covered by three new tests: - TestWalkBlocksSkipsUnsupportedBlockChildren - TestIngestPageLeavesIncompleteWhenBlockChildrenUnsupported - TestWalkBlocksDoesNotRetirePreviousBlocksOnPartialWalk Fixes #59 --- internal/notionapi/api.go | 101 ++++++++++++++------ internal/notionapi/api_test.go | 170 +++++++++++++++++++++++++++++++-- 2 files changed, 235 insertions(+), 36 deletions(-) diff --git a/internal/notionapi/api.go b/internal/notionapi/api.go index 0ea4a8a..a4f10ee 100644 --- a/internal/notionapi/api.go +++ b/internal/notionapi/api.go @@ -73,19 +73,22 @@ func (c Client) Sync(ctx context.Context, st *store.Store) (Summary, error) { return err } for _, page := range pages { - count, comments, err := c.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true, FetchComments: true}) + count, comments, warnings, err := c.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true, FetchComments: true}) if err != nil { return err } s.Pages++ s.Blocks += count s.Comments += comments + s.Warnings = append(s.Warnings, warnings...) } collections, err := c.searchCollections(ctx) if err != nil { return err } for _, collection := range collections { + // Database rows are ingested without FetchBlocks, so + // ingestCollection can never surface block-children warnings. rows, err := c.ingestCollection(ctx, st, collection) if err != nil { return err @@ -205,7 +208,7 @@ type ingestPageOptions struct { FetchComments bool } -func (c Client) ingestPage(ctx context.Context, st *store.Store, page obj, opts ingestPageOptions) (blockCount int, commentCount int, err error) { +func (c Client) ingestPage(ctx context.Context, st *store.Store, page obj, opts ingestPageOptions) (blockCount int, commentCount int, warnings []string, err error) { raw := notiontext.MarshalRaw(page) props := marshalAny(page["properties"]) parent := page.mapObj("parent") @@ -242,38 +245,48 @@ func (c Client) ingestPage(ctx context.Context, st *store.Store, page obj, opts } if opts.FetchBlocks { if err := st.ClearSyncState(ctx, SourceName, "page_blocks", p.ID); err != nil { - return 0, 0, err + return 0, 0, nil, err } } if err := st.UpsertPage(ctx, p); err != nil { - return 0, 0, err + return 0, 0, nil, err } if !p.Alive { if _, err := st.RetireSourcePageBlocks(ctx, SourceName, p.ID); err != nil { - return 0, 0, err + return 0, 0, nil, err } if _, err := st.RetireSourcePageComments(ctx, SourceName, p.ID); err != nil { - return 0, 0, err + return 0, 0, nil, err } - return 0, 0, nil + return 0, 0, nil, nil } var blocks, comments int + var blockWarnings []string if opts.FetchBlocks { - blocks, err = c.walkBlocks(ctx, st, p.ID, p.ID, p.SpaceID) + blocks, blockWarnings, err = c.walkBlocks(ctx, st, p.ID, p.ID, p.SpaceID) if err != nil { - return 0, 0, err - } - if err := st.SetSyncState(ctx, SourceName, "page_blocks", p.ID, "complete"); err != nil { - return 0, 0, err + return 0, 0, nil, err + } + warnings = append(warnings, blockWarnings...) + // Only mark this page's block sync complete when every children batch + // was fetched; a skipped batch (e.g. an unsupported block type) must + // leave it unmarked so notion-mcp's automatic repair pass (see + // automaticCandidates in internal/notionmcp/client.go, which treats an + // unset api/page_blocks "complete" state as a repair candidate) keeps + // retrying it instead of the gap going untracked. + if len(blockWarnings) == 0 { + if err := st.SetSyncState(ctx, SourceName, "page_blocks", p.ID, "complete"); err != nil { + return 0, 0, nil, err + } } } if opts.FetchComments { comments, err = c.ingestComments(ctx, st, p.ID, p.SpaceID) if err != nil { - return 0, 0, err + return 0, 0, warnings, err } } - return blocks, comments, nil + return blocks, comments, warnings, nil } func (c Client) ingestCollection(ctx context.Context, st *store.Store, collection obj) (int, error) { @@ -337,7 +350,10 @@ func (c Client) queryCollection(ctx context.Context, st *store.Store, collection } continue } - if _, _, err := c.ingestPage(ctx, st, obj(m), ingestPageOptions{CollectionID: collectionID}); err != nil { + // Row pages are ingested with FetchBlocks/FetchComments both + // false, so ingestPage never walks blocks here and its warnings + // return is always empty; nothing to propagate. + if _, _, _, err := c.ingestPage(ctx, st, obj(m), ingestPageOptions{CollectionID: collectionID}); err != nil { return count, err } count++ @@ -370,23 +386,30 @@ func (c Client) usesDataSourceAPI() bool { return c.Version >= "2025-09-03" } -func (c Client) walkBlocks(ctx context.Context, st *store.Store, pageID, parentID, spaceID string) (int, error) { +func (c Client) walkBlocks(ctx context.Context, st *store.Store, pageID, parentID, spaceID string) (int, []string, error) { syncedAt, err := st.NextSourceSyncAt(ctx, "block", SourceName) if err != nil { - return 0, err + return 0, nil, err } - count, err := c.walkBlocksAt(ctx, st, pageID, parentID, spaceID, syncedAt) + count, warnings, err := c.walkBlocksAt(ctx, st, pageID, parentID, spaceID, syncedAt) if err != nil { - return count, err + return count, warnings, err } - if _, err := st.RetireSourcePageBlocksNotSyncedAt(ctx, SourceName, pageID, syncedAt); err != nil { - return count, err + // A partial walk (some children batch was skipped, e.g. an unsupported + // block type) must not retire blocks this run never got a chance to + // re-fetch — otherwise a page that previously synced cleanly loses its + // existing content the moment it gains one unfetchable block. + if len(warnings) == 0 { + if _, err := st.RetireSourcePageBlocksNotSyncedAt(ctx, SourceName, pageID, syncedAt); err != nil { + return count, warnings, err + } } - return count, nil + return count, warnings, nil } -func (c Client) walkBlocksAt(ctx context.Context, st *store.Store, pageID, parentID, spaceID string, syncedAt int64) (int, error) { +func (c Client) walkBlocksAt(ctx context.Context, st *store.Store, pageID, parentID, spaceID string, syncedAt int64) (int, []string, error) { var count int + var warnings []string cursor := "" var displayOrder int64 for { @@ -396,7 +419,12 @@ func (c Client) walkBlocksAt(ctx context.Context, st *store.Store, pageID, paren } var resp obj if err := c.do(ctx, http.MethodGet, path, nil, &resp); err != nil { - return count, err + if isUnsupportedBlockChildrenError(err) { + warnings = append(warnings, fmt.Sprintf( + "Skipped children of block %s on page %s: %v", parentID, pageID, err)) + return count, warnings, nil + } + return count, warnings, err } for _, item := range asSlice(resp["results"]) { m, ok := item.(map[string]any) @@ -426,23 +454,24 @@ func (c Client) walkBlocksAt(ctx context.Context, st *store.Store, pageID, paren RawJSON: raw, SyncedAt: syncedAt, }); err != nil { - return count, err + return count, warnings, err } count++ if shouldFetchBlockChildren(block) { - n, err := c.walkBlocksAt(ctx, st, pageID, block.string("id"), spaceID, syncedAt) + n, childWarnings, err := c.walkBlocksAt(ctx, st, pageID, block.string("id"), spaceID, syncedAt) + warnings = append(warnings, childWarnings...) if err != nil { - return count, err + return count, warnings, err } count += n } } if !truthy(resp["has_more"]) { - return count, nil + return count, warnings, nil } cursor, _ = resp["next_cursor"].(string) if cursor == "" { - return count, nil + return count, warnings, nil } } } @@ -668,6 +697,20 @@ func isRestrictedResourceError(err error) bool { return ok && apiErr.StatusCode == http.StatusForbidden && apiErr.Code == "restricted_resource" } +// isUnsupportedBlockChildrenError reports whether err is Notion rejecting a +// block-children listing because the batch contains a block type integrations +// cannot receive (e.g. ai_block). Notion returns this as a 400 validation_error +// rather than a per-block signal, so the whole children listing must be skipped. +func isUnsupportedBlockChildrenError(err error) bool { + apiErr, ok := err.(notionAPIError) + if !ok { + return false + } + return apiErr.StatusCode == http.StatusBadRequest && + apiErr.Code == "validation_error" && + strings.Contains(apiErr.Message, "not supported via the API") +} + func userName(u obj) string { if name := u.string("name"); name != "" { return name diff --git a/internal/notionapi/api_test.go b/internal/notionapi/api_test.go index 8081b26..6025836 100644 --- a/internal/notionapi/api_test.go +++ b/internal/notionapi/api_test.go @@ -345,7 +345,7 @@ func TestIngestPageMarksBlockSyncOnlyAfterSuccessfulWalk(t *testing.T) { }, } client := Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient} - if _, _, err := client.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true}); err != nil { + if _, _, _, err := client.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true}); err != nil { t.Fatal(err) } synced, err := st.HasSyncState(ctx, SourceName, "page_blocks", "page1") @@ -357,7 +357,7 @@ func TestIngestPageMarksBlockSyncOnlyAfterSuccessfulWalk(t *testing.T) { } failBlocks = true - if _, _, err := client.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true}); err == nil { + if _, _, _, err := client.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true}); err == nil { t.Fatal("expected failed block walk") } synced, err = st.HasSyncState(ctx, SourceName, "page_blocks", "page1") @@ -414,7 +414,7 @@ func TestIngestArchivedPageRestoresDesktopBlocksWithoutRefetching(t *testing.T) }, }, } - if _, _, err := (Client{BaseURL: server.URL, Token: "secret"}).ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true, FetchComments: true}); err != nil { + if _, _, _, err := (Client{BaseURL: server.URL, Token: "secret"}).ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true, FetchComments: true}); err != nil { t.Fatal(err) } if requests != 0 { @@ -443,6 +443,77 @@ func TestIngestArchivedPageRestoresDesktopBlocksWithoutRefetching(t *testing.T) } } +func TestWalkBlocksDoesNotRetirePreviousBlocksOnPartialWalk(t *testing.T) { + syncNumber := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path != "/blocks/page1/children" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String()) + } + if syncNumber == 1 { + _, _ = w.Write([]byte(`{ + "object":"list", + "results":[ + {"object":"block","id":"para-a","type":"paragraph","has_children":false, + "created_time":"2026-01-01T00:00:00Z","last_edited_time":"2026-01-01T00:00:00Z", + "paragraph":{"rich_text":[{"plain_text":"A"}]}}, + {"object":"block","id":"para-b","type":"paragraph","has_children":false, + "created_time":"2026-01-01T00:00:00Z","last_edited_time":"2026-01-01T00:00:00Z", + "paragraph":{"rich_text":[{"plain_text":"B"}]}} + ], + "has_more":false + }`)) + return + } + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{ + "object":"error", + "status":400, + "code":"validation_error", + "message":"Block type ai_block is not supported via the API for your bot type." + }`)) + })) + defer server.Close() + + st, err := store.Open(filepath.Join(t.TempDir(), "notcrawl.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + client := Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient} + ctx := context.Background() + + syncNumber = 1 + if _, _, err := client.walkBlocks(ctx, st, "page1", "page1", "space1"); err != nil { + t.Fatalf("first sync: %v", err) + } + blocks, err := st.PageBlocks(ctx, "page1") + if err != nil { + t.Fatal(err) + } + if len(blocks) != 2 { + t.Fatalf("expected 2 blocks after first sync, got %d", len(blocks)) + } + + // Simulate the page later gaining an unsupported block, which makes the + // whole children batch 400 on the second sync. + syncNumber = 2 + _, warnings, err := client.walkBlocks(ctx, st, "page1", "page1", "space1") + if err != nil { + t.Fatalf("second sync: %v", err) + } + if len(warnings) != 1 { + t.Fatalf("expected one warning from the partial second sync, got %+v", warnings) + } + blocks, err = st.PageBlocks(ctx, "page1") + if err != nil { + t.Fatal(err) + } + if len(blocks) != 2 { + t.Fatalf("partial walk retired previously-synced blocks: got %d blocks, want 2", len(blocks)) + } +} + func TestWalkBlocksSkipsSyncedBlockCopyChildren(t *testing.T) { requestedCopyChildren := false server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -478,7 +549,7 @@ func TestWalkBlocksSkipsSyncedBlockCopyChildren(t *testing.T) { } defer st.Close() - count, err := (Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient}).walkBlocks(context.Background(), st, "page1", "page1", "space1") + count, _, err := (Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient}).walkBlocks(context.Background(), st, "page1", "page1", "space1") if err != nil { t.Fatal(err) } @@ -497,6 +568,91 @@ func TestWalkBlocksSkipsSyncedBlockCopyChildren(t *testing.T) { } } +func TestWalkBlocksSkipsUnsupportedBlockChildren(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path != "/blocks/page1/children" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String()) + } + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{ + "object":"error", + "status":400, + "code":"validation_error", + "message":"Block type ai_block is not supported via the API for your bot type." + }`)) + })) + defer server.Close() + + st, err := store.Open(filepath.Join(t.TempDir(), "notcrawl.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + + count, warnings, err := (Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient}).walkBlocks(context.Background(), st, "page1", "page1", "space1") + if err != nil { + t.Fatalf("expected unsupported block children to be skipped, not fatal: %v", err) + } + if count != 0 { + t.Fatalf("unexpected block count: %d", count) + } + if len(warnings) != 1 { + t.Fatalf("expected one warning, got %+v", warnings) + } +} + +func TestIngestPageLeavesIncompleteWhenBlockChildrenUnsupported(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path != "/blocks/page1/children" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String()) + } + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{ + "object":"error", + "status":400, + "code":"validation_error", + "message":"Block type ai_block is not supported via the API for your bot type." + }`)) + })) + defer server.Close() + + st, err := store.Open(filepath.Join(t.TempDir(), "notcrawl.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + ctx := context.Background() + page := obj{ + "id": "page1", + "archived": false, + "in_trash": false, + "parent": map[string]any{"type": "workspace", "workspace": true}, + "properties": map[string]any{ + "Name": map[string]any{ + "id": "title", "type": "title", + "title": []any{map[string]any{"plain_text": "Page"}}, + }, + }, + } + client := Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient} + _, _, warnings, err := client.ingestPage(ctx, st, page, ingestPageOptions{FetchBlocks: true}) + if err != nil { + t.Fatalf("expected unsupported block children to be non-fatal: %v", err) + } + if len(warnings) != 1 { + t.Fatalf("expected one warning, got %+v", warnings) + } + synced, err := st.HasSyncState(ctx, SourceName, "page_blocks", "page1") + if err != nil { + t.Fatal(err) + } + if synced { + t.Fatal("page with skipped block children was marked complete") + } +} + func TestWalkBlocksRetiresMissingAPIBlocksAfterSuccessfulWalk(t *testing.T) { includeBlock := true server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -531,7 +687,7 @@ func TestWalkBlocksRetiresMissingAPIBlocksAfterSuccessfulWalk(t *testing.T) { } defer st.Close() client := Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient} - if _, err := client.walkBlocks(ctx, st, "page1", "page1", "space1"); err != nil { + if _, _, err := client.walkBlocks(ctx, st, "page1", "page1", "space1"); err != nil { t.Fatal(err) } blocks, err := st.PageBlocks(ctx, "page1") @@ -543,7 +699,7 @@ func TestWalkBlocksRetiresMissingAPIBlocksAfterSuccessfulWalk(t *testing.T) { } includeBlock = false - if _, err := client.walkBlocks(ctx, st, "page1", "page1", "space1"); err != nil { + if _, _, err := client.walkBlocks(ctx, st, "page1", "page1", "space1"); err != nil { t.Fatal(err) } blocks, err = st.PageBlocks(ctx, "page1") @@ -601,7 +757,7 @@ func TestWalkBlocksFetchesOriginalSyncedBlockChildren(t *testing.T) { } defer st.Close() - count, err := (Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient}).walkBlocks(context.Background(), st, "page1", "page1", "space1") + count, _, err := (Client{BaseURL: server.URL, Version: "2026-03-11", Token: "secret", HTTP: http.DefaultClient}).walkBlocks(context.Background(), st, "page1", "page1", "space1") if err != nil { t.Fatal(err) }