Don't panic when unmarshaling blocks (#50)

* don't panic

* Block() returns err

* Rework unknown block type logic, add tests

---------

Co-authored-by: David Stotijn <dstotijn@gmail.com>

block.go

@@ -2,10 +2,14 @@ package notion
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"time"
 )
 
+// ErrUnknownBlockType is used when encountering an unknown block type.
+var ErrUnknownBlockType = errors.New("unknown block type")
+
 // Block represents content on the Notion platform.
 // See: https://developers.notion.com/reference/block
 type Block interface {
@@ -63,6 +67,7 @@ type blockDTO struct {
 	LinkToPage       *LinkToPageBlock       `json:"link_to_page,omitempty"`
 	SyncedBlock      *SyncedBlock           `json:"synced_block,omitempty"`
 	Template         *TemplateBlock         `json:"template,omitempty"`
+	Unsupported      *UnsupportedBlock      `json:"unsupported,omitempty"`
 }
 
 type baseBlock struct {
@@ -812,6 +817,24 @@ func (b BreadcrumbBlock) MarshalJSON() ([]byte, error) {
 	})
 }
 
+type UnsupportedBlock struct {
+	baseBlock
+}
+
+// MarshalJSON implements json.Marshaler.
+func (b UnsupportedBlock) MarshalJSON() ([]byte, error) {
+	type (
+		blockAlias UnsupportedBlock
+		dto        struct {
+			Unsupported blockAlias `json:"unsupported"`
+		}
+	)
+
+	return json.Marshal(dto{
+		Unsupported: blockAlias(b),
+	})
+}
+
 type BlockType string
 
 const (
@@ -880,13 +903,21 @@ func (resp *BlockChildrenResponse) UnmarshalJSON(b []byte) error {
 	resp.Results = make([]Block, len(dto.Results))
 
 	for i, blockDTO := range dto.Results {
-		resp.Results[i] = blockDTO.Block()
+		block, err := blockDTO.Block()
+		if err != nil {
+			// Any error (even `ErrUnknownBlockType`) is explicitly returned.
+			// We don't silently drop blocks with an unknown/unmapped type,
+			// because this could lead to surprises/unexpected list behaviour
+			// for users.
+			return fmt.Errorf("notion: failed to parse block (id: %q, type: %q): %w", blockDTO.ID, blockDTO.Type, err)
+		}
+		resp.Results[i] = block
 	}
 
 	return nil
 }
 
-func (dto blockDTO) Block() Block {
+func (dto blockDTO) Block() (Block, error) {
 	baseBlock := baseBlock{
 		id:          dto.ID,
 		hasChildren: dto.HasChildren,
@@ -919,101 +950,106 @@ func (dto blockDTO) Block() Block {
 	switch dto.Type {
 	case BlockTypeParagraph:
 		dto.Paragraph.baseBlock = baseBlock
-		return dto.Paragraph
+		return dto.Paragraph, nil
 	case BlockTypeHeading1:
 		dto.Heading1.baseBlock = baseBlock
-		return dto.Heading1
+		return dto.Heading1, nil
 	case BlockTypeHeading2:
 		dto.Heading2.baseBlock = baseBlock
-		return dto.Heading2
+		return dto.Heading2, nil
 	case BlockTypeHeading3:
 		dto.Heading3.baseBlock = baseBlock
-		return dto.Heading3
+		return dto.Heading3, nil
 	case BlockTypeBulletedListItem:
 		dto.BulletedListItem.baseBlock = baseBlock
-		return dto.BulletedListItem
+		return dto.BulletedListItem, nil
 	case BlockTypeNumberedListItem:
 		dto.NumberedListItem.baseBlock = baseBlock
-		return dto.NumberedListItem
+		return dto.NumberedListItem, nil
 	case BlockTypeToDo:
 		dto.ToDo.baseBlock = baseBlock
-		return dto.ToDo
+		return dto.ToDo, nil
 	case BlockTypeToggle:
 		dto.Toggle.baseBlock = baseBlock
-		return dto.Toggle
+		return dto.Toggle, nil
 	case BlockTypeChildPage:
 		dto.ChildPage.baseBlock = baseBlock
-		return dto.ChildPage
+		return dto.ChildPage, nil
 	case BlockTypeChildDatabase:
 		dto.ChildDatabase.baseBlock = baseBlock
-		return dto.ChildDatabase
+		return dto.ChildDatabase, nil
 	case BlockTypeCallout:
 		dto.Callout.baseBlock = baseBlock
-		return dto.Callout
+		return dto.Callout, nil
 	case BlockTypeQuote:
 		dto.Quote.baseBlock = baseBlock
-		return dto.Quote
+		return dto.Quote, nil
 	case BlockTypeCode:
 		dto.Code.baseBlock = baseBlock
-		return dto.Code
+		return dto.Code, nil
 	case BlockTypeEmbed:
 		dto.Embed.baseBlock = baseBlock
-		return dto.Embed
+		return dto.Embed, nil
 	case BlockTypeImage:
 		dto.Image.baseBlock = baseBlock
-		return dto.Image
+		return dto.Image, nil
 	case BlockTypeAudio:
 		dto.Audio.baseBlock = baseBlock
-		return dto.Audio
+		return dto.Audio, nil
 	case BlockTypeVideo:
 		dto.Video.baseBlock = baseBlock
-		return dto.Video
+		return dto.Video, nil
 	case BlockTypeFile:
 		dto.File.baseBlock = baseBlock
-		return dto.File
+		return dto.File, nil
 	case BlockTypePDF:
 		dto.PDF.baseBlock = baseBlock
-		return dto.PDF
+		return dto.PDF, nil
 	case BlockTypeBookmark:
 		dto.Bookmark.baseBlock = baseBlock
-		return dto.Bookmark
+		return dto.Bookmark, nil
 	case BlockTypeEquation:
 		dto.Equation.baseBlock = baseBlock
-		return dto.Equation
+		return dto.Equation, nil
 	case BlockTypeDivider:
 		dto.Divider.baseBlock = baseBlock
-		return dto.Divider
+		return dto.Divider, nil
 	case BlockTypeTableOfContents:
 		dto.TableOfContents.baseBlock = baseBlock
-		return dto.TableOfContents
+		return dto.TableOfContents, nil
 	case BlockTypeBreadCrumb:
 		dto.Breadcrumb.baseBlock = baseBlock
-		return dto.Breadcrumb
+		return dto.Breadcrumb, nil
 	case BlockTypeColumnList:
 		dto.ColumnList.baseBlock = baseBlock
-		return dto.ColumnList
+		return dto.ColumnList, nil
 	case BlockTypeColumn:
 		dto.Column.baseBlock = baseBlock
-		return dto.Column
+		return dto.Column, nil
 	case BlockTypeTable:
 		dto.Table.baseBlock = baseBlock
-		return dto.Table
+		return dto.Table, nil
 	case BlockTypeTableRow:
 		dto.TableRow.baseBlock = baseBlock
-		return dto.TableRow
+		return dto.TableRow, nil
 	case BlockTypeLinkPreview:
 		dto.LinkPreview.baseBlock = baseBlock
-		return dto.LinkPreview
+		return dto.LinkPreview, nil
 	case BlockTypeLinkToPage:
 		dto.LinkToPage.baseBlock = baseBlock
-		return dto.LinkToPage
+		return dto.LinkToPage, nil
 	case BlockTypeSyncedBlock:
 		dto.SyncedBlock.baseBlock = baseBlock
-		return dto.SyncedBlock
+		return dto.SyncedBlock, nil
 	case BlockTypeTemplate:
 		dto.Template.baseBlock = baseBlock
-		return dto.Template
+		return dto.Template, nil
+	case BlockTypeUnsupported:
+		dto.Unsupported.baseBlock = baseBlock
+		return dto.Unsupported, nil
 	default:
-		panic(fmt.Sprintf("type %q is unsupported", dto.Type))
+		// When this case is selected, the block type is supported in the Notion
+		// API, but unknown in this library.
+		return nil, ErrUnknownBlockType
 	}
 }

client.go

@@ -437,7 +437,7 @@ func (c *Client) FindBlockByID(ctx context.Context, blockID string) (Block, erro
 		return nil, fmt.Errorf("notion: failed to parse HTTP response: %w", err)
 	}
 
-	return dto.Block(), nil
+	return dto.Block()
 }
 
 // UpdateBlock updates a block.
@@ -472,10 +472,11 @@ func (c *Client) UpdateBlock(ctx context.Context, blockID string, block Block) (
 		return nil, fmt.Errorf("notion: failed to parse HTTP response: %w", err)
 	}
 
-	return dto.Block(), nil
+	return dto.Block()
 }
 
 // DeleteBlock sets `archived: true` on a (page) block object.
+// Will return UnsupportedBlockError if it deletes the block but cannot decode it
 // See: https://developers.notion.com/reference/delete-a-block
 func (c *Client) DeleteBlock(ctx context.Context, blockID string) (Block, error) {
 	req, err := c.newRequest(ctx, http.MethodDelete, "/blocks/"+blockID, nil)
@@ -500,7 +501,7 @@ func (c *Client) DeleteBlock(ctx context.Context, blockID string) (Block, error)
 		return nil, fmt.Errorf("notion: failed to parse HTTP response: %w", err)
 	}
 
-	return dto.Block(), nil
+	return dto.Block()
 }
 
 // FindUserByID fetches a user by ID.

client_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -3074,6 +3075,15 @@ func TestFindBlockChildrenById(t *testing.T) {
 										}
 									]
 								}
