Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsupported Data Type with Class Pointers #28

Closed
dadams39 opened this issue Jun 21, 2022 · 24 comments
Closed

Unsupported Data Type with Class Pointers #28

dadams39 opened this issue Jun 21, 2022 · 24 comments
Assignees

Comments

@dadams39
Copy link
Contributor

dadams39 commented Jun 21, 2022

Upon writing M365 data with JsonSerializationWriter.WriteObjectValue()
Receiving error:

"unsupported AdditionalData type: map[string]*jsonserialization.JsonParseNode"}

It should be noted that when the messageId is pulled from Graph Explorer the Writer is able to parse it correctly. However, I have seen this failure on multiple types of mail. Two different additional data headers are included below:

Number 1:

map[string]interface {} ["@odata.type": *"#microsoft.graph.eventMessageResponse", "isDelegated": *false, 
"endDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002a5460), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002a54e0), ], 
"type": *"singleInstance", "responseType": *"accepted", "@odata.etag": *"W/\"DAAAABYAAAB8wYc0thTTTYl3RpEYIUq+AAAfar4a\"", "isOutOfDate": *false, "isAllDay": *false,
 "meetingMessageType": *"meetingAccepted",
 "startDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002a5330), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002a53c0), ], ]

Number 2:

map[string]interface {} ["type": *"singleInstance", "isDelegated": *false, "location": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["displayName": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e5720), "locationType": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e5780), "uniqueIdType": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e57e0), ], "startDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e5540), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e55a0), ], "endDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e5630), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002e5690), ], "meetingMessageType": *"meetingRequest", "meetingRequestType": *"newMeetingRequest", "@odata.type": *"#microsoft.graph.eventMessageRequest", "@odata.etag": *"W/\"CwAAABYAAADSEBNbUIB9RL6ePDeF3FIYAAAAAAsl\"", "isOutOfDate": *false, "responseRequested": *true, "isAllDay": *false, ]
@dadams39
Copy link
Contributor Author

dadams39 commented Jun 21, 2022

Troubleshooting updates

If the pointer for the ...kiota-serialization-json-go.JsonParseNode is changed to base types, we get the following test

func TestAdditionalDataMapPointers(t *testing.T) {
	serializer := NewJsonSerializationWriter()
	data := "a string"
	aNum := 2
	aBool := false
	var ifce error
	adlData := map[string]interface{}{
		"type":        JsonParseNode{},
		"isDelegated": true,
		"location": map[string]*string{"displayName": (*string)(&data),
			"locationType": (*string)(&data),
			"uniqueIdType": (*string)(&data)},
		"startDateTime": map[string]*int{"dateTime": (*int)(&aNum),
			"timeZone": (*int)(&aNum)},
		"endDateTime": map[string]int{"dateTime": *(*int)(&aNum),
			"timeZone": *(*int)(&aNum)},
		"meetingMessageType": &data,
		"meetingRequestType": &aNum,
		"@odata.type":        NewJsonSerializationWriterFactory(),
		"@odata.etag":        "W/\"CwAAABYAAADSEBNbUIB9RL6ePDeF3FIYAAAAAAsl\"",
		"isOutOfDate":        &aBool,
		"responseRequested":  aBool,
		"isAllDay":           ifce,
	}
	err := serializer.WriteAdditionalData(adlData)
	t.Logf("Error: %v\n", err)
}

The current version passes the above test case.

@baywet baywet self-assigned this Jun 21, 2022
@baywet
Copy link
Member

baywet commented Jun 21, 2022

Hi @dadams39,
Thanks for reaching out. So far this is expected behavior since no case is implemented for map values in the method.

What I don't understand is why store the location information as an additional data entry when the event model has a location field ?

@dadams39
Copy link
Contributor Author

The original query to msgraph is: client.UsersById(user).MailFoldersById(aFolder).Messages().Get()
For some reason, eventMessages or "@odata.type": *"#microsoft.graph.eventMessageRequest" are translated with these placed into the additional data slots.

@dadams39
Copy link
Contributor Author

I'm going to attempt to add in that functionality. I will include the above test in. If you think of additional tests that you feel are necessary, please notify me here.

@baywet
Copy link
Member

baywet commented Jun 22, 2022

thanks for the additional information. For the odata type, it is expected since neither message/event, outlook item (parent) or entity (grand parent) have a defined property for it. For location, it is not expected since the model has a field for it.

@dadams39
Copy link
Contributor Author

We don't seem to have a handler for *jsonserialization.JsonParseNode in the writer.

Any suggestions?

dadams39 added a commit to dadams39/kiota-serialization-json-go that referenced this issue Jun 22, 2022
- Initial look at adding maps to writer for primative types
@baywet
Copy link
Member

baywet commented Jun 22, 2022

I'd rather fix the deserialization process instead to make sure we don't get into a situation where we have a JsonParseNode in the deserialized model.

@dadams39
Copy link
Contributor Author

That's fair. How would we go about that?

@dadams39
Copy link
Contributor Author

PR #29 submitted to add the functionality for mapping. Unfortunately, I cannot tell if this fix alone will take care of the errors I was getting when I started. When I tried to checkout my branch, I received the following error:

imports                                                                        
        github.com/dadams39/kiota-serialization-json-go: github.com/dadams39/[email protected]: parsing go.mod:
        module declares its path as: github.com/microsoft/kiota-serialization-json-go                                           
                but was required as: github.com/dadams39/kiota-serialization-json-go   

@dadams39
Copy link
Contributor Author

@baywet, do we have any leads on the deserialization change that needs to happen?

  • Would you like to make a different issue?

@baywet
Copy link
Member

baywet commented Jun 27, 2022

Hi @dadams39
Thanks for your patience on this, we still have a bunch of people out for various reasons and the workloads has increased significantly for those of us still in.
I believe you were able to narrow down and create a new issue for the root cause via #30, correct?
In which case I'll hold for the fix of 30 before processing #29

@dadams39
Copy link
Contributor Author

The root cause for this error is the incorrect serialization of odata.type --> #microsoft.graph.eventMessageResponse
I'll have to find some of these in my main testing account to be able to verify.

Strategy:

  • To pull data from my test account.
  • Serialize the data into (object, []byte) pairs
  • Then I will parse them. I will try to hydrate it with the parseNode.
  • Once I get a few test cases, I will try to verify where the serialization writer went wrong if it did at all.
  • Stretch goal for later should be that the writer shouldn't create objects that are invalid JSON.

@dadams39
Copy link
Contributor Author

@baywet PR #29 is a feature update as the mapping that I troubleshot here is the byproduct of eventMessageResponses being incorrectly serialized as messages. The failure of #28 is a separate issue which I have given steps for troubleshooting. This should present insight on #30, but I won't know until I get deeper into the results.

@dadams39
Copy link
Contributor Author

dadams39 commented Jun 28, 2022

Verified the #microsoft.graph.eventMessageResponse in GraphExplorer is valid JSON. The object is able to be hydrating without error using ParseNode functions. SerializationWriter causes an error. Will update this thread when I have a viable solution.

Will start troubleshooting from step 1.

@dadams39
Copy link
Contributor Author

dadams39 commented Jun 28, 2022

Line 245:

  err := item.Serialize(w) --> err := m.OutlookItem.Serialize(writer) // Line 686

Fails on AdditionalData

map[string]interface {} ["@odata.context": *"https://graph.microsoft.com/v1.0/$metadata#users('f435c656-f8b2-4d71-93c3-6e092f52a167')/messages/$entity", "type": *"singleInstance", "endDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x14000226ed0), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x14000226f30), ], "isDelegated": *false, "meetingMessageType": *"meetingAccepted", "responseType": *"accepted", "@odata.type": *"#microsoft.graph.eventMessageResponse", "@odata.etag": *"W/\"DAAAABYAAAB8wYc0thTTTYl3RpEYIUq+AAAfar4a\"", "isAllDay": *false, "startDateTime": map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x14000226de0), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x14000226e40), ], "isOutOfDate": *false, ]

@dadams39
Copy link
Contributor Author

The proposed fix is a temporary fix. Objects of

 "@odata.type": "#microsoft.graph.eventMessageRequest",

Serialize with the additonalData keys:

"startDateTime": , 
"isDelegated":,
"responseRequested":,
"@odata.type": 
"meetingMessageType": 
"@odata.etag":,
"endDateTime":
"type": 
"@odata.context": 
"meetingRequestType": 
"isOutOfDate":
"isAllDay":

startDateTime and endDateTime are populated with the following:

 map[string]*github.com/microsoft/kiota-serialization-json-go.JsonParseNode ["dateTime": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x140002088d0), "timeZone": *(*"github.com/microsoft/kiota-serialization-json-go.JsonParseNode")(0x14000208930), ]

The change I proposed introduces a case map[string]*JsonParseNode that unpacks the embedded key/value pairs. The subsequent tests are going to be in the following PR. I have not seen messages concerning Issue #30.

dadams39 added a commit to dadams39/kiota-serialization-json-go that referenced this issue Jun 28, 2022
Changes ensure that the collections are properly deliminated and map support for @odata.type --> microsoft.graph.eventMessageRequest
dadams39 added a commit to dadams39/kiota-serialization-json-go that referenced this issue Jun 28, 2022
Test case extended to ensure support for microsoft.graph.eventMessageResponse
@baywet
Copy link
Member

baywet commented Jun 28, 2022

Thanks for digging into this, let's take a step back for a second.

On this list of properties

"startDateTime": , 
"isDelegated":,
"responseRequested":,
"@odata.type": 
"meetingMessageType": 
"@odata.etag":,
"endDateTime":
"type": 
"@odata.context": 
"meetingRequestType": 
"isOutOfDate":
"isAllDay":

Only these should be in the additional data

"@odata.type": 
"@odata.etag":,
"@odata.context": 

Because the SDK has properties defined also here

All the properties that are expected to end up as additional properties are of type string, and there's already a case to handle it.
So my argument is that, from the case you're describing, we shouldn't have to change anything in the code handling additional properties just yet.

Now, looking further in that direction, the reason why we find these properties in additional data instead of their fields, is because you're getting a message object instead of an event message request object.

This is because the the discriminator method doesn't contain an entry for eventRequestMessage.

These entries are dictated by the conversion library before the SDK is being generated, and we already have an issue for this.

The next steps are to:

  1. fix the issue in the conversion library
  2. release the conversion library
  3. update the conversion library in hidi, our conversion tool
  4. release hidi
  5. trigger a new generation of the Go SDK
  6. release the Go SDK.

As I eluded in other issues, our team has a number of people out for various reasons, and we've just uncovered a number of conversion issues (including the one impacting us here), so this is likely to take a couple of weeks to be fixed end to end. Thank you for your patience.

Regarding your PR, I do believe some of the changes are valuable including:

  • deduplicating some cases
  • using a switch for code clarity

I however don't believe adding cases for map[string]something is valuable at this point for the reasons I've explained above and to avoid deviating too much from other languages. If you're willing to clean it up, we'll be more than happy to merge it.

I hope this lengthy post clarifies the situation and provides an explanation to the root cause. Don't hesitate if you have questions/comments, and thank you so much for your extended participation in the Go SDK.

@dadams39
Copy link
Contributor Author

This issue relates to and the issue microsoft/kiota#648. Previously event docs stated that it was possible to receive messages from the Teams application during this call. This was a condition in legacy (or perhaps it lingers even now), but it points to a common problem that the serialization library will have to face. As new services and objects become available, additionalData may have to hold some of these properties until the supporting discriminator functions can be fine tuned.

The issue was created because a query for mail returns several different types of mail, including EventMessage which falls into the space of being "newly" created in the last few versions.

// EventMessageable 
type EventMessageable interface {
    Messageable
    i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.Parsable
    GetEndDateTime()(DateTimeTimeZoneable)
    GetEvent()(Eventable)
    ...

at a glance, it is likely we will run into this again as the functionality expands. I understand the lack of resources within the team, and I will do what I can to help. I do need to get this issue resolved though.

My next steps are:

  • remove mapping from the PR.
  • Await an update on eventMessages

@baywet
Copy link
Member

baywet commented Jun 28, 2022

Thanks for your understanding. The addition of new derived types and SDKs running in production "behind" is a topic we've talked a lot about both in our API design guidance sessions and later in our Kiota design work. This a big reason why we have additional data and why that additional data supports serializing "generic" JsonParseNode so gaps in the schemas/api descriptions can be bridged without breaking applications.
I don't think adding support for maps in there is that useful since a map can be represented by a generic JsonParseNode (as a scalar property, there are effectively the same in JSON).

dadams39 added a commit to dadams39/kiota-serialization-json-go that referenced this issue Jun 28, 2022
As per the agreement within the Issue, mapping capability has been
removed until further notice. Test suites adjusted accordingly.
@dadams39
Copy link
Contributor Author

dadams39 commented Jul 8, 2022

A question concerning this. Currently, one can only call client.UserById(user).Messages.Get() there is no option for getting requesting an eventMessage or for transforming a message into an eventMessage in order to not have the problems that we see above. Is there any talk about increasing the support for these data types or creating any interfaces in this library?

@baywet
Copy link
Member

baywet commented Jul 8, 2022

This is an interesting question, which could be interpreted a couple of different ways.

"Is there a way to get only eventMessages?"
From an OData perspective, yes, by appending an OData type cast segment to the request. Where the original request would be something like /me/messages, the type cast one would be /me/messages/microsoft.graph.eventMessage and return only the event messages. However I've just tried it, and it doesn't seem to be working at the service level.
It'd make a good feature request if one doesn't already exist.

"Could I get eventMessages deserialized to the right eventMessage struct instead of the base message type?"
OData mandates to reply with and odata type annotation to tell client "this is in fact derived type X". which the service returns. (see the example payload)
Right now the SDK has support for it even though we're missing some information from the conversion library (working with the team to get all the right information translated).
https://github.com/microsoftgraph/msgraph-sdk-go/blob/eb2d097f9010a618832461f649740084d7823b02/models/message.go#L97

{
            "@odata.type": "#microsoft.graph.eventMessageRequest",
            "@odata.etag": "W/\"CwAAABYAAAAs+XSiyjZdS4Rhtwk0v1pGAADoHTot\"",
            "id": "AAMkAGNmMGZiNjM5LTZmMDgtNGU2OS1iYmUwLWYwZDc4M2ZkOGY1ZQBGAAAAAAAK20ulGawAT7z-yx90ohp-BwAs_XSiyjZdS4Rhtwk0v1pGAAAAAAEJAAAs_XSiyjZdS4Rhtwk0v1pGAADoYRf4AAA=",
            "subject": "very important meeting"
        }

After querying the service you can type assert the object, with something like this

if eventMessage, isEventMessage := messageInstance.(models.EventMessageable); isEventMessage {
    eventMessage.GetMeetingMessageType() // this property is specific to the derived type and wouldn't be accessible without the assertion
}

@dadams39
Copy link
Contributor Author

dadams39 commented Jul 8, 2022

Could I get eventMessages deserialized to the right eventMessage struct instead of the base message type?"
OData mandates to reply with and odata type annotation to tell client "this is in fact derived type X". which the service returns. (see the example payload)
Right now the SDK has support for it even though we're missing some information from the conversion library (working with the team to get all the right information translated).

Thanks for the thorough reply. I did see: https://github.com/microsoftgraph/msgraph-sdk-go/blob/eb2d097f9010a618832461f649740084d7823b02/models/message.go#L97. However, this requires a parsenode and the response return value is that of a messageable object. There does not seem to be a way to send it back through the system to get the right type. I did try option number one of requesting an eventMessage by id to get it back in the correct form and that also was not supported.

The interesting thing is that there is an eventMessageRequest and eventMessageResponse and these derived types will both load information into the additional data struct when converted to a messageable object. The interesting thing is if one creates either of these structs, one cannot pass along the valid additional data from the original. I'm curious as to what implications this will have for this library in particular. From an internal standpoint, it would be a nice feature to be able to set additional data in new objects.

@baywet
Copy link
Member

baywet commented Jul 8, 2022

event message request & response: this is where we're missing some information and we're working in the conversion library to get this fixed. Message should contain entries for event response and event request so you get the right type all the way. microsoft/OpenAPI.NET.OData#240

Allowing to set things in additional data from a new model: we do have some support for that for scalar values as you already know. One of the reasons why we're not investing in more advanced scenarios in that space is because we'd rather have better models generation and downcast support for people to use them and get better value. Another reason is if we start putting complex types that don't implement parsable is this library won't know what to do with them.

@baywet
Copy link
Member

baywet commented Sep 8, 2022

Closing since this has been addressed in the conversion library and the SDK now has all the mapping information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants