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.

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

Why does AddEntityOptions exists and why does it derive from UpsertEntityOptions (when UpdateEntityOptions and MergeEntityOptions don't) #6231

Closed
ahsonkhan opened this issue Nov 15, 2024 · 2 comments · Fixed by #6314
Assignees
Labels
blocking-release Blocks release design-discussion An area of design currently under discussion and open to team and community feedback. Tables

Comments

@ahsonkhan
Copy link
Contributor

ahsonkhan commented Nov 15, 2024

This options bag doesn't seem to be used to modify client behavior for TableClient::AddEntity:

struct AddEntityOptions : public UpsertEntityOptions
{
AddEntityOptions() = default;
/**
* @brief Create Entity options constructor.
*
* @param other Upsert Entity options.
*/
explicit AddEntityOptions(UpsertEntityOptions const& other) { (void)other; }
};

Azure::Response<Models::AddEntityResult> TableClient::AddEntity(
Models::TableEntity const& tableEntity,
Models::AddEntityOptions const& options,
Core::Context const& context)
{
(void)options;

Following-up from #6229 (comment)

.NET doesn't expose this:
https://github.com/Azure/azure-sdk-for-net/blob/4e1173308d32f0413788359a87376e673f2ab15b/sdk/tables/Azure.Data.Tables/src/TableClient.cs#L603

GoLang has it, but it contains MetadataFormat, rather than it being empty/inheriting from UpsertOptions:
https://github.com/Azure/azure-sdk-for-go/blob/3ebd0d439f9d8aa06a0764a60892a8001b997a34/sdk/data/aztables/client.go#L247

cc @LarryOsterman (since this is regarding API review)

@ahsonkhan ahsonkhan added blocking-release Blocks release design-discussion An area of design currently under discussion and open to team and community feedback. Tables labels Nov 15, 2024
@gearama
Copy link
Member

gearama commented Nov 15, 2024

works as exopected

@gearama gearama closed this as completed Nov 15, 2024
@gearama gearama reopened this Nov 15, 2024
@ahsonkhan
Copy link
Contributor Author

The bare-minimum fix here (without knowing a lot about the intended usage) would be to remove this inheritance from UpsertEntityOptions, so the options for Add, Update, Merge are consistent:

struct AddEntityOptions : public UpsertEntityOptions

That said, @LarryOsterman believes we have another issue:

The explicit constructor for AddEntityOptions which takes an UpsertEntityOptions is incorrect because it doesn't initialize the base class - instead it ignores the options passed in.

@gearama gearama mentioned this issue Dec 30, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release Blocks release design-discussion An area of design currently under discussion and open to team and community feedback. Tables
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants