Models should implement std lib's JSON Un/marshaler interfaces

[eric-millin]

I was writing tests and was very surprised to find that the models can't be de/serialized using standard Go conventions built around the std lib.

I did a little testing and found that the std lib's interfaces are easy to implement as wrappers around the existing deserialization code. Here's what I go working for a mock ChangeNotificationCollectionResponse.

func (m *ChangeNotificationCollectionResponse) UnmarshalJSON(b []byte) error {
	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(models.CreateChangeNotificationCollectionResponseFromDiscriminatorValue)
	if err != nil {
		return err
	}

	resp := v.(*models.ChangeNotificationCollectionResponse)
	m.BaseCollectionPaginationCountResponse = resp.BaseCollectionPaginationCountResponse
	m.value = resp.GetValue()

	return nil
}

The code above is a poc implemented in my own project. I believe you should be able to do it even more simply

func (m *ChangeNotificationCollectionResponse) UnmarshalJSON(b []byte) error {
	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(models.CreateChangeNotificationCollectionResponseFromDiscriminatorValue)
	if err != nil {
		return err
	}

        *m = *v.(*ChangeNotificationCollectionResponse)

         return nil
}

Add the UnmarshalJSON wrapper then allowed me to use json.Unmarshal.

[eric-millin]

I imagine your code is autogenerated, so you may not care about repetition across files. If you do care, I realized you can save yourself some trouble by leveraging generics

func UnmarshalJSON[T serialization.Parsable](b []byte, model *T, parser serialization.ParsableFactory) error {
	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(parser)
	if err != nil {
		return err
	}

	*model = v.(T)

	return nil
}

//...

func (m *ChangeNotificationCollectionResponse) UnmarshalJSON(b []byte) error {
	return UnmarshalJSON(
		b, &m, models.CreateChangeNotificationCollectionResponseFromDiscriminatorValue,
	)
}

[baywet]

Hi @eric-millin,
Thanks for using the Go SDK and for reaching out.

You are correct, this is generated using kiota.

Marshalling JSON

There are a number of reasons why we didn't enable support for this scenario:

• The additional indirection allows support different serialization formats (JSON/text/uri form encoded...)
• Support for discriminator (a field of the payload dictates which type to use)
• Support for advanced types that are serialized as a string in JSON (duration, date only, time only, date time offset, ...)
• Support for dirty tracking through a backing store (upcoming)

Ultimately this SDK should be an infrastructure layer (in DDD terms) and the service should be encapsulated, allowing for easy mocking and standard communication with the other layers through application layer models.

Generics usage

While working to improve this preview SDK, we actually did try Go generics. Our findings were that with such a large codebase the memory pressure on the compiler increased by a lot, making builds fail. After lots of considerations we decided to roll the use of generics back. You can see our progression in #129

With that additional context, is there something we're missing in terms of scenarios that might not be possible today?

[eric-millin]

With that additional context, is there something we're missing in terms of scenarios that might not be possible today?

Thanks for the speedy reply and detailed answer.

I asked my team as a sanity check and everyone was very surprised that the structs don't support using the stdlib's json.Un/Marshal functions. In fact, I can't think of a package I've seen that supports JSON serialization, but doesn't support using the stdlib to do it.

I certainly understand why you implemented your own deserialization approach. However, I'm missing what prevents implementing the Un/marshaler interfaces as an additive change. UnmarshalJSON and MarshalJSON would just be wrappers around your custom implementation--in fact, that's why those interfaces exist, to allow for custom deserialization behavior.

As to why someone would want to deserialize in the first place, the simplest example I can give is in our endpoint for change notifications. We get a notification blob and we want to unmarshal it to an SDK model. I expect to be able to do that with json.Unmarshal, but I can't.

[eric-millin]

• The additional indirection allows support different serialization formats (JSON/text/uri form encoded...)
• Support for discriminator (a field of the payload dictates which type to use)
• Support for advanced types that are serialized as a string in JSON (duration, date only, time only, date time offset, ...)

Here's my example implementation that would seem to satisfy your requirements

func (m *ChangeNotificationCollectionResponse) UnmarshalJSON(b []byte) error {
	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(models.CreateChangeNotificationCollectionResponseFromDiscriminatorValue)
	if err != nil {
		return err
	}

        *m = *v.(*ChangeNotificationCollectionResponse)

         return nil
}

[baywet]

Thanks for the additional context here.

This approach (customizing the default platform serialization behavior) was the approach we were using for the current dotnet and java SDKs (non kiota based). It led at best to convoluted code, at worst to inaccuracies with the serialized/deserialized results. And it ties the model to specific serialization format.

Have you tried using the JsonParseNode to handle the deserialization of such types for you automatically? You could do so by implementing the Parsable interface (like other models) until we ship types for change notifications themselves.

[eric-millin]

Thanks for the additional context here.

This approach (customizing the default platform serialization behavior) was the approach we were using for the current dotnet and java SDKs (non kiota based). It led at best to convoluted code, at worst to inaccuracies with the serialized/deserialized results. And it ties the model to specific serialization format.

Have you tried using the JsonParseNode to handle the deserialization of such types for you automatically? You could do so by implementing the Parsable interface (like other models) until we ship types for change notifications themselves.

I think so: this is the pattern we're following now for change notifications

	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(models.CreateChangeNotificationCollectionResponseFromDiscriminatorValue)
	if err != nil {
		return err
	}

So, there is a way to do what I want, but it doesn't enable me to follow Go conventions.

Clearly I don't understand how the files are generated, which I imagine isn't easy to put together. However, as an outsider it doesn't look difficult to write a script that would iterate the struct files in the models directory, find the discriminator name in file, then use that to generate working Un/MarshalJSON functions that can be appended to the code.

[baywet]

