PagedResponse<T> KeyVault Certificate GetPropertiesOfCertificates does not honor the IncludePending optional parameter after the first page

[ahsonkhan]

The call to fetch the first page sets the appropriate query parameters based on the input parameter value:

azure-sdk-for-cpp/sdk/keyvault/azure-security-keyvault-certificates/src/certificate_client_paged_response.cpp

Lines 28 to 31 in bef4201

	GetPropertiesOfCertificatesOptions options;
	options.NextPageToken = NextPageToken;
	*this = m_certificateClient->GetPropertiesOfCertificates(options, context);
	CurrentPageToken = options.NextPageToken.Value();

But subsequent pages do not have that value set. That means, if the includePending param is set to true, it will not return all the certificates (including pending ones), if the pending certificate happens to be listed in a page other than the first.

azure-sdk-for-cpp/sdk/keyvault/azure-security-keyvault-certificates/src/certificate_client.cpp

Lines 384 to 395 in bef4201

	CertificatePropertiesPagedResponse CertificateClient::GetPropertiesOfCertificates(
	GetPropertiesOfCertificatesOptions const& options,
	Azure::Core::Context const& context) const
	{
	(void)options;
	// Request and settings
	auto request = ContinuationTokenRequest({CertificatesPath}, options.NextPageToken);
	if (options.IncludePending)
	{
	request.GetUrl().AppendQueryParameter(
	IncludePendingQuery, options.IncludePending.Value() ? TrueQueryValue : FalseQueryValue);
	}

Here's the swagger (not sure if this requires some fix to the swagger):
https://github.com/Azure/azure-rest-api-specs/blob/4a4acecea9901c29e19ba50f2d4cf65b20115b69/specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.5/certificates.json#L30-L83

A potential fix (to illustrate the concept) would be to add an options field to the PagedResponse<T> object (in this case CertificatePropertiesPagedResponse):

GetPropertiesOfCertificatesOptions m_operationOptions;

And set this before returning it from the client, rather than creating a new options bag within OnNextPage(), making sure it is passed in to subsequent method calls. Within GetPropertiesOfCertificates:

auto pagedResponse = CertificatePropertiesPagedResponse(
    std::move(value), std::move(rawResponse), std::make_unique<CertificateClient>(*this));
pagedResponse.m_operationOptions = options;
return pagedResponse;

Within OnNextPage:

m_operationOptions.NextPageToken = NextPageToken;
*this = m_certificateClient->GetPropertiesOfCertificates(m_operationOptions, context);
CurrentPageToken = m_operationOptions.NextPageToken.Value();

Sample repro:

#include <iostream>
#include <azure/identity.hpp>
#include <azure/keyvault/certificates.hpp>

#include <vector>
#include <sstream>
#include <iomanip>
#include <cstdint>
#include <string>

using namespace Azure::Identity;
using namespace Azure::Security::KeyVault::Certificates;

int main()
{
	// Pre-req: Create 25 certificates first so a page is full (either through the portal or programmatically) 

	// Case 1: Create a certificate (either on the portal or programmatically) on the first page, and run this, right away.
        // Works as expected.
	// Case 2: Create a certificate (either on the portal or programmatically) on any subsequent page, and run this, right away.
        // Doesn't work as expected.

	try
	{
		// TODO: Set to your own KeyVault URL
		auto keyVaultUrl = "https://<keyvault-name>.vault.azure.net/";
		auto cred = std::make_shared<AzureCliCredential>();
		CertificateClient client(keyVaultUrl, cred);

		int countFalse = 0;
		std::cout << "Certificates in the key vault (includePending = false):" << std::endl;
		for (auto cert = client.GetPropertiesOfCertificates(); cert.HasPage(); cert.MoveToNextPage())
		{
			std::cout << "Found " << cert.Items.size() << " certificates." << std::endl;
			for (auto item : cert.Items)
			{
				std::cout << item.Name << std::endl;
				countFalse++;
			}
		}

		int countTrue = 0;
		GetPropertiesOfCertificatesOptions options;
		options.IncludePending = true;
		std::cout << "Certificates in the key vault (includePending = true):" << std::endl;
		for (auto cert = client.GetPropertiesOfCertificates(options); cert.HasPage(); cert.MoveToNextPage())
		{
			std::cout << "Found " << cert.Items.size() << " certificates." << std::endl;
			for (auto item : cert.Items)
			{
				std::cout << item.Name << std::endl;
				countTrue++;
			}
		}

		// Expected countFalse < countTrue in both cases, since there's a certificate pending.
		// Case 1: In the case where the certificate gets created on the first page:
		//  -> countFalse < countTrue
		// Case 2: But, in the case where the certificate gets created on any other subsequent page:
		//  -> countFalse = countTrue
		std::cout << "countFalse = " << countFalse << " vs countTrue = " << countTrue << std::endl;
	}
	catch (Azure::Core::Credentials::AuthenticationException const& ex)
	{
		std::cout << ex.what() << std::endl;
	}
}

vcpkg.json

{
    "dependencies": [
      "azure-identity-cpp",
      "azure-security-keyvault-certificates-cpp"
    ]
  }

vcpkg-configuration.json

{
    "default-registry": {
      "kind": "git",
      "baseline": "b61706e96cde9bdb6ecab86156fb44547cdafb28",
      "repository": "https://github.com/microsoft/vcpkg"
    },
    "registries": [
      {
        "kind": "artifact",
        "location": "https://github.com/microsoft/vcpkg-ce-catalog/archive/refs/heads/main.zip",
        "name": "microsoft"
      }
    ]
  }

The issue is pervasive across all the PagedResponse<T> methods that follow this pattern within the KeyVault SDKs, but GetPropertiesOfCertificates seems to be the only one that has optional parameters which are settable by the SDK methods (unlike maxResults) and hence has an actual behavioral bug here.

All other SDKs PagedResponse<T> pattern, including Storage, set the optional parameters appropriately in both the first and subsequent pages:

azure-sdk-for-cpp/sdk/storage/azure-storage-blobs/src/blob_container_client.cpp

Line 367 in bef4201

pagedResponse.m_operationOptions = options;

Related issues in other languages:
Azure/azure-sdk-for-net#47202
#6235
Azure/azure-sdk-for-python#38589
Azure/azure-sdk-for-go#23772
Azure/azure-sdk-for-js#31803
Azure/azure-sdk-for-java#42988