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.

Sign up for GitHub

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

Add samples for all AppConfig pager methods (GetSnapshots, GetConfigurationSettings, GetRevisions) with comparisons of what we'd expect the user experience to be. #6341

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Add samples for all AppConfig pager methods (GetSnapshots, GetConfigurationSettings, GetRevisions) with comparisons of what we'd expect the user experience to be. #6341

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c61a32
Add samples for all AppConfig pager methods (GetSnapshots, GetConfigu…
ahsonkhan Jan 14, 2025
938f410
Adding GetConfigurationSettings for snapshot.
ahsonkhan Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
290 changes: 288 additions & 2 deletions sdk/appconfiguration/azure-data-appconfiguration/samples/appconfig_basic_operation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ using namespace Azure::Data::AppConfiguration;
using namespace Azure::Identity;

// Retreive labels based on filters
static void RetreiveLabels(ConfigurationClient& configurationClient)
static void RetrieveLabels(ConfigurationClient& configurationClient)
{
// Current

Expand Down Expand Up @@ -74,6 +74,280 @@ static void RetreiveLabels(ConfigurationClient& configurationClient)
#endif
}

// Retreive key values based on filters
static void RetrieveConfigurationSettings(ConfigurationClient& configurationClient)
{
// Current

{
GetKeyValuesOptions options;
options.Label = "production*";

for (GetKeyValuesPagedResponse keyValuesPage
= configurationClient.GetKeyValues("accept", options);
keyValuesPage.HasPage();
keyValuesPage.MoveToNextPage())
{
if (keyValuesPage.Items.HasValue())
{
std::vector<KeyValue> list = keyValuesPage.Items.Value();
std::cout << "KeyValues List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;

if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}

// Expected

#if 0
{
GetConfigurationSettingsOptions options;
options.Label = "production*";

for (GetConfigurationSettingsPagedResponse keyValuesPage
= configurationClient.GetConfigurationSettings(options);
keyValuesPage.HasPage();
keyValuesPage.MoveToNextPage())
{
if (keyValuesPage.Items.HasValue())
Copy link
Member

@antkmsft antkmsft Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is beyond AppConfig, and this is not question only to you, but maybe we should have a design discussion with the architects, but I am wondering if we should make pageable items as non-nullable, at least, by default - I don't nullable here is the best user experience, and I don't expect nullable vs size == 0 to ever have meaningful distinction in the context of Azure SDK. Ideally, the users would just go with for (item : pageable.Items) right away, without checking for HasValue().
The spec says it is optional, and I get that, but if we don't take that literally, we could special-case that.
To be extra careful, we could make an option for that, which you could toggle to Off for the potential services where there is a meaningful distinction.

Copy link
Member

Choose a reason for hiding this comment

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

That's an interesting question. I think I agree with you that Items should just be a vector<T> and not a Nullable<vector<T>>.

My one concern is that I suspect it is legal for a pageable to return 0 entries for a page even if there are more pages still to come (in other words, HasPage is true, but Items is empty. It is a coding mistake for a customer to treat Items.empty() as if it was the same as HasPage() returning false, but it would be an easy mistake for a developer to make. Making Items be nullable basically renders that mistake impossible at the cost of a poor developer experience.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, but technically nothing prevents pageable.Items.HasValue() == false while pageable.HasPage() == true. And I also thought another possible mistake for the user to make - to assume that Items.Value().size() > 0 if Items.HasValue() == true. So, it is complicated, for sure, but maybe we could make it a tiny bit better.
Whether the service returns [] or null/undefined, we can generate code to hide that from user, in all cases the result will be as if it was [].

Copy link
Contributor Author

@ahsonkhan ahsonkhan Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good conversation. To Larry's point, having an empty page (where HasPage = true) is something we'd need to consider keeping the distinction clear on.

I am reminded that the customer can make the same call directly to the service and would have to handle the null or empty case, even outside of the SDK.
The simplest (and easiest to explain) stance is to honor the spec and service behavior as the source of truth. We could try to introduce usability improvements like this as a potential UX win for SDK users, but I am not sure if we potentially lead to further confusion or unintended consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your point, @antkmsft, the existing hand-written SDKs don't use Nullable<T> for the collection: https://github.com/Azure/autorest.cpp/issues/475

{
std::vector<KeyValue> list = keyValuesPage.Items.Value();
std::cout << "KeyValues List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;

if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}
#endif
}

// Retreive configuration settings for a snapshot
static void RetrieveConfigurationSettingsForSnapshot(ConfigurationClient& configurationClient)
{
// Current

{
GetKeyValuesOptions options;
options.Snapshot = "snapshot-name";

for (GetKeyValuesPagedResponse keyValuesPage
= configurationClient.GetKeyValues("accept", options);
keyValuesPage.HasPage();
keyValuesPage.MoveToNextPage())
{
if (keyValuesPage.Items.HasValue())
{
std::vector<KeyValue> list = keyValuesPage.Items.Value();
std::cout << "KeyValues List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;

if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}

// Expected

#if 0
{
GetConfigurationSettingsOptions options;
options.Snapshot = "snapshot-name";

for (GetConfigurationSettingsPagedResponse keyValuesPage
= configurationClient.GetConfigurationSettings(options);
keyValuesPage.HasPage();
keyValuesPage.MoveToNextPage())
{
if (keyValuesPage.Items.HasValue())
{
std::vector<KeyValue> list = keyValuesPage.Items.Value();
std::cout << "KeyValues List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;

if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}
#endif
}

// Retreive snapshots based on filters
static void RetrieveSnapshots(ConfigurationClient& configurationClient)
{
// Current

{
GetSnapshotsOptions options;
options.Name = "production*";
options.Status = {SnapshotStatus::Ready, SnapshotStatus::Archived};

for (GetSnapshotsPagedResponse snapshotsPage
= configurationClient.GetSnapshots("accept", options);
snapshotsPage.HasPage();
snapshotsPage.MoveToNextPage())
{
if (snapshotsPage.Items.HasValue())
{
std::vector<Snapshot> list = snapshotsPage.Items.Value();
std::cout << "Snapshot List Size: " << list.size() << std::endl;
for (Snapshot snapshot : list)
{
std::cout << snapshot.Name;

if (snapshot.RetentionPeriod.HasValue())
std::cout << " : " << snapshot.RetentionPeriod.Value();

if (snapshot.Status.HasValue())
std::cout << " : " << snapshot.Status.Value().ToString();
std::cout << std::endl;
}
}
}
}

// Expected

#if 0
{
GetSnapshotsOptions options;
options.Name = "production*";
options.Status = {SnapshotStatus::Ready, SnapshotStatus::Archived};

for (GetSnapshotsPagedResponse snapshotsPage = configurationClient.GetSnapshots(options);
snapshotsPage.HasPage();
snapshotsPage.MoveToNextPage())
{
if (snapshotsPage.Items.HasValue())
{
std::vector<Snapshot> list = snapshotsPage.Items.Value();
std::cout << "Snapshot List Size: " << list.size() << std::endl;
for (Snapshot snapshot : list)
{
std::cout << snapshot.Name;

if (snapshot.RetentionPeriod.HasValue())
std::cout << " : " << snapshot.RetentionPeriod.Value();

if (snapshot.Status.HasValue())
std::cout << " : " << snapshot.Status.Value().ToString();
std::cout << std::endl;
}
}
}
}
#endif
}

// Retreive revisions based on filters
static void RetrieveRevisions(ConfigurationClient& configurationClient)
{
// Current

{
GetRevisionsOptions options;
options.Key = "some-key";

for (GetRevisionsPagedResponse revisionsPage
= configurationClient.GetRevisions("accept", options);
revisionsPage.HasPage();
revisionsPage.MoveToNextPage())
{
if (revisionsPage.Items.HasValue())
{
std::vector<KeyValue> list = revisionsPage.Items.Value();
std::cout << "Revisions List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;
if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}

// Expected

#if 0
{
GetRevisionsOptions options;
options.Key = "some-key";

for (GetRevisionsPagedResponse revisionsPage = configurationClient.GetRevisions(options);
revisionsPage.HasPage();
revisionsPage.MoveToNextPage())
{
if (revisionsPage.Items.HasValue())
{
std::vector<KeyValue> list = revisionsPage.Items.Value();
std::cout << "Revisions List Size: " << list.size() << std::endl;
for (KeyValue keyValue : list)
{
Azure::Nullable<std::string> valueOfKey = keyValue.Value;
if (valueOfKey.HasValue())
{
std::cout << keyValue.Key << " : " << valueOfKey.Value() << std::endl;
}
else
{
std::cout << "Value for: '" << keyValue.Key << "' does not exist." << std::endl;
}
}
}
}
}
#endif
}

int main()
{
try
Expand Down Expand Up @@ -181,7 +455,19 @@ int main()
#endif

// Retreive labels based on filters
RetreiveLabels(configurationClient);
RetrieveLabels(configurationClient);

// Retreive configuration settings based on filters
RetrieveConfigurationSettings(configurationClient);

// Retreive configuration settings for a snapshot
RetrieveConfigurationSettingsForSnapshot(configurationClient);

// Retreive snapshots based on filters
RetrieveSnapshots(configurationClient);

// Retreive revisions based on filters
RetrieveRevisions(configurationClient);

// Delete a configuration setting

Expand Down
Loading