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

Improved handling of generic media types #471

Closed
thompson-tomo opened this issue Nov 13, 2024 · 12 comments
Closed

Improved handling of generic media types #471

thompson-tomo opened this issue Nov 13, 2024 · 12 comments
Labels
Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question

Comments

@thompson-tomo
Copy link
Contributor

Within the openapi spec i am using (https://api.parkassist.com/docs/index.html) it is using generic media types for requests due to supporting both json & xml.

/signs:
    post:
      description: Creates signs. Any ids specified are ignored.
      requestBody:
        description: The signs to create.
        content:
          '*/*':
            schema:
              type: array
              items:
                $ref: '#/components/schemas/ParkAssist.PaseServer.WebServices2.Dto.Sign'
        required: false

The above is generating the following method

public async Task<List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign>?> PostAsync(Stream body, Action<RequestConfiguration<DefaultQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default)

which is valid, however if the full media type is specified in the openapi, the below method is generated which is easy for a user to use.

public async Task<List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign>?> PostAsync(List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign> body, Action<RequestConfiguration<DefaultQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default)

What would be helpful is if the media type is defined as one of the below:

  • star/star
  • application/star

where star = *

The below method is generated, and it uses application/json

public async Task<List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign>?> PostAsync(List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign> body, Action<RequestConfiguration<DefaultQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default)

And it also generates which would is expecting "application/json" and if not matched, results in a failure.

public async Task<List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign>?> PostAsync(List<global::Skidata.Dashboard.Client.ParkAssist.Models.Sign> body, string MediaType, Action<RequestConfiguration<DefaultQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default)

When #342 is implemented that also becomes a supported media type to be passed in.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Nov 13, 2024
@baywet
Copy link
Member

baywet commented Nov 14, 2024

Hi @thompson-tomo
Thank you for using kiota and for reaching out.

I'd say this description is not specific enough. From what it describes, it'd mean I could also send it an image, YAML, etc... Which I'm pretty sure the API would be unhappy with.
It'd be much better for this description to have two entries, one for application/json and another for application/xml

We don't have any plans to add any "compatibility" mechanism, allowing users to say "map star/star to application/json during the generation", it'd be a slippery slope.

@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Nov 14, 2024
@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question labels Nov 14, 2024
@michaeldcanady
Copy link

My use-case is different but is excluded by the lack of this functionality. I am working with an API where you can attach files/images/etc. to a request and in it’s documentation it says it supports / so by extensions when you call /attachment/{sys_id}/file it can return nearly any content type.

@michaeldcanady
Copy link

Here is the documentation for reference: https://www.servicenow.com/docs/bundle/xanadu-api-reference/page/integrate/inbound-rest/concept/c_AttachmentAPI.html

The section in question is: Attachment - GET /now/attachment/{sys_id}/file

@thompson-tomo
Copy link
Contributor Author

Agree @michaeldcanady use case is deeply connected to this.

For me what would be most beneficial is if not otherwise specified it is expected that the api will return the same content type as the request for instance posting json is expected to return json unless the api or developer specifies otherwise.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Nov 15, 2024
@baywet
Copy link
Member

baywet commented Nov 18, 2024

Thank you for the additional information.

Can you both expand on the use cases here please? I'm still struggling to understand why/what's needed here.
Thanks!

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Nov 18, 2024
@michaeldcanady
Copy link

michaeldcanady commented Nov 18, 2024

@baywet to expand on it further/restate - hopefully more clearly.

Service-Now has a API endpoint that returns the raw data of an uploaded file (can be any content type).

For example take an image. Since Kiota requires a parse node factory for all serialization, if I want to get the raw data of any image/ type I'll either need to registry the different nodes for every image/ type or, and ideally, register one for image/* which can serialize any image type.

And while I limited it to only images, a user could upload a JSON file, HTML file, etc. (any content type) and I need to be able to get the raw byte data - which as I understand it - you have to register a parse node factory for each content type.

The idea would be, the same as the ErrorMapping system, where it checks for an exact match first, then search for a fuzzy match, before defaulting to */*

@baywet
Copy link
Member

baywet commented Nov 18, 2024

Thank you for the additional information.

I think this might be where the disconnect is.

Parse node factories are only required for content... that needs to be parsed (e.g. JSON to a model, text to a scalar value, etc...)
These factories will be applied in order of preference for the media type:

  1. exact match of foo/vendor+bar (strips any additional parameter)
  2. exact match of foo/bar (strips the vendor if present)

see the implementation

For things that cannot be parsed/don't result in models, kiota will project a Stream in the fluent API, which allows you to access the response body directly.

Same logic applies for request bodies.

So now, either:

  • your content cannot/doesn't need to be parsed by the generated client, kiota projects a stream, nothing to do on your end.
  • your content is a specialized version of an already implemented media type (e.g. application/github+json), and you want to apply extra logic for that semantic, you need to implement IParseNode, and the factory, and register the vendor type. Kiota projects models.
  • your content is a specialized version of an already implemented media type (e.g. application/github+json), and you DON'T want to apply extra logic for that semantic. Nothing to do. Kiota projects models.
  • your content is another structured type that kiota doesn't have an implementation for (e.g. xml or yaml). You need to implement IParseNode, also during generation, you need to pass that extra media type to structured media type argument. Kiota will then generate models.

I hope that clarifies the confusion.

Let us know if you have any additional comments or questions.

@michaeldcanady
Copy link

Would you mind writing an example of how the content is streamed? Looking at the RequestAdapter.Send* methods, I'm not seeing a way to without a parse node factory.

@baywet
Copy link
Member

baywet commented Nov 19, 2024

As in the generated code?
Here is one

(download a user's profile picture)

To call it people write something like

using var photoStream = await client.Users["jane"].Photo.Content.GetAsync();
//do something with the stream

Let us know if you have any additional comments or questions.

@michaeldcanady
Copy link

Thank you for the example! I went to see how the implementation was done in the go variation (since that's what I've been working with) and I have a question:

  1. I noticed https://github.com/microsoftgraph/msgraph-sdk-go/blob/main/groups/item_photo_value_content_request_builder.go#L74 "SendPremitive" was being used instead of "SendPremitiveCollection" (the method I was trying to use), which I find confusing since normally for a collection of bytes it would be the later and not the former (based on what I've seen with the parsers), so why is that?

@baywet
Copy link
Member

baywet commented Nov 25, 2024

Although it might be confusing, the name of the method has to do with the return type, not the request body type.
Let us know if you have any additional comments or questions.

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.

@github-project-automation github-project-automation bot moved this from Waits for author 🔁 to Done ✔️ in Kiota Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question
Projects
Archived in project
Development

No branches or pull requests

3 participants