Generated SDK is not correct

[1oglop1]

This is a follow-up on #479.

Originally we've discussed a validation of the input parameter for the method graphClient.applications.byApplicationId("") https://github.com/microsoftgraph/msgraph-sdk-typescript/blob/2c6c78a9a893ce2b244f34a0d1f0cfeff3e75dda/packages/msgraph-sdk-applications/applications/index.ts#L48-L52 Now I think the method is straight-up wrong. Because the documentation string mentions a collection but the result should be a single item or error. The problem is that this method does not call /applications/{application-id} | applications.application.GetApplication but instead queries the list method operationId: applications.application.ListApplication.

I've looked at the OA spec https://github.com/microsoftgraph/msgraph-metadata/blob/0ce6deaacf593d97bacecf9a2ebbcf5f6b199abe/openapi/v1.0/openapi.yaml and discovered several things:

• The SDK methods have weird names or use the name of HTTP method, instead they should IMO use the name from of the operationId example:

// Original
const appRequest = graphClient.applications.byApplicationId("")
const application = await appRequest.get();  // 

// according to API spec
const appItem = graphClient.applications.GetApplicationByAppId("") // or GetApplication

• Generated structure does not follow the docs which makes the SDK difficult to use.
• It is not possible/simple to use item methods without creating an item. This required the developer to go through the hoops and null checks. (I think I saw several similar issues in kiota)

  // @microsoft/msgraph-sdk-applications/applications/item
  const appRequest: ApplicationItemRequestBuilder = graphClient.applications.byApplicationId("")
  const result = await appRequest.toDeleteRequestInformation();
  
  // easier would be something like this
  graphClient.applications.DeleteApplication({appId: "123"}) 

I think that AWS SDK v3 has much nicer patterns and it would be great if cloud SDKs could follow a similar interface. https://github.com/aws/aws-sdk-js-v3

const { DynamoDBClient, ListTablesCommand } = require("@aws-sdk/client-dynamodb");

(async () => {
  const client = new DynamoDBClient({ region: "us-west-2" });
  const command = new ListTablesCommand({});
  try {
    const results = await client.send(command);
    console.log(results.TableNames.join("\n"));
  } catch (err) {
    console.error(err);
  }
})();

If I translate this to ms-graph, it may look like this:

const graphClient = createGraphServiceClient(requestAdapter);
// import {GetApplicationByAppId} from "@microsoft/msgraph-sdk-applications"
const getApplicationsCommand = new GetApplicationByAppId({id: 12})
const item = await graphClient.send(getApplicationsCommand)

I find this much more explicit than the current module augmentation. https://github.com/microsoftgraph/msgraph-sdk-typescript/blob/2c6c78a9a893ce2b244f34a0d1f0cfeff3e75dda/packages/msgraph-sdk-applications/index.ts#L6-L12

[baywet]

Hi @1oglop1 , Thank you for opening this issue as a follow-up. There is a bit of context here that might be useful for other readers.

First off, there has been a general confusion for the application between the object ID which is the ID property on Microsoft graph and the application ID which is the client ID. Those are two different properties and the API sometimes accepts one sometimes accepts the other. At this point this is historical and besides improving the documentation there is not much we can do to fix that.

Another aspect to the sdk is that the generation code segments of the fluent API closely map with the path segmentation of the rest API itself. This is done this way to ensure we can support the large API surface of Microsoft graph which has over 20,000 operations. The other reason why it's done this way is to reduce the mental burden of mapping one to another. So while the AWS SDK might work in their context, we could not apply that at scale to the Microsoft graph SDK.

About the modular augmentation, the reason why we had to implement that arguably complex solution was to reduce the end bundle size of any application. Long story short, if you only use a couple of operations, you don't want the full API surface to be pulled into your runtime application bundle, which would negatively impact the performance of your application.

With this additional context, what's happening here causing the confusion between the list operation and the get operation is the fact that you're not providing an ID to the parameter. That compounds with the fact that the underlying parameters validation is not validating for the missing ID. Which also compounds with the different types of IDs issue I outlined earlier. And lastly, when the URL template gets expended or when the service reaches two slashes it returns the result of a list operation instead of a 404.

I believe in fixing the perimeter validation will already go a long way to clarifying the confusion. I'll go ahead and create an issue in the corresponding dependency repository.

[baywet]

Here is the related issue https://github.com/microsoft/kiota-typescript/issues/1299

[baywet]

Update: the issue has been addressed a while ago now and should not be present anymore if you upgrade all dependencies to latest. I simply forgot to revert here, sorry about that. Closing. Let us know if you have any additional comments or questions.