How To Find Bugs, Part 4: Design -> Analysis -> Design

At LMAX, we have a bit of a case of primitive obsession. Many datatypes are represented internally with longs: prices and quantities (in a fixed-point representation), contract sizes, and IDs for more or less everything (customers, instruments, all sorts of things).

This is a deliberate tradeoff: while we understand the benefits of a richer set of types, we care a great deal about performance, and primitives are good for performance. Comparisons and (where appropriate) maths are incredibly cheap, and primitives avoid garbage collection issues. Specialized collection classes let us build maps which don’t require an object allocation for entries, and so on…

This is particularly important in the core of our system, but of course it permeates through all the layers, up to the UI elements, and that’s where we hit a minor snag. We build our APIs using Json, parsing responses and generating requests via Gson.

The problem here is what happens to those longs when we convert objects to Json. Longs, of course, are numbers, so we generate Json with that field as a number.

Javascript only has one number type, and it’s double-precision floating point. Double-precision floating point can’t exactly represent all the values a long can – which means it’s possible that the long (backend) -> double (client) -> long (backend) conversion processes involved in loading and modifying something could come back with a slightly different value to the one we sent out.

If that number was representing an account id, that would be Very Bad.

So, let’s break down the problem. Conversions from Java types to Json can be lossy: we know that can happen with longs, can it happen with other types? The Json datatypes are Number, String, Boolean, Array, Object, and null.

Strings are simple enough, as are booleans and nulls: these map consistently.

Of all the primitive number types, obviously double and float can be represented accurately by a double, and so (it turns out) can all integer types of 32 bits or less. Long’s the only primitive we need to worry about.

Arrays represent any collection, and Objects any map or arbitrary object: between them, they cover literally anything else we can think of.

So it’s just longs (and boxed Longs) we’re worried about, right? Well, no, but let’s assume we are for a moment and I’ll cover the other cases later.

We’ve got a little bit of homegrown framework around our web APIs, in which any response object implements the marker interface Response, which means we can describe our proposed rule as follows:

If a class is a Response, and it has a (non-static) field that’s a long or a Long, that’s a bug.

OK, so what does that look like? Eliding fields/constructors because they’re noise, and eliding some unimplemented methods for suspense, the meat looks like:

public class LongInJsonDetector extends BytecodeScanningDetector
{
    @Override
    public void visitField(final Field field)
    {
        if(isResponseClass() &&
           !field.isStatic() &&
           !isLong(field))
        {
            bugReporter.reportBug(...);
        }          
    }
}

Fairly simple. That opens up two questions: how do we check if the class we’re in is a response class, and how do we check if the field is a long? Let’s handle the latter first.

Ideally, we’d get the java.lang.Class of the field, and compare it to long.class or java.lang.Long.class. Barring that, we’d find some common type with a sensible equals() implementation we can convert both Class and Field to, and then we can see what we have. I haven’t found the former, but it’s possible to do the latter by using the (String) signature of a Field to construct a ClassDescriptor:

public boolean isLong(field) 
{
    ClassDescriptor fieldD = DescriptorFactory.createClassDescriptorFromFieldSignature(field.getSignature());
    ClassDescriptor longD = DescriptorFactory.createClassDescriptor(long.class);
    ClassDescriptor LongD = DescriptorFactory.createClassDescriptor(Long.class);

    return (fieldD.equals(longD.class) || fieldD.equals(LongD.class));   
}

That’s not too horrible. What about checking if we’re in a response class? Well, first we get the class we’re in, and then we just check if that class is an instance of Response.

public boolean isResponseClass()
{
    ClassDescriptor response = DescriptorFactory.createClassDescriptor(Response.class);
    return isInstanceOf(getXClass(), response);
}

Unfortunately, there’s no instanceOf method on XClass, so we need to roll our own. Lovely. How do we work that out? Well, it’s an instance of Response if it is itself Response, if it implements the Response interface, or if its superclass is an instance of Response. Checking if the class is itself Response is pretty simple:

public boolean isInstanceOf(XClass clazz, ClassDescriptor target)
{
    if(clazz.getClassDescriptor().equals(target))
    {
        return true;
    }
    ...
}

And we can look at its interfaces simply:

public boolean isInstanceOf(XClass clazz, ClassDescriptor target)
{
    ...
    for(ClassDescriptor i: clazz.getInterfaceDescriptorList())
    {
        if(i.equals(target)) 
        {
            return true;  
        }
    }
    ...
}

And looking at the superclasses isn’t too tricky either (eliding exception checking and other tedium):

public boolean isInstanceOf(XClass clazz, ClassDescriptor target)
{
    ...
    // could be null, if we passed in an interface or Object
    ClassDescriptor supC = clazz.getSuperclassDescriptor();
    if(supC != null)
    {
        return isInstanceOf(supC.getXClass(), target);
    }
    ...
}

And if none of those are the case, it’s false. So summing up:

public boolean isInstanceOf(XClass clazz, ClassDescriptor target)
{
    if(clazz == null)
    {
        return false;
    }

    if(clazz.getClassDescriptor().equals(target))
    {
        return true;
    }

    for(ClassDescriptor i: clazz.getInterfaceDescriptorList())
    {
        if(i.equals(target)) 
        {
            return true;  
        }
    }

    // could be null, if we passed in an interface or Object
    ClassDescriptor supC = clazz.getSuperclassDescriptor();
    if(supC != null)
    {
        return isInstanceOf(supC.getXClass(), target);
    }
    
    return false;
}

Well, it’s a bit annoying that we had to reinvent that wheel, but never mind: that’ll do the trick.

So that’s our detector done, right? Well… no, not quite. That’ll catch any long/Long fields in a Response object, but it won’t catch Collection or Map or domain class fields which contain Long and so on. Hopefully I’ll get to those in the next post soon…

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s