square / connect-csharp-sdk

C# client library for the Square Connect APIs
https://docs.connect.squareup.com/
Apache License 2.0
27 stars 25 forks source link

Response classes seem unnecessary #130

Closed Alfetta159 closed 4 years ago

Alfetta159 commented 5 years ago

Most Response classes aggregate a specific response property and an error property where only one will be used. Why not simply return the response or throw an exception like the older endpoints do?

With the result/error response, it forces the user to either have to check for an error or risk creating a NullReferenceException which is far less helpful than the ApiException. And if the dev does check for an error, they might be able to handle it there, but if not, then they'll probably just throw an exception.

StephenJosey commented 5 years ago

The current behavior is to actually throw an exception; I don't believe the error field is ever used. As an example using the V2 Locations API:

LocationsApi locationsApi = new LocationsApi();
try
{
   ListLocationsResponse locations = locationsApi.ListLocations();
  Console.Write(locations.Locations[0]);
} catch (ApiException e)
{
  Console.Write(e.Message);
}

and failing to supply an access token would result in the following: Error calling ListLocations: {"errors":[{"category":"AUTHENTICATION_ERROR","code":"UNAUTHORIZED","detail":"This request could not be authorized."}]} so the exception is thrown and caught, and the response actually never uses the error field.

Please let me know if I'm misunderstanding here, or if you have further questions about this.

Alfetta159 commented 5 years ago

If the error field is never used, then why is it there, and why is it aggregated with the actual data from the response? Why not simply make it like this, the way many 'List' methods are set up? (e.g. ListItems)

var locationsApi = new LocationsApi();

try
{
  List<locations> locations = locationsApi.ListLocations();
  Console.Write(locations[0]);
} catch (ApiException e)
{
  Console.Write(e.Message);
}

A better way to put it: If you look at the original code snippet, If the result of the ListLocations call returns a ListLocationsResponse object with a value for the Error member and null for the Locations member, then this will throw a NullReferenceException and the catch will ignore it.

LocationsApi locationsApi = new LocationsApi();
try
{
   ListLocationsResponse locations = locationsApi.ListLocations();
  Console.Write(locations.Locations[0]);
} catch (ApiException e)
{
  Console.Write(e.Message);
}
 catch (NullReferenceException e)
{
  Console.Write("Did you check the Error property first before you dereferenced the Locations property?");
}

The documentation for the ListLocationsResponse says:

Defines the fields that are included in the response body of a request to the ListLocations endpoint. One of errors or locations is present in a given response (never both).

For the record, I admit that I should go and run the code and force a bad call that creates the error condition, but regardless, it just seems that it should be returning good data or throwing some kind of exception, but not returning error data.

StephenJosey commented 4 years ago

Sorry for the delayed response here. I agree, the documentation is confusing. In REST calls, there is either errors or locations. I'm relaying this over to our SDK team to make sure it gets properly addressed.

StephenJosey commented 4 years ago

I'll close this issue for now. Please take a look at the new .NET SDK as this one is now deprecated.

havenwood commented 4 years ago

You can find the new Square .NET SDK in the square/square-dotnet-sdk repo.