Bad HashCodeCacheKeyGenerator implementation

[GoogleCodeExporter]

We've just upgraded from springmodules-cache 0.8a to 1.1.2 and faced the 
problem that new implementation of HashCodeCacheKeyGenerator is "not nice" 
comparing with old one.

Try this test. It fails.

@Test
public void testBug() {
        HashCodeCacheKeyGenerator generator = new HashCodeCacheKeyGenerator();

        HashCodeCacheKeyGenerator.LongGenerator gen = generator.getGenerator(new Long(429), "57");
        HashCodeCacheKeyGenerator.LongGenerator gen2 = generator.getGenerator(new Long(430), "47");

        Assert.assertNotSame(generator.generateKey(gen),
                             generator.generateKey(gen2));
}

Original issue reported on code.google.com by gray.b...@gmail.com on 14 Dec 2010 at 10:11

[GoogleCodeExporter]

That is a danger in using a simple hash code algorithm. The List class in Java 
exhibits the same behavior:

groovy:000> l1 = new LinkedList();
===> []
groovy:000> l1.add(429l);
===> true
groovy:000> l1.add("57");
===> true
groovy:000> l1.hashCode();
===> 15958
groovy:000> l2 = new LinkedList();
===> []
groovy:000> l2.add(430l);
===> true
groovy:000> l2.add("47");
===> true
groovy:000> l2.hashCode();
===> 15958

If collisions like this are possible in your code please change to one of the 
other key generation classes. If you still want to use hashes for keys the 
MessageDigestCacheKeyGenerator will not be susceptible to this problem.

We're also considering changing the default key generator to the 
StringCacheKeyGenerator for the 1.2 release.

Original comment by eric.dalquist on 14 Dec 2010 at 2:25

• Changed state: WontFix
• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

This is very very dangerous. Plz change the implementation to

/**
 * @author Eric Dalquist
 * @version $Revision: 454 $
 */
public class HashCodeCacheKeyGenerator extends 
AbstractHashingCacheKeyGenerator<HashCodeCacheKeyGenerator.LongGenerator, Long> 
{

    /**
     * Little utility class to fake a mutable long
     */
    public static class LongGenerator {
        protected static final long [] MULTIPLIERS = {31, 37, 41, 43, 47, 53, 59, 61, 67, 71};
        private long hash = 1;
        private int index = 0;

        private LongGenerator() {

        }

        public void append(long hash) {
            this.hash = MULTIPLIERS[index++ % MULTIPLIERS.length] * this.hash + hash;
        }
    }

    /**
     * @see com.googlecode.ehcache.annotations.key.AbstractCacheKeyGenerator#AbstractCacheKeyGenerator()
     */
    public HashCodeCacheKeyGenerator() {
    }

    /**
     * @see com.googlecode.ehcache.annotations.key.AbstractCacheKeyGenerator#AbstractCacheKeyGenerator(boolean, boolean)
     */
    public HashCodeCacheKeyGenerator(boolean includeMethod, boolean includeParameterTypes) {
        super(includeMethod, includeParameterTypes);
    }

    @Override
    public LongGenerator getGenerator(Object... data) {
        return new LongGenerator();
    }

    @Override
    public Long generateKey(LongGenerator generator) {
        return generator.hash;
    }

    @Override
    protected void append(LongGenerator generator, boolean[] a) {
        for (final boolean element : a) {
            generator.append(element ? 1231 : 1237);
        }
    }

    @Override
    protected void append(LongGenerator generator, byte[] a) {
        for (final byte element : a) {
            generator.append(element);
        }
    }

    @Override
    protected void append(LongGenerator generator, char[] a) {
        for (final char element : a) {
            generator.append(element);
        }
    }

    @Override
    protected void append(LongGenerator generator, double[] a) {
        for (final double element : a) {
            generator.append(Double.doubleToLongBits(element));
        }
    }

    @Override
    protected void append(LongGenerator generator, float[] a) {
        for (final float element : a) {
            generator.append(Float.floatToIntBits(element));
        }
    }

    @Override
    protected void append(LongGenerator generator, int[] a) {
        for (final int element : a) {
            generator.append(element);
        }
    }

    @Override
    protected void append(LongGenerator generator, long[] a) {
        for (final long element : a) {
            generator.append(element);
        }
    }

    @Override
    protected void append(LongGenerator generator, short[] a) {
        for (final short element : a) {
            generator.append(element);
        }
    }

    @Override
    protected void appendGraphCycle(LongGenerator generator, Object o) {
        generator.append(0);
    }

    @Override
    protected void appendNull(LongGenerator generator) {
        generator.append(0);
    }

    @Override
    protected void appendHash(LongGenerator generator, Object e) {
        if (e instanceof Double) {
            generator.append(Double.doubleToLongBits(((Double)e).doubleValue()));
        }
        else if (e instanceof Long) {
            generator.append(((Long)e).longValue());
        }
        else {
            generator.append(e.hashCode());
        }
    }
}

Original comment by bernd.zu...@gmail.com on 17 Apr 2012 at 1:03

• Added labels: ****
• Removed labels: ****