+							},
+							{
+								"object": "block",
+								"id": "5e113754-eae4-4da9-96d2-675977acce99",
+								"created_time": "2021-05-14T09:15:00.000Z",
+								"last_edited_time": "2021-05-14T09:15:00.000Z",
+								"has_children": false,
+								"type": "unsupported",
+								"unsupported": {}
 							}
 						],
 						"next_cursor": "A^hd",
@@ -3102,6 +3112,7 @@ func TestFindBlockChildrenById(t *testing.T) {
 							},
 						},
 					},
+					&notion.UnsupportedBlock{},
 				},
 				HasMore:    true,
 				NextCursor: notion.StringPtr("A^hd"),
@@ -3114,6 +3125,13 @@ func TestFindBlockChildrenById(t *testing.T) {
 					hasChildren:    false,
 					archived:       false,
 				},
+				{
+					id:             "5e113754-eae4-4da9-96d2-675977acce99",
+					createdTime:    mustParseTime(time.RFC3339, "2021-05-14T09:15:00.000Z"),
+					lastEditedTime: mustParseTime(time.RFC3339, "2021-05-14T09:15:00.000Z"),
+					hasChildren:    false,
+					archived:       false,
+				},
 			},
 			expError: nil,
 		},
@@ -3139,6 +3157,30 @@ func TestFindBlockChildrenById(t *testing.T) {
 			},
 			expError: nil,
 		},
+		{
+			name: "unknown block type",
+			respBody: func(_ *http.Request) io.Reader {
+				return strings.NewReader(
+					`{
+						"object": "list",
+						"results": [
+							{
+								"object": "block",
+								"id": "ae9c9a31-1c1e-4ae2-a5ee-c539a2d43113",
+								"created_time": "2021-05-14T09:15:00.000Z",
+								"last_edited_time": "2021-05-14T09:15:00.000Z",
+								"has_children": false,
+								"type": "foobar"
+							}
+						],
+						"next_cursor": null,
+						"has_more": false
+					}`,
+				)
+			},
+			respStatusCode: http.StatusOK,
+			expError:       fmt.Errorf(`notion: failed to parse HTTP response: notion: failed to parse block (id: "ae9c9a31-1c1e-4ae2-a5ee-c539a2d43113", type: "foobar"): unknown block type`),
+		},
 		{
 			name: "error response",
 			respBody: func(_ *http.Request) io.Reader {
@@ -3200,7 +3242,7 @@ func TestFindBlockChildrenById(t *testing.T) {
 				t.Fatalf("error not equal (expected: %v, got: %v)", tt.expError, err)
 			}
 
-			if diff := cmp.Diff(tt.expResponse, resp, cmpopts.IgnoreUnexported(notion.ParagraphBlock{})); diff != "" {
+			if diff := cmp.Diff(tt.expResponse, resp, cmpopts.IgnoreUnexported(notion.ParagraphBlock{}, notion.UnsupportedBlock{})); diff != "" {
 				t.Fatalf("response not equal (-exp, +got):\n%v", diff)
 			}