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

Builder should support application-specific JSON MIME specifiers #1197

Closed
Tracked by #25 ...
hermann-noll opened this issue Feb 14, 2022 · 5 comments · Fixed by #1461
Closed
Tracked by #25 ...

Builder should support application-specific JSON MIME specifiers #1197

hermann-noll opened this issue Feb 14, 2022 · 5 comments · Fixed by #1461
Assignees
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience

Comments

@hermann-noll
Copy link

It seems like currently only application/json is supported as MIME specifier. Instead the builder should also accept specifiers with the pattern application/myapp+json as is the case for e.g. GitHub (application/vnd.github.mercy-preview+json)

Additionally when using an OpenAPI document like below the kiota builder throws a NullReferenceException instead of a diagnostic error.

swagger: '2.0'
info:
  version: 1.0.0
  title: Sample
  description: ''
host: hostname
schemes:
  - https
paths:
  /something:
    get:
      description: ''
      produces:
        - application/myapp+json
      responses:
        '400':
          description: Indicates failure.
          schema:
            type: string
@baywet baywet self-assigned this Feb 14, 2022
@baywet baywet added type:bug A broken experience help wanted Issue caused by core project dependency modules or library generator Issues or improvements relater to generation capabilities. labels Feb 14, 2022
@baywet
Copy link
Member

baywet commented Feb 14, 2022

Hi @hermann-noll
Thanks for trying kiota and thanks for reporting this issue.

There are multiple aspects to this issue, here is the detailed explanation:

While the generator aims to support multiple content types for the payload (request/response), it's currently hard-coded to only look for the application/json and application/octect-stream content types.

.Where(c => c.Key == "application/json")

private const string RequestBodyBinaryContentType = "application/octet-stream";

This aspect dates from the POC era of Kiota and we have multiple issues logged to unlock additional content types/scenarios

Once the generation goes through, for the client to work properly, it needs registered ParseNode factories and SerializationWriter factories to handle the payloads correctly. They are also locked to a very specific content type

public string ValidContentType { get; } = "application/json";

public string ValidContentType { get; } = "application/json";

In theory, the reason why the API returns to your request a vendor specific mime type is because your application should be a different parser than "just JSON" to parse the response. It is not always the case in practice and parsing with the default implementations we provide might work just fine.
In any case, I believe that as the very least a new set of libraries should be introduced for that vendor type so even if they only inherit from the default to override the mime type, they can evolve separately should the need arise in the future.

Does all of that make sense?

In which case here is what I suggest should be done:

  • update the code in the kiota generator to use a regex instead of hard-coded constant, during the generation process
  • standup a new repo for those vendor specific JSON libs

@darrelmiller
Copy link
Member

@baywet We can safely assume that any media type that ends in +json can be parsed with our standard JSON parsing. https://datatracker.ietf.org/doc/html/rfc6838#section-4.2.8

We should figure out what the model is for adding support for standard JSON based media types like application/problem+json and application/collection+json.

@baywet
Copy link
Member

baywet commented Feb 14, 2022

thanks for jumping in @darrelmiller. So effectively parsing a application/vnd+json response with our current implementations should also work because the body "is just JSON"?

@baywet baywet added fixed and removed help wanted Issue caused by core project dependency modules or library labels Mar 29, 2022
@baywet
Copy link
Member

baywet commented Mar 29, 2022

Hi @hermann-noll ,
Thanks for your patience on this one.
I've just authored #1461 that addresses this. I yet have to author PRs for dotnet and typescript as we moved those libraries to other repos.

generation change

Imagining we get the following content type in the description for the response content application/vnd.github.mercy-preview+json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8.

  1. ;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8 will be ignored (considered as application/vnd.github.mercy-preview+json
  2. vnd.github.mercy-preview+ will be ignored (considered application/json
  3. The described types will get generated

runtime changes

Imagining we get the following content type (runtime application/vnd.github.mercy-preview+json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8

  1. ;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8 will be ignored (considered as application/vnd.github.mercy-preview+json
  2. The registry will look for a serialization writer/parse node factory for application/vnd.github.mercy-preview+json, use that if it finds it.
  3. If it doesn't find it, it'll look for application/json instead.

@hermann-noll
Copy link
Author

Hello @baywet ,

Great to hear, I am looking forward to using these changes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants