The SRP And How I F****d It

This blog gives an introduction to the single responsibility, then gives a real life case study of how I broke the SRP and consequently caused a bug.

We all know the S in SOLID stands for 'Single Responsibility principle' (SRP). The aim of this blog is to demonstrate a real-world example of a poor design choice I made that broke the principle and how it caused problems down the line.

Classes (or other entities such as methods or namespaces) respecting the SRP are designed to do one thing only, and to do it well. For example the .NET StringBuilder class is really good appending strings together without excess heap allocation but is really dreadful at any other task - it isn't going to make you a web request or write to any files for you.

Sitting here at a coffee shop (shamelessly a chain) i phased out for a little and overheard the barista tell a customer 'i stick with what i know', sticking with what he knows, being himself, only doing the one thing of being himself, that’s the SRP in a nutshell. I inspected the chairs and the tables and the mugs and marvelled at how good at their jobs they were. There was no green tea leaking from my mug, nothing falling off my table and my chair was comfy and at a good height. If only code could be this bliss - everything was doing one job and doing it well. Sure one can break the SRP by standing on a chair, but you wouldn’t catch me buying a table because of how good it is to stand upon. Real world objects are built for one thing, it just feels natural. A large part of OOP is modelling real world objects in our code, thus a natural desire for single responsibility emerges.

Some other benefits of the SRP are that they let you keep a tickbox in your mind while you're testing code - test one class for its one responsibility and we can tick that class off as OK in our mind. Breaking the principle could lead to situations where we feel we've fully tested a class because it sure as shit does what it says on the tin but forget about the extra salt that was added in a hurry... this sounds all too familiar. Enter the real-world example as promised, concerning an integration with a third party payment processor (Ingenico - TODO add link to docs).

The requirement for a payment uri looks like the following:

