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

GraphClientsDesign.md #570

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

nikithauc
Copy link
Contributor

No description provided.

baywet
baywet previously approved these changes Jan 18, 2022
}
```

- To acheive the above design we will need to customize the auto-generated `GraphServiceClient`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could achieve that by adding the API method to a base request adapter in the core repo. This way your code doesn't need to be duplicated between v1 and beta

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean exactly? How extensible is the base request adapter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't exist yet, but this is how I handled it in Go. kiota defines an http request adapter. Go core a BaseGraphRequestAdapter, which adds a bit of customization for graph on top of kiota. In this case the customization could be the API method to allow execution of arbitrary requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following are reasons to use an inheritance:

  • Tasks constructors such as PageIterator and LargeFileUpload tasks should accept both GraphServiceClient and GraphCoreClient
// both should work
const pageIterator = PageIterator(GraphServiceClient, options);
or
const pageIterator = PageIterator(GraphCoreClient, options);
  • Using inheritance will help in extending features from core to service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point being that the page iterator doesn't need the client but just the request adapter to be able to execute requests.
And today we don't have a mechanism in Kiota to make the client inherit from something.
You can have a look at the implementation of Page Iterator for Go microsoftgraph/msgraph-sdk-go-core#27

You could use the RequestAdapter property on the client, like the dotnet implementation, but I believe this will change to use the request adapter straight away because of the inheritance limitation.
https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/5ad51c9fca2e4f28a5dd68bdc4e528d7abb0daab/src/Microsoft.Graph.Core/Tasks/PageIterator.cs#L154

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baywet, what would look like a call using the request adapter. Do we really want the complexity to land of the developer? Is that even complex?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const pageIterator = PageIterator(GraphServiceClient.requestAdapter, options);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The discussion around bringing your own client emphasized that a user should be able to access the core features and the service library with a same client instance. This leads to the option for inheritance so that we can couple the core client and the service client.

  2. it doesn't exist yet, but this is how I handled it in Go. kiota defines an http request adapter. Go core a BaseGraphRequestAdapter, which adds a bit of customization for graph on top of kiota. In this case the customization could be the API method to allow execution of arbitrary requests.

Even with the BaseGraphRequestAdapter we would need to customize the GraphServiceClient by adding a api() public method GraphServiceClient so that the user do something like GraphServiceClient.api();

  1. I am also thinking whether we should be abstracting the RequestAdapter and the Kiota surfaces from the user in an example like const pageIterator = PageIterator(GraphServiceClient.requestAdapter, options); @sebastienlevert I think we discussed about the abstraction of Kiota libraries in the Graph layers. What do you think about this?

  2. const pageIterator = PageIterator(GraphServiceClient.requestAdapter, options); would be similar to const pageIterator = PageIterator(GraphCoreClient.api(), options);`

I can see the value of a extending the GraphRequestAdapter in the Graph SDKs which will allow extensibility and easy customization. This should be plugged in optimally.

zengin
zengin previously approved these changes Jan 18, 2022
design/GraphClientsDesign.md Show resolved Hide resolved
@nikithauc nikithauc dismissed stale reviews from zengin and baywet via 612d9b2 January 19, 2022 03:45
Copy link

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's outside of the scope of this change but having some explanation of the difference between service and core libs, why we need them both and if they're both relevant to developers working with our SDKs (and if so when) would help everyone who's not familiar with this architecture.

@nikithauc
Copy link
Contributor Author

Not sure if it's outside of the scope of this change but having some explanation of the difference between service and core libs, why we need them both and if they're both relevant to developers working with our SDKs (and if so when) would help everyone who's not familiar with this architecture.

@waldekmastykarz #558 should contain some of the answers.

The service libraries are the fluent libraries which contain the generated code. For example, the msgraph-sdk-dotnet today. The service libraries wrap the core libraries (like msgraph-sdk-dotnet wraps the msgraph-sdk-dotnet-core).

The core library contain the logic to make the http calls, middlewares, authentication, tasks such as page iteration and logic.

The current Graph JS SDK library which contains the client.api() call. For users who opt out of the fluent style can use the core library alone.

@waldekmastykarz
Copy link

Thank you for the additional information @nikithauc. My understanding was the the service library also exposes core library functionality. If that's the case, couldn't we just ship the service library to avoid providing additional choices and guidance around when developers should use which library and instead just have one resource to point them to?

@baywet
Copy link
Member

baywet commented Jan 24, 2022

@waldekmastykarz we're on a model of service libraries depending on one core library to avoid duplication. Technically there are 5 clouds x 2 version = 10 different APIs with more or less "endpoints". https://docs.microsoft.com/en-us/graph/deployments
People using any service library will get all the functionality offered in core as core will be a dependency.
Don't hesitate to hesitate to review our sdk design principles to get a better sense of those decisions.

@sebastienlevert
Copy link
Contributor

@waldekmastykarz question stays valid though. When to use core vs. when to use service library when you are looking at "only" the .api() approach to call the API.

This is how I think about it (in the world of JavaScript or even in larger discussions about SDKs).

  • Core is for users that want to leverage the underlying capabilities (middlewares, authentication, tasks, etc.) and don't need the fluent API, nor want to update their packages with every new release of the service library (which would be weekly). Only advanced scenarios would require this.
  • Service library is a library that composes Core and offers the fluent API. This is the library we want most of our users to use to help with endpoint discoverability but also to offer the best developer experience possible.

Documentation, guidance, and patterns will all be built with the service library as it will uncover all the value from Core and more.

@waldekmastykarz
Copy link

Core is for users that want to leverage the underlying capabilities (middlewares, authentication, tasks, etc.)

@sebastienlevert does that mean that you won't be able to use middleware with the service library?

nor want to update their packages with every new release of the service library

I think we need to be careful how we frame it because there's a risk people will read it as: if I don't update my library each week, my app will break, which isn't the case. You only need to update if you want to get access to the latest APIs introduced in Graph. On top, it's a good pattern to keep dependencies up-to-date to benefit from latest fixes and improvements and that's a general rule rather than something specific to Graph SDKs.

If we want developers to use the service library with fluent APIs, then we should lead with it, underline its benefits and ensure that all code samples that we offer, from docs to Graph Explorer, show its usage.

@sebastienlevert
Copy link
Contributor

@waldekmastykarz the service library has all the features of the core. So you will absolutely be able to add, modify and play with the middlewares. So as you say, we should lead with the service library, have all samples built with it and we should almost not discuss about the core. The service library becomes the entry point for Graph (like it is for any other language, to be honest). The only difference here is that JavaScript never had a concept of service library so people were effectively using "core" in the past. We have to be careful with the messaging and be clear about the USP.

@waldekmastykarz
Copy link

Understood. I wonder if we should use the terms Service- and Core library publicly, or if we should just call it a Graph SDK where the default experience is the fluent API but you can fallback to raw REST calls if need be.

@sebastienlevert
Copy link
Contributor

Agree with your last statement. It would be called the Graph SDK. Nothing specific to "Service". Same we do with other languages.

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

Successfully merging this pull request may close these issues.

5 participants