Wednesday, March 25, 2015

One sign of a really bad API

By Steve Endow

The last several months, I've been working with a third party product that integrates with Dynamics GP.

While the product sometimes technically works, it is absolutely riddled with bugs.  These aren't subtle bugs where you need to perform 26 steps with specific data in perfect sequence to trigger them.  These are glaring, obvious, brick-wall-in-the-middle-of-the-road bugs that will cause an error with the simplest data entry example.  In 10 minutes I found a half dozen bugs in the product--and that was on just 1 window.

The product has a web service that serves as the back end to their Dynamics GP integration--so their custom GP windows call the web service, rather than calling DLLs.  That web service also serves as the API if you want to integrate with their product.  Great, makes sense.

After working through numerous bugs in the product, we finally got it working and I got my integration working with it as well.  All seemed peaceful.

Until one day we noticed that some records that were sent to the product through my integration weren't in the database.  Records existed in GP, but didn't exist in the third party product tables.

After being puzzled for several minutes, we eventually tried to manually enter the same data into the product window in GP.  We entered the necessary data and clicked on Save.

So the problem wasn't my integration per se, it was that this particular data was failing to save.  We looked at the data, pondered for a minute, and saw that the data in one of the fields was relatively long--51 characters.

Curious, we made that value shorter and clicked Save.  The save was then successful.  I then opened the diagnostic logs for the web service and saw that it had logged an error.
String or binary data would be truncated
   at System.Data.SqlClient.SqlCommand.ExecuteNonQuery()
I then checked the SQL table, and sure enough, the field length was 50 characters.  Our 51 character field value was triggering the error.

Developers should immediately recognize the issue, and alarm bells should be ringing.

This tells us that the developer of the web service API is not validating input values or lengths in their API when they attempt to save data to their own tables.  So if a field is 50 characters and I submit 51 characters, their method will fail with a SQL exception.

Digging through the log file, it looks like they are building SQL statements by concatenating a string.

INSERT INTO SomeTable (TransactionType, Amount, TransactionTime, IsProcessed, CurrencyId, InvoiceNum, PurchaseOrderNum, SubmittedBy, CompanyId, CustomerId, CompanyName, AgreementId)
VALUES (1,10.99,1,'3/23/2015 2:37:08 PM',2,1,1, 'STDINV3588', '', '1', 'sa','Test Company','','-1','TI TESTCO100',2,2,'Test Company', 0)

While there might be some rare situations where I build a SQL statement like this, I do it reluctantly as an exception, and I never do it with user generated data.  The potential for errors from unvalidated data is bad enough, not to mention issues like SQL injection.

What they should be doing is something like this:

SqlParameter[] sqlParameters = new SqlParameter[2];
sqlParameters[0] = new SqlParameter("@CustomerID", System.Data.SqlDbType.VarChar, 15);
sqlParameters[0].Value = gpCustomerID.Trim();
sqlParameters[1] = new SqlParameter("@CompanyID", System.Data.SqlDbType.Int);

sqlParameters[1].Value = gpCompanyID;

In this example, my code is building parameters for a SQL command and is defining data types and maximum value lengths.  Customer ID must be a VarChar of no more than 15 characters, and Company ID must be an integer.  This doesn't handle all of the potential problems of someone submitting a 20 character customer ID (which should be caught and handled earlier in the process), but it at least prevents a SQL truncation exception.

This isn't rocket science and isn't a developer secret.  It's pretty basic .NET development.

After finding this issue, I had to modify my integration code to truncate input values before submitting them to the web service API.

All because the developer of an API for a "commercial" software product that is being sold to Dynamics GP customers for thousands of dollars doesn't know how to validate inputs.  And the "QA department" clearly didn't do a whole lot of testing.

Developer, please pick up the chalk and start writing.

I make coding mistakes all the time, but this clearly isn't a typo or a small mental lapse.  It's a fundamental design issue in the code that correlates well with all of the bugs we have found in the product.

On the plus side, this developer is making me feel like a coding genius.

Steve Endow is a Microsoft MVP for Dynamics GP and a Dynamics GP Certified IT Professional in Los Angeles.  He is the owner of Precipio Services, which provides Dynamics GP integrations, customizations, and automation solutions.

You can also find him on Google+ and Twitter

No comments: