slack icon indicating copy to clipboard operation
slack copied to clipboard

Fix the block ContextElements field marshaling

Open geodimm opened this issue 3 years ago • 4 comments

The ContextElements field is incorrectly marshalled to JSON - it adds an extra nested "Elements" field to the resulting "elements" field in the object. The issue seems to be caused by the https://github.com/slack-go/slack/blob/93035c946040f00b2eb69dc9a954f83a9803e931/block_conv.go#L364 function signature. The receiver shouldn't be a pointer.

Relevant issue describing the same problem: https://github.com/golang/go/issues/6468

Here's a code snippet to reproduce:

package main

import (
	"encoding/json"
	"fmt"

	"github.com/slack-go/slack"
)

func main() {
	fmt.Println("1. Start with valid JSON")
	inputJSON := `{"type":"context","elements":[{"type":"image","image_url":"https://github.githubassets.com/favicons/favicon.svg","alt_text":"github logo"},{"type":"mrkdwn","text":"issue"}]} `
	fmt.Printf("\tInput: %+v\n", inputJSON)
	parsed := slack.ContextBlock{}
	fmt.Println("2. Unmarshal")
	err := json.Unmarshal([]byte(inputJSON), &parsed)
	if err != nil {
		panic(err)
	}

	fmt.Printf("\tParsed: %+v\n", parsed)

	fmt.Println("3. Marshal")
	mashaled, err := json.Marshal(parsed)
	if err != nil {
		panic(err)
	}

	fmt.Printf("\tMarshaled: %s\n", mashaled)

	parsedAgain := slack.ContextBlock{}
	fmt.Println("4. Unmarshal again")
	err = json.Unmarshal(mashaled, &parsedAgain)
	if err != nil {
		panic(err)
	}

	fmt.Printf("\tParsed again: %+v\n", parsedAgain)
}

Output of running the program before and after the fix

Before

$ go run main.go

1. Start with valid JSON
        Input: {"type":"context","elements":[{"type":"image","image_url":"https://github.githubassets.com/favicons/favicon.svg","alt_text":"github logo"},{"type":"mrkdwn","text":"issue"}]}
2. Unmarshal
        Parsed: {Type:context BlockID: ContextElements:{Elements:[0xc0000a30e0 0xc0000a3230]}}
3. Marshal
        Marshaled: {"type":"context","elements":{"Elements":[{"type":"image","image_url":"https://github.githubassets.com/favicons/favicon.svg","alt_text":"github logo"},{"type":"mrkdwn","text":"issue"}]}}
4. Unmarshal again
panic: json: cannot unmarshal object into Go struct field ContextBlock.elements of type []json.RawMessage

goroutine 1 [running]:
main.main()
        /repos/gopg/main.go:34 +0x425
exit status 2

After

$ go run main.go

1. Start with valid JSON
        Input: {"type":"context","elements":[{"type":"image","image_url":"https://github.githubassets.com/favicons/favicon.svg","alt_text":"github logo"},{"type":"mrkdwn","text":"issue"}]}
2. Unmarshal
        Parsed: {Type:context BlockID: ContextElements:{Elements:[0xc0000a2ff0 0xc0000a3140]}}
3. Marshal
        Marshaled: {"type":"context","elements":[{"type":"image","image_url":"https://github.githubassets.com/favicons/favicon.svg","alt_text":"github logo"},{"type":"mrkdwn","text":"issue"}]}
4. Unmarshal again
        Parsed again: {Type:context BlockID: ContextElements:{Elements:[0xc0000a3470 0xc0000a3500]}}

geodimm avatar Apr 14 '22 17:04 geodimm

In this case, you can parse correctly if you fix your snippets like below.

// before L14
parsed := slack.ContextBlock{}

// after L 14
var parsed *slack.ContextBlock

I don't think this is a bug. However, I believe it is also easy for users to misuse.

kanata2 avatar Apr 14 '22 22:04 kanata2

Yes, indeed. Having the receiver as a value instead of a pointer works either way. Having it as a pointer works only if a pointer is passed.

geodimm avatar Apr 15 '22 11:04 geodimm

If we change the receiver type to a value instead of a pointer, I believe this change may be applied to all XxxBlock types. This is to avoid misuse.

kanata2 avatar Apr 15 '22 12:04 kanata2

Hi @kanata2 ,

Sorry for the long delay. Here's the change applied to all types including some tests :)

geodimm avatar Apr 30 '22 16:04 geodimm