Besides the potential difficulty it'd transgress a tenant of Kiota client generation that models only rely on kiota abstractions and they are not tied to any specific format or serialization "library". Kiota rolls out with default implementations people are more than welcome to use, but can choose to re-implement should they not be happy with them. So we're not likely to change that aspect now unless we get overwhelming feedback we should. Besides adding this would increase the SDK size significantly which has proven to be a challenge.

I do believe that part of the issue here is that no example is provided to handle change notifications in Go, like we have in java and other languages. But it's on our roadmap once we fix the missing types and once this SDK GAs.

I hope that clarifies our plans and the rationale behind them.

[eric-millin]

Besides adding this would increase the SDK size significantly which has proven to be a challenge.

We experienced some of the side effects of that challenge. Many thanks for the recent changes that made our builds faster and dev experience better.

Re examples, are there any Go examples of subscription token validation? I'm referring to something analogous to this Java code:
https://github.com/microsoftgraph/java-spring-webhooks-sample/blob/a8d6f4fc1339fe2bff35ff3ffdec08ea01e11cd2/graphwebhook/src/main/java/com/example/graphwebhook/TokenHelper.java#L36

[baywet]

Thanks for the kind words.

Not at the moment (at least not published by Microsoft) but it's something on the documentation team that'll happen once we include the missing types and once the SDK GAs.

[eric-millin]

Thanks for all the help. Closing the issue

[eric-millin]

@baywet Just wanted to share one bit of related feedback that I got during code review. Coworker specifically pointed out the need for custom deserialization code and referred to the SDK as "clunky and non-idiomatic".

There are a lot of really great things about the SDK. The abstractions make many things easy, particularly leveraging polymorphism. However, at some points I think you'll need to make it more Go-ish.

Thanks again for your help and thoughtful consideration of the issues I raised.

[baywet]

thanks for the feedback. Would it help if the serialization package had a generic marshall/unmarshall function?

user := NewUser()
//do stuff to user
result := marsall(user)
//byte array
// marsall function comes from github.com/microsoft/kiota-serialization-json-go

This way it'd save people from having to understand the different layers of the client?

[eric-millin]

thanks for the feedback. Would it help if the serialization package had a generic marshall/unmarshall function?

user := NewUser()
//do stuff to user
result := marsall(user)
//byte array
// marsall function comes from github.com/microsoft/kiota-serialization-json-go

This way it'd save people from having to understand the different layers of the client?

@baywet I didn't see this last response, apologies. I think a helper like that would be a good improvement.

I really don't want to be "that guy" who's always harping on the same thing and being annoying as hell, so I'm fine moving on. However, if you have the patience to indulge me, maybe you can explain how wrapping your proposed marshall function in MarshalJSON would be problematic. You obviously don't owe me an explanation, but I am curious from a design perspective. I've done a lot of metacoding in the past, but certainly not at the scale you are.

EDIT: I realized that part of what I don't get about the design is that from the user perspective, the structs are bound to a serialization format by their very existence. The Graph API is JSON, so it seems like it's a no-brainer to make JSON support simple, easy, and idiomatic.

What I envision is roughly

func (user User) MarshalJSON() ([]byte, error) {
   // your helper is used here
   result := marsall(user)
   // etc
}

// now I can do things the Go way
user := NewUser()
//do stuff to user
b, err := json.Marshal(user)

[eric-millin]

FWIW, this is how I do things now, but it sounds like you're saying there's a fragility here that you want to avoid. That makes me wonder if I want to avoid it too...

func UnmarshalGraphModel[T serialization.Parsable](b []byte, model *T, parser serialization.ParsableFactory) error {
	jpn, err := kjson.NewJsonParseNode(b)
	if err != nil {
		return err
	}

	v, err := jpn.GetObjectValue(parser)
	if err != nil {
		return err
	}

	*model = v.(T)

	return nil
}

[baywet]

to clarify my points:

We are not going to add marshalling functions to each model. This would tie models to serialization formats which goes against our design principles. Microsoft Graph serializes things to text/plain, application/json, application/x-www-form-urlencoded and a few other content types depending on which endpoint and OData convention we're talking about. So our design has to account for that. The fact that our design didn't account for it in previous generation SDKs (dotnet, java...) was causing serious limitations. In additions, kiota, the generator this SDK is based on, might be used for APIs serializing to XML, or other formats.
Another drawback of this approach would be the increase in SDK size for something that is effectively a cross cutting concern.

A single set of static functions to handle marshalling generically for all models is a good approach, I wasn't suggesting it was brittle or anything. I think the confusion came from the previous point. What we could do is have those functions in the JSON package itself, or better yet, would you be willing to submit a pull request to add them there?

[eric-millin]

to clarify my points:

We are not going to add marshalling functions to each model. This would tie models to serialization formats which goes against our design principles. Microsoft Graph serializes things to text/plain, application/json, application/x-www-form-urlencoded and a few other content types depending on which endpoint and OData convention we're talking about. So our design has to account for that. The fact that our design didn't account for it in previous generation SDKs (dotnet, java...) was causing serious limitations. In additions, kiota, the generator this SDK is based on, might be used for APIs serializing to XML, or other formats. Another drawback of this approach would be the increase in SDK size for something that is effectively a cross cutting concern.

Gotcha, thanks.

A single set of static functions to handle marshalling generically for all models is a good approach, I wasn't suggesting it was brittle or anything. I think the confusion came from the previous point. What we could do is have those functions in the JSON package itself, or better yet, would you be willing to submit a pull request to add them there?

I'd be happy to. Is the generic function I shared above + tests sufficient or do you have something else in mind?

[baywet]

no that looks great as it is, only the doc comments are missing (and the tests). Thanks for the contribution!

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[baywet]

related microsoft/kiota-serialization-json-go#71