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

How can we handle untyped JSON? #2319

Closed
darrelmiller opened this issue Feb 19, 2023 · 24 comments · Fixed by #4095
Closed

How can we handle untyped JSON? #2319

darrelmiller opened this issue Feb 19, 2023 · 24 comments · Fixed by #4095
Assignees
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. WIP
Milestone

Comments

@darrelmiller
Copy link
Member

Currently in the OpenAPI for Graph we have this which I'm fairly sure is wrong. I don't think the @odata.Type comes back with graph.JSON payloads

    microsoft.graph.Json:
      title: Json
      required:
        - '@odata.type'
      type: object
      properties:
        '@odata.type':
          type: string
          default: '#microsoft.graph.Json'

and uses of it look like this.

            contentInfo:
              anyOf:
                - $ref: '#/components/schemas/microsoft.graph.Json'
                - type: object
                  nullable: true

I tried something similar to this in a different API and was expecting to see other properties appearing in the AdditionalData collection but there was nothing.

I would expect

            someThing:
              type: object
              properties:
                someObject:
                  type: object
                someArray:
                  type: Array

to project a type like

class SomeThing {
  someObject: JsonObject;
  someArray: JsonArray;
}

However, to do that, the serializer would have to expose via some kind of interface that would communicate what types to use for generic JSON content.

@baywet
Copy link
Member

baywet commented Feb 19, 2023

Tying the generation result to any specific format would break one of our tenets.

However we could use the a default entity class added to the abstractions with no properties so all the undocumented properties would go to the additional data.

For the array it's a bit more complicated since it could be an array of objects or scalar values.

And we need to think about writing back too.

Overall I thought the last time we talked about the we decided not to facilitate those scenarios since that would make kiota wrap around the serialization librairies with little control over what's going on. Falling early encourages API producers to fix their description.

@baywet baywet added enhancement New feature or request generator Issues or improvements relater to generation capabilities. Needs: Author Feedback and removed Needs: Triage 🔍 labels Feb 19, 2023
@baywet baywet added this to the Kiota post-GA milestone Feb 19, 2023
@darrelmiller
Copy link
Member Author

darrelmiller commented Feb 21, 2023

I wonder if we could fallback to a memorystream/byte array for properties that we don't know how to deserialize? That would at least allow the consumer to manually deserialize the properties.

This isn't just about "fixing" the API description. There are parts of APIs that are declared as "untyped" content. Microsoft Graph does this in a number of places where content is simply described as JSON.

The problem with relying on AdditionalProperties is that it only supports primitives in a reasonable way. Where additional properties are complex types like objects and arrays, we expose it as JSON strings. At least that's how we used to do it. I'm assuming Kiota does it the same.

@andrueastman
Copy link
Member

Where additional properties are complex types like objects and arrays, we expose it as JSON strings. At least that's how we used to do it. I'm assuming Kiota does it the same.

At the moment the serialization library exposes these types as object representation of the json Object depending on the serialization library. As the json library uses System.Text.Json these are exposed as instance of JsonElement. If someone build a json lib with NewtonSoft it would probable a JObject instance.

https://github.com/microsoft/kiota-serialization-json-dotnet/blob/3a5f1f71fa082cb882ba73cb6060ef3719e744d4/src/JsonParseNode.cs#L354

I think in this scenario,

            someThing:
              type: object
              properties:
                someObject:
                  type: object
                someArray:
                  type: Array

could project to generic object types like

class SomeThing {
  someObject: object;
  someArray: object;
}

And then the serialization libraries could still assign the appropriate type to the objects such as JsonElement for System.Text.Json.
Consumers would however, need to be aware that they need to cast the object to the type used by the serialization library and this way the generator would be agnostic of the serialization construct while the serialilizer still does its job?

@baywet
Copy link
Member

baywet commented Feb 21, 2023

The value of supporting arbitrary JSON decreases a great deal when more advanced concepts like union/intersection types are supported. (our previous generation of SDKs didn't have support for that)
With that in mind I believe the only scenario for arbitrary JSON is when the API producer doesn't own/know about the payload. Like the 3rd party webpart configuration objects in SharePoint pages APIs. But again, this is poor API design since they ask for a schema when building the webpart to begin with. It's just that we don't have infrastructure in place to vary the API schema by tenant like dynamics does and didn't have the infrastructure to generate clients by tenant (which we now have thanks to kiota).

I'm just worried that we're trying to enable scenarios that will produce terrible client code instead of adding incentive for client consumers to talk to the API producers into improving their design.

Right now I believe most languages deserialize scalar properties into the additional data and anything else is stored as IParseNode instance. But we're probably not symmetric in the serialization.

To Andrew's point: using a generic object that people have to cast will make for an error prone client code, I'd much rather just generate with an IParseNode type for those unknown properties. And we'd probably need to augment that interface with a kind (object/array/scalar) to enable symmetric serialization.

@microsoft-github-policy-service

This comment was marked as outdated.

@andrueastman
Copy link
Member

Re-opening this as this is needed for API's using the microsft.graph.Json class which are currently generated incorrectly.

@microsoft-github-policy-service

This comment was marked as outdated.

2 similar comments
@microsoft-github-policy-service

This comment was marked as outdated.

@microsoft-github-policy-service

This comment was marked as outdated.

@baywet baywet added this to the Kiota v1.10 milestone Nov 28, 2023
@baywet baywet modified the milestones: Kiota v1.10, Kiota v1.11 Jan 11, 2024
@MartinM85
Copy link
Contributor

Is there any way how the community can help with this? I guess it requires changes in more repositories, at least some subtasks which don't require a deep knowledge of Kiota before the main implementation can be done.

@baywet
Copy link
Member

baywet commented Jan 12, 2024

@MartinM85 thanks for offering your help.
I'm guessing the first baby steps we could take here are:

  1. introduce the types suggested by Darrel in kiota abstractions dotnet
  2. add a test json payload in kiota serialization json that'd contain: a object with known properties and unknown properties to cover all cases (known as in "part of the parsable model definition")
  3. define unit test cases to implement symmetric parsing/serialization (and the other way around) of that payload
  4. implement the code in JsonParseNode and JsonSerializationWriter so we get to the expected result.

Once we have a working prototype in dotnet, we can then think about replicating the efforts across other languages.

Thoughts? don't hesitate if you have follow up questions

@MartinM85
Copy link
Contributor

Hi @baywet, it's quite clear. I will have probably have more questions later.

@LockTar
Copy link

LockTar commented Apr 3, 2024

@baywet I see that this has a milestone for v1.13. I'm using v1.12. If I use the latest version of all the related Nuget packages, we get issues with microsoft/kiota-dotnet#175.

When we downgrade the following packages to the specific versions (almost latest) everything is working fine with v1.12.

Microsoft.Kiota.Abstractions Version=1.8.1 -> 1.7.12
Microsoft.Kiota.Serialization.Json Version = 1.2.0 -> 1.1.8
Microsoft.Kiota.Http.HttpClientLibrary Version = 1.3.8 -> 1.3.7

So what will v1.13 change? What could be the reason?
In v1.12 (with downgraded packages), AdditionalData is a Dictionary<string, object>.
In v1.12 with the latest version of the packages this is at runtime UntypedArray.

@sebastienlevert
Copy link
Contributor

The generated code would change with untyped JSON @LockTar and 1.13 was shipped this morning. Can you validate this solves the issue now?

@LockTar
Copy link

LockTar commented Apr 9, 2024

Hi,
If the generated code is changed in v1.13 then I believe it will going to work. At the moment we don't have time to update but if you say is true then indeed my issue will go away, Of course I need to change the code that is using the generated code. But that's ok.

If I update in the future and it doesn't work I will create a new issue. For now my comment can be discarded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants