microsoftgraph / msgraph-sdk-php

Microsoft Graph Library for PHP.
Other
578 stars 144 forks source link

Count on users (and probably all other count-enabled request builders) is failing silently #1366

Closed sebastienlevert closed 9 months ago

sebastienlevert commented 1 year ago

And it prevents further execution of an application.

$graphServiceClient->users()->count()->get()->wait();

Error is the following:

#0 /home/slevert/src/temp/msgraph-php/vendor/microsoft/kiota-serialization-json/src/JsonParseNode.php(107): Microsoft\Graph\Generated\Models\ODataErrors\ODataError::createFromDiscriminatorValue()
#1 /home/slevert/src/temp/msgraph-php/vendor/microsoft/kiota-http-guzzle/src/GuzzleRequestAdapter.php(602): Microsoft\Kiota\Serialization\Json\JsonParseNode->getObjectValue()
#2 /home/slevert/src/temp/msgraph-php/vendor/microsoft/kiota-http-guzzle/src/GuzzleRequestAdapter.php(237): Microsoft\Kiota\Http\GuzzleRequestAdapter->throwFailedResponse()
#3 /home/slevert/src/temp/msgraph-php/vendor/php-http/promise/src/FulfilledPromise.php(35): Microsoft\Kiota\Http\GuzzleRequestAdapter->Microsoft\Kiota\Http\{closure}()
#4 /home/slevert/src/temp/msgraph-php/vendor/microsoft/kiota-http-guzzle/src/GuzzleRequestAdapter.php(272): Http\Promise\FulfilledPromise->then()
#5 /home/slevert/src/temp/msgraph-php/vendor/microsoft/microsoft-graph/src/Generated/Users/Count/CountRequestBuilder.php(45): Microsoft\Kiota\Http\GuzzleRequestAdapter->sendPrimitiveAsync()
#6 /home/slevert/src/temp/msgraph-php/src/main.php(42): Microsoft\Graph\Generated\Users\Count\CountRequestBuilder->get()
sebastienlevert commented 1 year ago

This appears because I don't specify the ConsistencyLevel header... For counts, should we automatically add it? What are we doing in .NET and other languages? https://learn.microsoft.com/en-us/graph/api/user-list?view=graph-rest-1.0&tabs=php#example-3-get-only-a-count-of-users

Ndiritu commented 1 year ago

We don't automatically add it in the other languages. Graph's OpenAPI schema at the moment doesn't flag ConsitencyLevel as a required header for the $count requests. Also not sure there's a way to specify a default value for a required header.

baywet commented 1 year ago

We should NOT automatically add it. The reason why the service teams required that header was to:

  1. give control to the consumer over which service they want to target.
  2. make sure existing applications keep talking to DS API (instead of Mezzo), avoiding any edge cases which would cause side effects in production.

Without disclosing too much publicly, over time and based on telemetry, the service teams will relax the header/count requirements on a per endpoint basis so existing applications get transparently migrated over to mezzo.

sebastienlevert commented 1 year ago

That means we should know this in our snippet generation, then?

Ndiritu commented 9 months ago

Confirmed that our snippets contain the ConsistencyLevel header here