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

Implement Request Adapter #364

Closed
10 of 11 tasks
baywet opened this issue Jul 21, 2021 · 3 comments · Fixed by #1048
Closed
10 of 11 tasks

Implement Request Adapter #364

baywet opened this issue Jul 21, 2021 · 3 comments · Fixed by #1048
Assignees
Labels
enhancement New feature or request PHP

Comments

@baywet
Copy link
Member

baywet commented Jul 21, 2021

Looking at the sdk-design features list, there are a number of component that exist today in the graph core library which belong to the kiota http core library. Most of those components are the middlewares. If they exist, move the following components:

  • pipeline (if not provided by the native http client)
  • authorization handler
  • retry handler
  • compression handler
  • logging handler
  • chaos handler
  • redirect handler (provided by Guzzle)
  • (optional) telemetry handler (make it generic enough so the request
    changes can be specified by a callback)
  • multi-part content
  • Implement RequestAdapter interface
  • Implement KiotaClientFactory

AB#10329

@baywet baywet added enhancement New feature or request PHP labels Jul 21, 2021
@baywet baywet added this to the GA milestone Jul 21, 2021
@MIchaelMainer
Copy link
Member

To clarify, we don't want the same implementation from our PHP core for Microsoft Graph in Kiota, we want to use our implementation as a reference for adding a generic implementation in Kiota HTTP Core. This is my understanding.

Some of these will not be added to the default pipeline. Is it safe to say that the authorization, retry, and redirect handlers can be added to the default middleware pipeline?

Please check me.

@baywet
Copy link
Member Author

baywet commented Sep 13, 2021

You can look at #360 to get more context and experience feedback from the dotnet work on that aspect.

I don't think we need an auth middleware anymore given the current architecture for Kiota.

The goal is always to put all the non graph specific things in Kiota cores so it's available to others, and keep the graph specific things in Graph core, so we have a good feature set for graph users.
When possible and required, things should be made generic and extensible, such as the work that's been done on #609 for the telemetry handler.

In terms of the defaults, I'd argue that most if not all middlewares defined in kiota http core should be part of the defaults, unless you have a specific point?
Then, GraphCore, can always "rework the defaults" provided by kiota core (removing some if required, adding the graph specific ones).

Let me know if that answers most questions here, and don't hesitate to ask follow up questions.

@Ndiritu Ndiritu changed the title Move middleware and other generic components from graph core to the http core library Implement Http Core Nov 3, 2021
@Ndiritu Ndiritu changed the title Implement Http Core Implement Request Adapter Nov 4, 2021
@Ndiritu Ndiritu mentioned this issue Dec 6, 2021
1 task
@Ndiritu
Copy link
Contributor

Ndiritu commented Jan 17, 2022

Notes for future reference:

  • Guzzle provides a built-in Redirect Handler that meets our SDK design requirements including our security consideration on not sending Authorization tokens to a different host
  • Guzzle supports decompression by default by relying on curl to add Accept-Encoding headers & decompress request bodies. Decompression can be configured further using the decode_content Guzzle config option.
  • Guzzle provides built-in logging middleware (https://github.com/guzzle/guzzle/blob/master/src/Middleware.php#L192)
  • Guzzle's built in MockHandler handles "Manual mode" scenarios defined by Chaos Handler design spec. The PHP Chaos Handler will only handle "random mode" scenarios.

This was referenced Jan 17, 2022
@RabebOthmani RabebOthmani moved this to Todo in Kiota Jan 24, 2022
Repository owner moved this from Todo to Done in Kiota Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PHP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants