[BUG] When using (*JsonParseNode).GetXValue methods it panics

[michaeldcanady]

When using (*JsonParseNode).GetXValue methods it panics due to the value being assert as specific type when it's not.

module version: 1.0.7
golang version: 1.22.5
OS: Window 11

minimum reproducible code:

package main

import (
	"log"

	jsonserialization "github.com/microsoft/kiota-serialization-json-go"
)

func main() {
	data := []byte(`"I am not a float"`)
	node, _ := jsonserialization.NewJsonParseNode(data)
	float, err := node.GetFloat64Value()
	if err != nil {
		log.Print(err)
		return
	}
	log.Print(float)
	return
}

console output:

panic: interface conversion: interface {} is *string, not *float64

goroutine 1 [running]:
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetFloat64Value(0xc000068620)
	.../go/pkg/mod/github.com/microsoft/[email protected]/json_parse_node.go:463 +0xb2
main.main()
	.../min.go:12 +0x105

[michaeldcanady]

simplest fix I can see for this issue is make all GetXValue methods, type check when converting a value. if "ok" is false, return an error. something like:

func (n *JsonParseNode) GetFloat64Value() (*float64, error) {
	if n == nil || n.value == nil {
		return nil, nil
	}

       val, ok := n.value.(*float64)
      if !ok {
         return nil, fmt.Errorf("Expected type: *float64, got %T", n.value)
      }

	return n.value.(*float64), nil
}

[michaeldcanady]

would probably be easiest to just use something like this:

func GetValue[T any](in interface{}) (*T, error) {
	var zero *T

	if in == nil {
		return nil, nil
	}

	val, ok := in.(*T)
	if !ok {
		return nil, fmt.Errorf("Expected type: %T, got %T", zero, in)
	}

	return val, nil
}

in each method

[andrueastman]

Thanks for raising this @michaeldcanady

I believe the suggestion here is valid. The code seems to assume the type is surely known ahead of time but such a failure should indeed be propagated through the error return.

Any chance this is something you'd be willing to submit a PR for?

[michaeldcanady]

Yes, I've already begun work on one! I countered an issue, not with this package, where all numbers are assumed to be float64 and as such fail the type check.

[michaeldcanady]

@andrueastman
Do we want anything that can be of type X to be converted (ex: the value is 1, which is a number, but the user calls GetStringValue, should it error or return "1".)?

[andrueastman]

@andrueastman I believe if it's possible for the conversion to occur, we should return the value.

As this is a Json library, the parsed json would ideally have string surrounded by double quotes. So, calling GetFloat64Value on a json value surrounded by double quotes should fail(its a string). Similarly, if the json value is a number, it won't have doulbe quotes, so calling GetStringValue value would also fail.

This is however different from types derived from strings like Guid where you can still get a string value from it or get the Guid value. Let us know if you have any more questions.

[michaeldcanady]

One more, hopefully, the following test from .\intersection_type_wrapper_test.go is failing - assuming it was passing before - it seems to be failing since the new parsing won't handle the int as string.

func TestItParsesIntersectionTypeComplexProperty2(t *testing.T) {
	source := "{\"displayName\":\"McGill\",\"officeLocation\":\"Montreal\", \"id\": 10}"
	sourceArray := []byte(source)
	parseNode, err := NewJsonParseNode(sourceArray)

	if err != nil {
		t.Error(err)
	}
	result, err := parseNode.GetObjectValue(internal.CreateIntersectionTypeMockFromDiscriminator)
	if err != nil {
		t.Error(err) // value '10' is not compatible with type string
	}
	assert.NotNil(t, result) // Expected value not to be nil.
	cast, ok := result.(internal.IntersectionTypeMockable)
	assert.True(t, ok) // panic: runtime error: invalid memory address or nil pointer dereference
	assert.NotNil(t, cast.GetComposedType1())
	assert.NotNil(t, cast.GetComposedType2())
	assert.Nil(t, cast.GetStringValue())
	assert.Nil(t, cast.GetComposedType3())
	assert.Nil(t, cast.GetComposedType1().GetId())
	assert.Nil(t, cast.GetComposedType2().GetId()) // it's expected to be null since we have conflicting properties here and the parser will only try one to avoid having to brute its way through
	assert.Equal(t, "McGill", *cast.GetComposedType2().GetDisplayName())
}

[michaeldcanady]

Certainly! Here's a more polished and constructive critique:

---

Upon closer inspection, it appears the issue arises from the following code block. If GetStringValue returns an error—which it will, since "id" is a numeric value—the error is returned directly:

res["id"] = func(n absser.ParseNode) error {
    val, err := n.GetStringValue()
    if err != nil {
        return err
    }
    if val != nil {
        e.SetId(val)
    }
    return nil
}

Additionally, in the following code block, there is no check to determine if e or e.composedType1 are nil. This seems to be a commonly missed check in the IntersectionTypeMock, TestEntity, and SecondTestEntity implementations:

func (e *IntersectionTypeMock) GetComposedType1() TestEntityable {
    return e.composedType1
}

This lowers the fault tolerance of the tests and by extension makes it so if the value fails to set the test will panic.

[saviorand]

@michaeldcanady I'm still getting an error here when e.g. converting from string "1" to int32, is this intended behaviour?
I see there's a test for this, but this is pretty important for my use case (getting Drive Items/photos with a string in one of the properties). Or should I go to microsoftgraphsdk repo for this?

failed to get drive items: *errors.errorString > error: &errors.errorString{s:"value '1' is not compatible with type int32"}

[michaeldcanady]

As this is a Json library, the parsed json would ideally have string surrounded by double quotes. So, calling GetFloat64Value on a json value surrounded by double quotes should fail(its a string). Similarly, if the json value is a number, it won't have doulbe quotes, so calling GetStringValue value would also fail.

Hello @saviorand! Thank you for your question, Assuming you’re referenced JSON is like the following:

{
"Key":"1"
}

Then yes.

the goal is to parse what the underlying type is. So in the above example even though it’s a stringified integer, the value is a string. I included the discussion point where this was decided.

[saviorand]

@michaeldcanady I've shared this in an issue upstream in msgraph-sdk microsoftgraph/msgraph-sdk-go#667 , I don't know if you have contact with that team? Seems like they are not responding to issues there since a while

[michaeldcanady]

@saviorand I unfortunately do not.