• A base uri (e.g. http://www.paymentprocessor.com/makeapayment)
• Query string keys/values taken from a possible known set (e.g. AMOUNT=100&CURRENCY=GBP
• Include a HASH parameter, which is a hash of the query string in alphabetical order, prepended with a shared api secret.

Here's an example:

http://www.paymentprocessor.com/makeapayment?AMOUNT=100&CURRENCY=GBP&PASSWORD=testapipassword&USER=testapiuser&HASH=0fb6d…

If the shared secret between us and the payment processor is 'SECRET' the string to hash would be

SECRETAMOUNT=100&CURRENCY=GBP&PASSWORD=test&apipassword&USER=test&apiuser

I thought using a builder pattern was in order for this class, here was the barebones of my first attempt...

This seems mostly kosher to me, the only thing the PaymentUriBuilder can do is build up a URL according to the business rules. We all know there’s a myriad of alternative designs, but they aren’t the focus of this blog post anyways - what happened next is.

While I was testing the system as a whole I decided that there should probably be some informational logging whenever customer was redirected to a URL generated by my class. The logging in itself is trivial to add - just remember the URL that was generated and send it off to the logging module. Piece of cake. Here’s how it looked:

Can you spot the issue yet? Here’s how the log entry looked:

06/09/2019 15:08:00

Informational

Generated Payment Page URL: 'http://www.paymentprocessor.com/makeapayment?AMOUNT=500&CURRENCY=GBP&PASSWORD=testapipassword&USER=testapiusername&HASH=a7f89c...'

Yes I’d royally fucked up - logging passwords is a sure fire way to at least get a slap on the wrist. Customer details would also be included in these URLs - names, addresses, emails - logging these would be a great way to get some first hand experience with GDPR regulations of which we're all trying our best to adhere.

Thankfully I spotted the blunder before passing project onto testing, so I searched my heart for a couple of potential approaches to remove sensitive information from the URL.

1 - Only log selected parameters

This was the first approach I took and is probably the quickest to implement. I changed the logging to resemble something like the following:

Logger.AddInformational($"Generated Payment Page URL - Base URL: '{paymentUrl}' Amount: '{paymentMode.Amount}' Currency: '{paymentModel.CurrencyCode}'"); // etc

On reflection I don't know how I ever favoured this method of logging the URL; we've snookered ourselves into adding code duplication. For the logging code to make sense, it MUST access a subset of the properties that the URL generation code accesses. In our example we're accessing the Amount and CurrencyCode properties multiple times, once when building the builder object and once when logging the payment request. Our intra-statement dependency is a real liability, consider in the future we have to send a full currency name rather than just the code to the payment processor - it would be all too easy to change

.Append("CURRENCY", paymentModel.CurrencyCode)

... to ...

.Append("AMOUNT", paymentModel.CurrencyName)

… then completely forget to change the logging code because we were being badgered to hurry up and accompany a friend to our local dining establishment of choice, leading to conflicting actual behaviour and logged behaviour.

Another issue with the logging code is that some parameters would always be absent, even if they were part of the generated URL. Imagine getting a call at 1am because every single payment returns an authentication failure. What are some of the first questions you might ask yourself outside of the generic 'What's changed? Is the API down? Etc'. I'd be willing to bet many would ask ‘Are we even sending credentials in the first place?' As logging option 1 only logs a subset of parameters there's no reliable way to tell. This sounds like an impending visit from a tired and frustrated friend at my desk the next morning, asking if I could please log as much as possible next time…

I can do better and my clients and colleagues deserve better, I thought.

2 - Redact sensitive parameters

This option involves maintaining a list of sensitive parameter keys such as...

• Password
• Customer Name
• Customer Email
• Customer Address

… which would be redacted from any URLs that were to be logged. Where this list should be maintained is a whole other discussion, should it be injected into the builder? Should each parameter know its sensitivity (e.g. use )? I decided on keeping the list inside the builder class itself since it is responsible for the business rules of generating a URL it doesn’t feel too wrong that it ought to know which parameters are sensitive.

Here’s the new additions to the builder class:

Now we can easily log the redacted url with a few code tweaks:

The log entries should be safe now, example

'Generated Payment Page URL: 'http://www.paymentprocessor.com/makeapayment?AMOUNT=500&CURRENCY=GBP&PASSWORD=XXX&USER=testapiusername&HASH=a7f89c...'

I was (inexplicably) happy with this, I don't think I tested the new logging at all because by this point I was feeling like the world's most human human rather than the world's best programmer like normal. It's only logging I thought to myself so I promptly forgot about the new class functionality and eventually passed the project along for internal testing.

Can you spot the issue yet?

Internal testing came and went and the project was released for user acceptance testing. Those pesky users are always thinking outside the box so inevitably I had to do some investigation on why some payments were failing. "I wonder what the payment requests look like" I said to myself, feeling a little smug I'd gone to the extra effort of adding all this logging because it was paying off already. So I had a poke around...

06/09/2019 15:08:00

Informational

Generated Payment Page URL: 'Generated Payment Page URL: 'http://www.paymentprocessor.com/makeapayment?AMOUNT=500&CURRENCY=GBP&XXX=testapipassword&USER=testapiusername&HASH=a7f89c...'

Why is it always MY code that’s the problem?

When you see the bug in action it's obvious, the problem lies in the string replacement:

tempSafeQueryString = tempSafeQueryString.Replace(sensitiveParameter, "XXX");

Each sensitiveParameter is a parameter key. The sensitive information is in the parameter values. Oh bother.

So I used some fancy LINQ to fix the problem for a third time and I think it works now, the fix was to do the redacting while pulling values out of the query string dictionary. It isn’t pretty but does the job:

Needless to say I wasn’t happy. How hadn’t I noticed this? Should this have been picked up in testing? Depends how you define should. This type of bug could have been spotted with a simple unit test, but sadly the project in question didn’t have unit tests and now is never the time to start adding them.

I think real issue with my PaymentUriBuilder class is that it tries to do too much. The class is simultaneously responsible for generating the payment URLs AND responsible for redacting data from a data structure. Redacting data is easy enough to do, I thought to myself, so probably doesn’t require anything fancy like a new class - that’s the moment I shot myself in the foot. That’s the moment I abandoned the single responsibility principle. Just because responsibilities are simple it doesn’t stop them from being distinct. Cutting and stabbing food are simple but we prefer to eat with a knife and fork rather than a knork.

The two tasks can and should have dedicated classes. In this case I would suggest a custom data structure is in order, who’s only responsibility is to provide two views to a shared set of a key/value pairs - a redacted view and a sensitive view. When we create an API URL to visit we can use the sensitive view, when we create a logging string we can use the redacted view. Here’s a quick example:

This could be consumed by the PaymentUriBuilder class as follows.

Our class is now one step closer to SOLID perfection - we’ve abstracted away the redaction logic. I think if I had made the same redaction mistake again in the class observing the SRP it could have been caught more easily - there’s an obvious need to test a class dicking around dictionaries.

I’ve heard many people state that the SRP is one of the most important principles to follow when writing code - I learned this the hard way so hopefully you don’t have to.

---