OpenPojo / openpojo

POJO Testing & Identity Management Made Trivial
http://openpojo.com
Apache License 2.0
156 stars 40 forks source link

SetConcreteRandomGenerator does not return the right SetInstance #56

Closed HenrikPilz closed 9 years ago

HenrikPilz commented 9 years ago

I have a private field which looks like

SortedSet sortedSet = newTreeSet()

now setSortedSet(SortedSet sortedSet) takes a SortedSet, but the SetConcreteRandomGenerator returns a HashSet, which is noch SortedSet ;).

HenrikPilz commented 9 years ago

I would like to contribute to your project, if you could tell me how to do that, I would appreciate it. I.e. I would fix this problem on my own, and just commit it to your project. Would that be ok? I can provide the changes I would suggest for the ServiceRegistrarTest and the SetConcreteRandomGenerator, if you like.

oshoukry commented 9 years ago

I was trying to simulate the condition you are seeing so I wrote this test, but don't see any issues.

    @Test
    public void shouldsetSortedSet() {
        PojoClass sortedSetPojoClass = PojoClassFactory.getPojoClass(AClassWithSortedSet.class);
        PojoValidator pojoValidator = new PojoValidator();
        pojoValidator.addTester(new SetterTester());
        pojoValidator.validate(sortedSetPojoClass);
    }

    public static class AClassWithSortedSet {
        private SortedSet sortedSet;

        public void setSortedSet(SortedSet sortedSet) {
            this.sortedSet = sortedSet;
            // Visually inspect the output just to ensure it is correct.
            System.out.println(BusinessIdentity.toString(sortedSet));
        }
    }

Could you please provide the following info?

What steps will reproduce the problem? 1. 2.

What is the expected output?

What do you see instead?

HenrikPilz commented 9 years ago

Thank you for your answer. Yes, I'll provide something tomorrow. The code above is not entirely what my code looks like. But I'll provide a Test like the one above. To your Questions: The expected output would be like nothing. Just that the test runs and does not fail. And I get a ReflectionException with 'argument type missmatch' But The complete stuff will come tomorrow.

HenrikPilz commented 9 years ago

So. First we come to the Test:

package com.openpojo.issues.issue56;

import org.junit.Test;

import com.openpojo.issues.issue56.sample.TestClass;
import com.openpojo.log.Logger;
import com.openpojo.log.LoggerFactory;
import com.openpojo.reflection.PojoClass;
import com.openpojo.reflection.impl.PojoClassFactory;
import com.openpojo.validation.PojoValidator;
import com.openpojo.validation.test.impl.SetterTester;

public class TestCase {
    private static Logger logger = LoggerFactory.getLogger(TestCase.class);

    @Test
    public void testSetGenerator() {
        logger.debug("-------------------------------------------------------");
        logger.debug("--- Start Test Setgenerator   -------------------------");
        logger.debug("-------------------------------------------------------");

        PojoValidator pojoValidator = new PojoValidator();
        pojoValidator.addTester(new SetterTester());

        PojoClass pojoClass = PojoClassFactory.getPojoClass(TestClass.class);

        pojoValidator.runValidation(pojoClass);

        logger.debug("-------------------------------------------------------");
        logger.debug("--- Test finished -------------------------------------");
        logger.debug("-------------------------------------------------------");
    }
}

with the following classes: TestClass

package com.openpojo.issues.issue56.sample;

import java.io.Serializable;
import java.util.SortedSet;
import java.util.TreeSet;

public class TestClass implements Comparable<TestClass>, Serializable, Cloneable {
    private static final long serialVersionUID = 1L;

    private Integer id;
    private String description;

    private SortedSet<TestSetClass> sortedTestSet = new TreeSet<TestSetClass>();

    public TestClass() {
        super();
    }

    public void setId(final Integer id) {
        this.id = id;
    }

    public Integer getId() {
        return this.id;
    }

    public void setDescription(final String description) {
        this.description = description;
    }

    public String getDescription() {
        return this.description;
    }

    public void setSortedTestSet(SortedSet<TestSetClass> sortedTestSet) {
        this.sortedTestSet = sortedTestSet;
    }

    public SortedSet<TestSetClass> getSortedTestSet() {
        return this.sortedTestSet;
    }

    /* (non-Javadoc)
     * @see java.lang.Comparable#compareTo(java.lang.Object)
     */
    public int compareTo(TestClass o) {
        if (id == null || o.id == null) {
            if (id != null) {
                return -1;
            } else if (o.id != null) {
                return 1;
            } else {
                return 0;
            }
        }
        return -2;
    }

    /* (non-Javadoc)
     * @see java.lang.Object#clone()
     */
    @Override
    protected Object clone() throws CloneNotSupportedException {
        final TestClass clonedObject = (TestClass) super.clone();
        clonedObject.setSortedTestSet(null);
        return clonedObject;
    }
}

And TestSetClass:

package com.openpojo.issues.issue56.sample;

import java.io.Serializable;

public class TestSetClass implements Comparable<TestSetClass>, Serializable, Cloneable {
    private static final long serialVersionUID = 1L;

    private Integer id;
    private String description;

    public TestSetClass() {
        super();
    }

    public void setId(final Integer id) {
        this.id = id;
    }

    public Integer getId() {
        return this.id;
    }

    public void setDescription(final String description) {
        this.description = description;
    }

    public String getDescription() {
        return this.description;
    }

    /* (non-Javadoc)
     * @see java.lang.Comparable#compareTo(java.lang.Object)
     */
    public int compareTo(TestSetClass o) {
        if (id == null || o.id == null) {
            if (id != null) {
                return -1;
            } else if (o.id != null) {
                return 1;
            } else {
                return 0;
            }
        }
        return -2;
    }

    /* (non-Javadoc)
     * @see java.lang.Object#clone()
     */
    @Override
    protected Object clone() throws CloneNotSupportedException {
        final TestSetClass clonedObject = (TestSetClass) super.clone();
        return clonedObject;
    }
}

What steps will reproduce the problem?

  1. Run the test above. :) What is the expected output? The test finishes with just saying everything's good. What do I see instead?
Failed tests:
  testSetGenerator(com.gasx.test.billing.datamodel.entity.TestEntities): argument type mismatch

If I inspect the Test with Eclipse, I first set a breakpoint for ReflectionException. The block where I end up is:

        try {
            return getAsMethod().invoke(instance, parameters);
        } catch (final IllegalArgumentException e) {
            throw ReflectionException.getInstance(e.getMessage(), e);
        } catch (final IllegalAccessException e) {
            throw ReflectionException.getInstance(e.getMessage(), e);
        } catch (final InvocationTargetException e) {
            throw ReflectionException.getInstance(e.getMessage(), e);
        }

With the following stack:

PojoMethodImpl.invoke(Object, Object...) line: 92   
PojoFieldImpl.invokeSetter(Object, Object) line: 103    
SetterTester.run(PojoClass) line: 47    
PojoValidator.runValidation(PojoClass) line: 82 
TestEntities.testSetGenerator() line: 87    

When I check the arguments for getAsMethod().invoke(instance, parameters); The argument instance is ok. But in parameters is a HashSet and not a TreeSet. I hope you can reproduce this. Thanks for your effort. :)

HenrikPilz commented 9 years ago

The simple solution from my point of view looks like the following:

/*
 * Copyright (c) 2010-2015 Osman Shoukry
 *
 *    This program is free software: you can redistribute it and/or modify
 *    it under the terms of the GNU Lesser General Public License as published by
 *    the Free Software Foundation, either version 3 of the License or any
 *    later version.
 *
 *    This program is distributed in the hope that it will be useful,
 *    but WITHOUT ANY WARRANTY; without even the implied warranty of
 *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *    GNU Lesser General Public License for more details.
 *
 *    You should have received a copy of the GNU Lesser General Public License
 *    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

package com.openpojo.random.collection.set;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.NavigableSet;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;

import com.openpojo.log.Logger;
import com.openpojo.log.LoggerFactory;
import com.openpojo.random.ParameterizableRandomGenerator;
import com.openpojo.random.RandomGenerator;
import com.openpojo.random.collection.util.CollectionHelper;
import com.openpojo.random.util.SerializableComparableObject;
import com.openpojo.reflection.Parameterizable;
import com.openpojo.reflection.construct.InstanceFactory;
import com.openpojo.reflection.impl.PojoClassFactory;

/**
 * This is random generator is responsible for generating concrete Set implementations <br>
 * <strong>Namely:</strong><br>
 * 1. HashSet <br>
 * 2. TreeSet <br>
 * 3. LinkedHashSet <br>
 * <br>
 *
 * @author oshoukry
 */
public final class SetConcreteRandomGenerator implements ParameterizableRandomGenerator {

    protected static Logger logger = LoggerFactory.getLogger(SetConcreteRandomGenerator.class);

    private final Class<?>[] TYPES = new Class<?>[] { HashSet.class, TreeSet.class, LinkedHashSet.class, Set.class,
            SortedSet.class, NavigableSet.class };

    private HashMap<Class<?>, Class<?>> mapInterfacesToTypes = new HashMap<Class<?>, Class<?>>();

    private SetConcreteRandomGenerator() {
        initInterfacesToTypes(null);
    }

    public static RandomGenerator getInstance() {
        return Instance.INSTANCE;
    }

    public void initInterfacesToTypes(String configFile) {
        if (configFile == null || configFile.isEmpty()) {
            if (mapInterfacesToTypes == null) {
                mapInterfacesToTypes = new HashMap<Class<?>, Class<?>>();
            }
            mapInterfacesToTypes.put(Set.class, HashSet.class);
            mapInterfacesToTypes.put(SortedSet.class, TreeSet.class);
            mapInterfacesToTypes.put(NavigableSet.class, TreeSet.class);
        } else {
            /**
             * @author: Henrik Pilz
             *          a more general approach would be nice here.
             *          I thought about something like an xml file to configure from outside.
             */
        }
    }

    @SuppressWarnings("rawtypes")
    public Object doGenerate(final Class<?> type) {

        Class<?> typeToGenerate = type;

        if (type.isInterface() && mapInterfacesToTypes.containsKey(type)) {
            typeToGenerate = mapInterfacesToTypes.get(type);
        }

        Set randomSet = (Set) InstanceFactory.getLeastCompleteInstance(PojoClassFactory.getPojoClass(typeToGenerate));
        CollectionHelper.buildCollections(randomSet, SerializableComparableObject.class);

        return randomSet;
    }

    public Collection<Class<?>> getTypes() {
        return Arrays.asList(TYPES);
    }

    public Object doGenerate(Parameterizable parameterizedType) {
        Class<?> setType = null;
        if (mapInterfacesToTypes.containsKey(parameterizedType.getType())) {
            setType = mapInterfacesToTypes.get(parameterizedType.getType());
        }

        Set initialSet = new HashSet();
        if (setType != null) {
            try {
                initialSet = (Set) setType.newInstance();
            } catch (InstantiationException e) {
                logger.error("Could not instantiate the setType [{0}]", setType);
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                logger.error("Could not find the setType [{0}]", setType);
                e.printStackTrace();
            }
        }
        CollectionHelper.buildCollections(initialSet, parameterizedType.getParameterTypes().get(0));
        return initialSet;
    }

    private static class Instance {
        private static final RandomGenerator INSTANCE = new SetConcreteRandomGenerator();
    }
}

At least that works for the TestCase from above.

The main changes are in the two doGenerate methods. It's just a quick fix. Maybe you will come up with something better.

oshoukry commented 9 years ago

Most excellent find... Thank you for the test case. The fix has been checked in and will be part of the upcoming release. See fix https://github.com/oshoukry/openpojo/commit/8e268190c7ca541495e678d2ce1ac14128ea01a6