azure-sdk-for-net: [TABLES] GetEntity/Async should not throw exception on 404

@christothes and @kieronlanning

Hate to put my head into the lion’s mouth…

Not sure what design guidelines you are referring to. The book Framework Design Guidelines written by the authors of the .NET Framework frown on throwing exceptions from APIs similar to this. This is a poor practice and shouldn’t be happening. Go and look at all Microsoft data APIs and you’ll not see this. (ADO, ADO.NET, EntityFramework, etc.)

Why not give developers a choice on the return value when a record is not found. By default (sigh) an exception is thrown. If the developer sets a property on the call or on the TableClient say, ReturnNullWhenRecordNotFound = false, but they set it to true, then return null instead of throwing an exception. Many problems solved.

There are other ways this could easily be fixed.

I STRONGLY agree with others who have stated that unnecessary exceptions logged to their Azure logs cause a lot of noise for an API that is not implemented correctly or like the rest of all data APIs I’ve ever used.

If you want to avoid a 404 when the resource does not exist, you could replace a call to GetEntity with a call to Query that includes the PartionKey and RowKey.

@christothes You are making the API not fluent at all. I have never ever seen an API throwing exceptions on 404 on a GET operation. I do not see a good reasoning behind this decision.

@christothes I still strongly disagree. Not with your above text, but on the premise that a record not found, should cause an exception.

A record not found is a condition or state, not an error. It does not warrant being elevated to an exception. This is a misuse of exceptions.

We can do this all day long, but I can see as the author of the guidelines you’ve made up your mind.

I recommend you look at all other data APIs, can you find a single data API that throws an exception when a record can’t be found? If not, you should correct your design guidelines to reflect the rest of the world’s view on this matter. If you find one, please send me a link.

This problem all boils down to deciding if a record not found operation is an error or just a simple state. All APIs I’ve ever used handle this as a state and not a runtime error.

The Azure Table APIs were designed to follow the Azure SDK Guidelines. According to those guidelines, anytime a service method returns 404, an exception will be thrown.

We are considering adding a method (something like GetEntityIfExists) that will not throw on a 404. There are other options we are looking into as well for how to best expose such a functionality in the library.

Thank you for your feedback!

I (firmly) believe it’s a standards problem. Someone, somewhere has arbitrarily decided that the existing standard for though shall not throw needlessly exceptions wasn’t good enough. Now, for these API, everyone needs dedicated knowledge.

So while there isn’t directly a problem in calling Query vs. Get, every single developer now and in the future must understand that the impact of calling a Get rather than a Query.

A fundamental, non-industry standard way of calling an API.

Any mistake, any missed Get in a PR/ code review has a financial impact. Especially in high-read system.

Should be this week.

Adding @KrzysztofCwalina to this thread as he defined the .NET Azure SDK guidelines @christothes was referring to (and also wrote the FDG).

@longtimedeveloper there are several APIs in the .NET Framework that throw when a record can’t be found. An example of this is the Dictionary look-up which will throw if the key is not found.

I agree with you and @kieronlanning that there are scenarios where not throwing an exception (even if the service returns 404) might be desirable. We can look into whether or not we can have a method similar to GetEntityIfExists that will not throw even when the service is returning 404.

However, even if we make this change, it will be a specific case for this library and not a general change in the guidelines as, in general, we throw and exception when we receive a 404 response from the service.

@christothes these guidelines are incorrect and I’m so glad the rest of .NET does not do this. These are in violation of the Framework Design Guidelines book that Microsoft published. Currently in its third release. These guidelines are not in line with any other Microsoft data API. Whoever wrote those Azure guidelines has not read nor followed the Framework Design Guidelines book. The good news is, this is easily correctable!

When I was a lead, I never allowed any team member to work without first reading the book Framework Design Guidelines. I required them to bring in questions from each chapter to ask me. All the code I’ve ever written follows this book. Highly recommend this book to any .NET developer. The authors are splendid developers and individuals.

Could you explain why choosing to call Query instead isn’t sufficient if throwing isn’t desirable when the entity doesn’t exist?

Honestly, that is not the point; developers should not have to ignore APIs because they have incorrect implementation.

GetAsync is super because it is async, and easy to call, and follows other data API for getting a single record. (except for the exception business)

QueryAsync has many powerful features but is overkill to get a single record. It returns an AsyncPageable that requires extra code to work with (you have to loop through the AsyncPageable collection), compared to the below two functions that use Query. But Query is not async.

From a CPU performance standpoint, GetAsync probably uses fewer resources than QueryAsync. Since Azure is a cloud service, I’m thinking SDK developers would want to ship customers the most performant code possible for common scenarios like getting a single record, correct?

My TableService for getting single records (Get, GetAsync replacements)

public T QueryFirstOrDefault<T>(Expression<Func<T, Boolean>> predicate) where T : class, ITableEntity, new() {
    return _tableClient.QueryAsync<T>(predicate).FirstOrDefault();
}

public T QuerySingleOrDefault<T>(Expression<Func<T, Boolean>> predicate) where T : class, ITableEntity, new() {
    return _tableClient.Query<T>(predicate).SingleOrDefault();
}

You can see I’ve worked around the poor design of Get and GetAsync. But these APIs should be fixed.

Framework Design Guidelines 3rd Edition on Amazon

BTW: I’m very passionate about code, but hope I’m not coming off as disrestful. If so, I apologize in advance. Best regards, Karl

I’m tracking some of these at https://github.com/Azure/azure-sdk-for-net/issues/25626. @christothes - consider adding GetEntity to the list.

@longtimedeveloper, if you opt-into ResponseStatusOption.NoThrow all error logging will be suppressed, i.e. we will treat 404 as a success.

adding @annelo-msft and @tg-msft who are working on related features.

@AlexGhiondea how do you reply to the question, “why is this API different from ADO, ADO.NET, EntityFramework, and the previous APIs for Table Storage?”

This API is the only one throwing on a get single record request.

@AlexGhiondea Thank you! @KrzysztofCwalina hope all is well with you and your family, been a long time! Thank you for continuing to update FDG.

This issue boils down to public data API consistency and consistency with data APIs (ADO, ADO.NET, Entity Framework, etc.) that developers have used for years. Developers don’t expect an exception from a data API because the record is not found. Doesn’t happen.

This smells like a leaky abstraction where you’re leaking your internal states out into your public API. Internally you got a 404 from some service, but the public data API should not have to be a victim to internal code.

An example of this is the Dictionary look-up which will throw if the key is not found.

Yes, but there are methods you can use like TryGetValue that mitigate getting an exception.

If you’re insisting on exceptions, why not provide TryGet and TryGetAsync or similar as you suggested?

The victims of these exceptions are the Azure Logs being littered with noisy useless exceptions, performance, and having to use the Query API methods instead of the Get API methods. Forcing developers to use Query so they can keep their Azure logs clean seems unnecessary.

Yes, understand this TryGet would only be implemented in this library.

If you do this, think of the performance gains and clean Azure logs!

Thank you, best regards, Karl

To voice what @longtimedeveloper has said, it does feel very much like the decision has been made, and actually, if that’s the default then it’s likely more dangerous to change it, regardless of the discussion here and the merits of either side (you have your hill, we have ours #amirght).

However… what we’re (sorry if I’m lumping anyone else into this) proposing is a middle-ground… an option in the client that allows developers to choose…

Throw exceptions on null for get or not.

It feels like a low impact compromise, and it solves a variety of problems for everyone who is used to non-throwing APIs for get-based operations.

Failing that, a GetOrDefault(Async) much like LINQs First/ SingleOrDefault that doesn’t throw internally (for high-performance scenarios).

Sure - just seems counter intuitive. Especially as a single bool on creating the client could make it work like every other non-Azure-SDK-package made…

@kieronlanning exactly my thoughts.

This needs to change to behave like any other normal service NO EXCEPTION on 404, just a normal 404 response.

@christothes I think that’s the definition everyone here has a problem with. 404 is a perfectly valid response - it doesn’t feel like an exceptional circumstance.

Making the users of the API jump through several ill-fitting hoops to satisfy that document is the wrong way around.

I’m not sure I know of many APIs within the .NET or programming space that actively throw an exception when you try and retrieve a resource that doesn’t exist. Null is fine… a response object we can check is ok. Throwing is never ok.

@christothes That’s a genuine surprise as the guidelines have always stated that throwing exceptions can be orders of magnitude slower (see https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance)

But, as that guidance suggests error code shouldn’t be used either - which is problematic, because HTTP responses rely on error codes.

However, the idea behind the Try-Parse Pattern could work in this instance?

A TryGetEntity and TryGetEntityAsync that doesn’t throw by default. Would that be acceptable?

This is outstanding. Thanks to everyone involved!

@christothes here’s another unpleasant side effect of needlessly throwing exceptions… our Application Insights is full of these:

That has a knock-on effect of rising costs in App Insights and/ or logs being sampled out.

This is needed because Application Insights is full of Failures when Table Storage returns 404, which is wrong IMO.

Thank you for your feedback. Tagging and routing to the team member best able to assist.