Naming are making things worse

MohamadAlallan-Pi commented 7 months ago

Hi,

MS Graph doesn't have enough documentation and the sdk naming is making things worse. I don't know what happened here, but it is the first time I see such a counter intuitive thing in Microsoft packages, this name is strange and not readable and not understood GetAllSites.GetAsGetAllSitesGetResponseAsync . I think it is missing one more GetAsGetAllAs... at the end

chadwackerman commented 7 months ago

This whole project is an embarrassment.

Dropbox has a client written by people who obviously don't know or care how .NET works, yet it's fine.

Meanwhile OneDrive crashes at runtime unless I cast something to a Microsoft.Graph.Drives.Item.Items.Item.DriveItemItemRequestBuilder.

andrueastman commented 7 months ago

Thanks for raising this @MohamadAlallan-Pi

The GetAsGetAllSitesGetResponseAsync naming is ideally built to be able to disambiguate response types that change based on the HTTP method being invoked as if the GetAllSites adds the response could be different from the generator work at https://github.com/microsoft/kiota/issues/2952

I think it is missing one more GetAsGetAllAs... at the end

Any chance you can confirm if there is a specific API you are trying to call that the SDK request builders are missing? https://learn.microsoft.com/en-us/graph/api/site-getallsites?view=graph-rest-1.0&tabs=csharp

chadwackerman commented 7 months ago

@andrueastman It's difficult to be constructive here because the whole Kiota thing is clearly something we'll all be looking back on in a few years and laugh about. "What the hell were we thinking?"

There are a few other Microsoft projects with similar problems. Polly comes to mind along with System.CommandLine. Like Graph, both are hellscapes where you press dot and nothing sensible happens. Want your HttpClient to retry once after a 3 second delay unless it's a 400 BadRequest or 401 Unauthorized? Good luck with that, sucker.

System.CommandLine has a "start the app" function with over 30+ overloads: funcs, tuples, actions. It's insane. They've taken the most obvious thing and put a poorly designed, undocumented abstraction over it.

Dropbox has "File." Graph has Drives.Item.Items.Item.DriveItemItemRequestBuilder which interacts atop... dunno, DrivesItemItems? DriveItemsItem?

I suppose the real question is: how much longer are you going to follow this obvious dead-end path?

Graph has a modestly complicated REST API and it deserves a proper client designed by people who actually want to make the technology available to developers. Instead you're off in the weeds dependent on some insane open source toolkit that generates nonsense words then throws it in the face of developers pretending it's "Fluent."

You've released 183 (!) versions of this monstrosity and it's only getting worse.

MohamadAlallan-Pi commented 7 months ago

@chadwackerman They make me feel we just discovered REST APIs

catmanjan commented 7 months ago

I agree with @chadwackerman, v4 Graph was hard enough to use and v5 seems even worse - can we get someone senior in this project to comment?

I would have thought this was very high profile considering it is the main entrypoint for dotnet developers into MS cloud, but right now it seems that someone DIY-ing a python client will have a better developer experience simply because the HTTP endpoints are documented and more intuitive than the DLLs!

baywet commented 7 months ago

Hi everyone 👋 Thank you for the feedback. Allow me to chime in here to provide additional context, and some work arounds

GetAsGetAllSitesGetResponseAsync

As Andrew mentioned, this was done to fix another issue while avoiding shipping a breaking change. We strive to avoid shipping breaking changes too often with our SDKs because they are disruptive to you. On the next major version of the SDK, this will be grouped with other breaking changes to be cleaned up and reverted back to GetAsync.

Apologies for the inconvenience here, it felt like the lesser impact here since: Microsoft Graph provides about 20 000 operations, about 500 or so of them use inline type definitions, fewer than 100 might have collided with one another. So we chose impacting 100 operations than 20 000 by major rev. Hopefully you can get behind our choice here.

If you can't wait for that next release to see the impact reduced, you can use kiota, the same code generator we use for this SDK, to generate your own. Make sure you pass the --exclude-backward-compatible argument so you don't get the obsolete method + the complex naming convention. Generating your own SDK provides additional advantages like being able to select only the APIs/operations you care about. If you have direct feedback about kiota itself, I encourage you to provide it directly on the dedicated repository.

other projects

Polly is not published by Microsoft, but by a third party. I encourage you to provide your feedback directly to them. System.CommandLine can be a bit complex at times, we use it for the Microsoft Graph CLI, However, it's owned by a different team, and they usually respond and adapt pretty well to feedback, I encourage you to provide yours directly on their repository.

Retrying automatically on requests

The Graph SDK (or a kiota client) already do that automatically for you on transient errors! (429, 503), no need to add anything else for those. It will also retry somewhat automatically (depending on the scenario) on a 401 with a www-authenticate header in the response (authentication challenge because of continuous access evaluation for example)

Drives.Item.Items.Item.DriveItemItemRequestBuilder

Let's unpack this one together:

First, an Item and a File are two different things on OneDrive/SharePoint/Microsoft Graph. The File represents the binary object, plus traditional metadata you'd regularly find in a local file system. The Item represents additional metadata that could have been customized by the customers (additional columns added in the list, etc...). I encourage you to read the documentation on the topic

Then, the Item (singular) segments here is a convention from Kiota because we're indexing into a collection. I get that it can be confusing with the domain term "Items" (the name for the collection) coming from the API itself, but again this confusion will appear only for a couple hundred operations from over 20 000. We tried to find the best term for that convention, and this one ended up being selected.

Regardless, you should never have to deal with with the namespace of a request builder in dotnet as:

you access the operation from the fluent API, here client.Drives["id"].Items["id"].GetAsync()
any additional query parameter/request option/request header is set through a callback ...GetAsync(x => x.QueryParameters.Select = ["foo"])
any inline request body can be set using the new syntax ...PostAsync(new() { Property = "value"})
any inline response body can be set using the var syntax var result = await client.Drives["id"].Items["id"].GetAsync()
any arbitrary URL can be set through the WithUrl method client.Drives["id"].Items.WithUrl("urlFromNext/DeltaLink").GetAsync()

So I'd be curious to why the namespace name matters to you at all with that information?

Hopefully that additional context provides more clarity on the decisions that led to the current experience. Don't hesitate if you have additional feedback/questions.

catmanjan commented 7 months ago

@baywet regarding documentation, where can developers learn how we are actually meant to make every possible Graph function using Kiota?

The documentation you linked is the pure REST API calls, and because the naming conventions between Kiota and the REST APIs are now so vastly different it is difficult to translate between them (v4 had this issue too, but not as bad)

chadwackerman commented 7 months ago

@baywet Thank you for daring to step into this conversation.

In my case, I have a helper method that determines the App Folder inside OneDrive. Like so many other gaps, this piece of functionality is not exposed because the design of the library assumes you'll be making one-off REST calls. Frankly the library seems optimized for whiteboards and blog posts instead of getting things done.

So the top of my code has this beauty and I have methods that return it so that other methods can compose paths beyond it.

using DriveItemRequestBuilder = Microsoft.Graph.Drives.Item.Items.Item.DriveItemItemRequestBuilder;

The "new syntax" for query parameters (barely documented in the v5 release notes) is clear as mud and not discoverable. Same for all the magic strings inside dictionaries. This really isn't what "fluent" is meant to be. Fluent was a clever trick to leverage autocomplete to make things discoverable. You're up to 20,000 types? Maybe it's not the right model now.

Previously I mentioned Polly and System.CommandLine in hopes of shining light on the problem here. In that spirit, another famous example:

https://developer.apple.com/documentation/contacts/cnlabelcontactrelationeldercousinmotherssiblingsdaughterorfatherssistersdaughter

Yes, there are reasons behind that whole mess. But they're trying to represent a graph as a list and for some reason they're doing it in the global namespace. And yes everybody is rolling their eyes and laughing at it, because it's obviously the wrong abstraction and a pretty dumb stab at the problem.

I encourage you to set up a retry using Microsoft.Extensions.Http.Polly to get a feel for Fluent-Gone-Bad. While I'm sure the people who designed it can rattle that stuff off instantly, it presents an instant, in-your-face brick wall to mortals.

Graph should be this magical thing that pulls all these Microsoft worlds into an easy-to-understand, super-fun-to-program library. I should discover entire aspects of the company and cool products I never heard of when I press dot. Instead it feels like I'm stomping around in C++ header files circa 1995.

I again suggest building a real library instead of shipping endless versions of what's little more than incomplete, buggy, robot generated glue barely sufficient for PowerShell scripts.

baywet commented 7 months ago

Thank you both for the additional context here.

@catmanjan I want to make sure I understand the question properly. Are you asking for the naming conventions when kiota projects the API into the fluent API or are you asking about how you can discover the different capabilities of the API?

@chadwackerman

Query parameters docs: is this what you're looking for (besides the snippets we already have in the graph documentation) https://learn.microsoft.com/en-us/openapi/kiota/query-parameters-request-headers ?

Wrapper method: I'm not sure I understand why you're returning the request builder? Clearly the intent was to enable reusability through a level of indirection. Why don't you simply return the task of the result type?

Polly integration issue: can you provide a snippet to illustrate your argument here please?

Graph structure: Microsoft graph is not technically a graph database (no ability to query proximity between objects). It's technically a graph (in the structure sense) but in practical terms is a tree (because of the way it's implemented via path segmentation over HTTP). I'll avoid expanding further on this aspect, this is the API we have, and it's the API the SDKs must provide an experience for. Given that we have a tree, and that each node/branch is unique, we modeled the fluent API on top of a tree structure as well (request builders, query parameters, etc...) and not a list as you mentioned. Prior attempts at modeling it as a directed graph with reusable sections (like previous versions of this SDK) left out a lot of capabilities from the API that were impossible to model this way. Prior attempts at making a general purpose odataish client (javascript SDK, other internal experiments) either left out a lot of information and the users had to fend for themselves, or assumed people would have a deep understanding of odata conventions. Even for the few who had that deep understanding, because the API is not a formal odata service, they couldn't use general purpose odata tooling and quickly hit limitations of the service. Based on all that prior work and experience, a fluent API based on a tree felt like the best experience we could provide in terms of discoverability, ease of use and API coverage. And the numbers in terms of usage back this initial assumption. Hopefully that additional context helps understand the current solution.

Now, we always strive to improve our products where we can. If you have additional, specific, actionable feedback about where things could be improved, we'd be happy to receive it.

pschaeflein commented 7 months ago

@baywet regarding documentation, where can developers learn how we are actually meant to make every possible Graph function using Kiota?

The documentation you linked is the pure REST API calls, and because the naming conventions between Kiota and the REST APIs are now so vastly different it is difficult to translate between them (v4 had this issue too, but not as bad)

Depending on where you live, this may be helpful...

chadwackerman commented 7 months ago

@baywet You're taking my examples a bit too literally and squaring off my attempts at a Socratic circle. I understand how you got here. You do not yet seem to understand where I'm coming from.

Of course you have lots of users! This thing is the only way to build atop hundreds of billions of dollars of Microsoft R&D. You could ship something that issues a console beep every time it POSTs and you'd still have lots of downloads. Not to mention that releasing hundreds of versions of robo-generated code sure helps pad those Nuget stats!

The real question is: are you serving these users properly and is the tech any good?

Query parameters docs: is this what you're looking for (besides the snippets we already have in the graph documentation) https://learn.microsoft.com/en-us/openapi/kiota/query-parameters-request-headers ?

No, in fact it's a good example of what I'm not looking for. When using a library like this I want to worry about as little as possible of what lies beneath it. Getting referred to the documentation of a dependency with a funny name is an indicator that this has broken down. Forcing me to learn the mental model of the tool that you built this thing on, heck even being aware that it even exists, is part of the problem. I know REST. I know OneDrive. Why do I know about kiota and Microsoft.Graph.Drives.Item.Items.Item.DriveItemItemRequestBuilder?

"OData too hard? Let's give 'em yet another bespoke async paging topology where they can spend three hours trying to get the parameters to line up instead..."

Your goal, I suppose, is to provide a Fluent API. So why when I hit dot does autocomplete not lead the way?

Wrapper method: I'm not sure I understand why you're returning the request builder? Clearly the intent was to enable reusability through a level of indirection. Why don't you simply return the task of the result type?

Yet another problem of the library is that it's not always clear when things will actually hit the net. Also again, it's Fluent and builders, right? Elsewhere in Microsoft you pass these around, build layers atop them. WebRequestBuilder, UriBuilder... why are these sane and the Graph ones... less so? And why are there so many of them? And as the original poster asks, why are the names so dumb?

Because compatibility? You ran a tool to generate thousands of types and now you can't rename them without breaking things. Umm...

Polly integration issue: can you provide a snippet to illustrate your argument here please?

Along with the Apple nieces-and-nephews thing this seems to be causing confusion. But if you're up for a quick homework assignment, try to create a console app with a singleton HttpClient wrapped by Microsoft.Extensions.Http.Polly that makes a call to www.bing.com and retries once after three seconds if it gets a Http code error suitable for retry. Again, this has nothing directly to do with Graph (and I do appreciate your clarification that retries are handled internally by the Graph library). But it illustrates by example another library where Microsoft seems to think it makes perfect sense but it really does not. Like Graph the docs use a lot of words but somehow always come up short on helping you build a mental model of what you're trying to achieve.

If Amazon S3 or Twilio or Stripe shipped libraries structured like this they'd have 0% market share. And if this approach is so great, why doesn't Azure use it?

baywet commented 7 months ago

I know REST. I know OneDrive. Why do I know about kiota and Microsoft.Graph.Drives.Item.Items.Item.DriveItemItemRequestBuilder?

I've already shared earlier why you shouldn't need to know about any of that, and why the only reason you need to is because of the wrapper you're trying to build. Wrapper method which, without you sharing additional context on why you're doing so, seems like it's not achieving the (assumed) goals of reusability or indirection.

So why when I hit dot does autocomplete not lead the way?

Again, what specifically is not working in the autocomplete today?

on helping you build a mental model of what you're trying to achieve.

Again, if help is what you're looking for, please share some additional context on what you're trying to achieve here.

If Amazon S3 or Twilio or Stripe shipped libraries structured like this they'd have 0% market share. And if this approach is so great, why doesn't Azure use it?

Azure breaks it's APIs in multiple completely isolated domains (e.g. the vision APIs do not share anything in common with the storage APIs from a modeling perspective). So does Amazon. As for Twilio and Stripe, their API surfaces are MUCH smaller than Microsoft Graph. All of them inherently didn't have to solve for the kind of problems we had to solve for.

chadwackerman commented 7 months ago

Wrapper method which, without you sharing additional context on why you're doing so, seems like it's not achieving the (assumed) goals of reusability or indirection.

@baywet: My method works great: does what it needs to do, works around missing functionality in your library, exposes stupid names because you have a bot generating them on a technology that doesn't map properly to the solution you're attempting to solve. See original post written by someone else.

Graph was also the only component I use that didn't work on .NET 8 launch day. Promptly fixed but doesn't give confidence that Microsoft really cares about this project. https://github.com/microsoftgraph/msgraph-sdk-dotnet/issues/2214

We've now reached the point of two smart people talking past each other so I'll bow out to make room for others to share their opinions, or simply come back in five years and see how this ages.

I'll avoid expanding further on this aspect, this is the API we have, and it's the API the SDKs must provide an experience for. Given that we have a tree...

microsoft-graph

I wish you the best of luck.

catmanjan commented 7 months ago

or are you asking about how you can discover the different capabilities of the API?

Yes this is what I'm after, it would be good if developers could have some documentation that is only Kiota formatted, currently you have to jump between REST URIs/python/dotnet to consume whats already on the doco website

baywet commented 7 months ago

@catmanjan can you expand on what you mean please by "some documentation that is only Kiota formatted", are the current doc comments not enough? (they also contain links to the public documentation)

catmanjan commented 7 months ago

@baywet for example if you're a new Graph developer using this library I think you are expected to start here: https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/dev/docs/overview.md

Which is light on details on what is possible, so you quickly have to swap over to the other Graph site https://learn.microsoft.com/en-us/graph/overview?view=graph-rest-1.0

These are fine as an experienced Graph developer because you are familiar with the correlation between REST and Kiota, but new developers find it very confusing reading about REST endpoints when Kiota is quite different

The C# sample request are good, but then the responses are JSON which isn't helpful either

baywet commented 7 months ago

Thanks for the additional information. I want to make sure I understand your concerns correctly.

It's hard to discover from the documentation what's possible on the API surface.
Responses being in JSON and not in the desired language is confusing.

Is that correct?

Have you ever come across this page and more importantly the tree view under API v1.0 reference?
How would you represent the returned data then? As an object being initialized in the language?

MohamadAlallan-Pi commented 7 months ago

Hi guys, you took this too far. it is as simple as this, can we have GetAllSites instead of GetAllSites.GetAsGetAllSitesGetResponseAsync because I rather go create my own active directory than using such a library.

baywet commented 7 months ago

On the next set of breaking changes it'll be GetAllSites.GetAsync. @maisarissi @sebastienlevert this whole conversation is a good reminder we should schedule a major version for kiota and for the dotnet SDK to cleanup those things.

stijnherreman commented 7 months ago

I know REST. I know OneDrive. Why do I know about kiota and Microsoft.Graph.Drives.Item.Items.Item.DriveItemItemRequestBuilder?

I've already shared earlier why you shouldn't need to know about any of that, and why the only reason you need to is because of the wrapper you're trying to build. Wrapper method which, without you sharing additional context on why you're doing so, seems like it's not achieving the (assumed) goals of reusability or indirection.

I can chime in on this. Doing an upgrade from v4 to v5 and I have to make changes like this:

- private async Task<IDriveItemChildrenCollectionRequest> GetChildrenRequest(...)
+ private async Task<Microsoft.Graph.Drives.Item.Items.Item.Children.ChildrenRequestBuilder> GetChildrenRequest(...)

- private PaginationResult<DriveItem> MapDeltaResult(IDriveItemDeltaCollectionPage result)
+ private PaginationResult<DriveItem> MapDeltaResult(Microsoft.Graph.Drives.Item.Items.Item.Delta.DeltaGetResponse result)

Now I'm just doing the upgrade here for code written by other people, so I can't comment on how much sense these reusable methods make, but it is real-world code.

stijnherreman commented 5 months ago

Follow-up to my previous comment: wrapping the API can introduce bugs if you don't pay good attention. For example client.Drives[driveId].Items[folderId].ItemWithPath(filename) returns a CustomDriveItemItemRequestBuilder that shadows the original properties, so a wrapper method taking a DriveItemItemRequestBuilder parameter will drop the filename from the URL.

This was all fine of course when working with the interfaces like IDriveItemRequestBuilder. I guess I'll refactor and throw out the wrapper methods 🙂

craig-blowfield commented 1 month ago

I know this is an old issue but @chadwackerman @catmanjan I completely agree.

I am new to MS Graph, but working with V5 of the SDK has been so painful and I have wasted so much time with this API and SDK.

These types of SDK's provide such little value to developers when trying to solve 'application' problems, adding a pointless abstraction on top of the REST API, which we essentially have to learn anyway to understand how we use the SDK's fluent API.

I would of saved so much time if I just just learnt the REST API directly and used a framework like https://flurl.dev/.

Also the documentation is just poor and the examples are not even syntactically correct in lots of places?

I think someone from MS really needs to get a handle on the poor state this SDK.

microsoftgraph / msgraph-sdk-dotnet