Support type variables

[ollik1]

Consider the following example:

abstract class Base<T> {
    T t;
}
class Concrete extends Base<String> {}

Currently easyRandom.nextObject(Concrete.class) would generate t as java.lang.Object which will then cause a runtime exception when used. The actual types are preserved in classes so that could be used to pick the correct randomizer instead.

[sin-dm]

Hello! What are the deadline for closing this issue?
Now I have this troubles when generating child objects. This issue must solve my problem

[fmbenhassine]

Duplicate/Similar to #375 , #242 and #386.

@ollik1 I would like to get your feedback on #426 (comment) before closing this as duplicate.

[seregamorph]

@benas if you have time you can check the intermediate state.
To simplify your review - it is separated into two parts:
First pivot - https://github.com/j-easy/easy-random/compare/master...seregamorph:generic-base-class?expand=1
it is a fast-forward of #426 (currently outdated) and latest origin/master. The build of this branch is success (note: it does not have new test cases).
Second - the fix of #426 that now supports intermediate classes to resolve generic types (new tests added). The logic is based on spring-framework spring-core org.springframework.core.ResolvableType.forField(Field, Class) method. All related logic copied from spring project (making spring-core dependency looks weird even if shade included), then step-by-step all unused methods were made private or deleted. You can assume that there is too many new lines, but you can simplify your review this way: compare original spring source and copied class. The order of methods is the same, only caches were removed.
The good thing about this implementation: it is promising. Please check the test ResolvableTypeTest, it covers the second case that you described (but still the implementation of random is incomplete) - generic collections of fields. I need a few more days, but there is a chance that I can handle it. Just wanted to know your opinion about the approach.

[seregamorph]

The implementation: https://github.com/seregamorph/easy-random/compare/generic-base-class...seregamorph:generic-base-class-spring-core-no-spring?expand=1 (it's too early to raise the PR, so the delta link)

[seregamorph]

One more implementation based on fasterxml-classmate dependency https://github.com/seregamorph/easy-random/compare/generic-base-class...seregamorph:generic-base-class-classmate?expand=1
The idea of using new dependency does not sound bad - it's pretty small, also it is used by fastxml jackson, that is extremely popular. But there are two tests that are failing: SeedParameterTests, probably because it relies on constant field order (either test should be changed, or in theory we still can satisfy the expected order) and ChainedSettersSupportTest.chainSettersWithOverriddenFieldShouldBeRandomized, because field override is not supported.

[fmbenhassine]

@seregamorph Thank you for this effort! However, I'm not in favor of copying code from Spring Framework (and that's a lot of code already). Moreover, those changes are based on #426 for which I already explained my concerns here.

I believe #439 is a middle ground that covers common use cases (it covers all tests suggested in #426 and more), so in my opinion, this is a good start for v4.3 which we can improve in subsequent iterations. Wdyt?

EDIT: I didn't see you previous message when I posted this message. I don't think we need a new dependency for this. We should aim at starting small and keeping it simple for now. We can always iterate on this and improve things in subsequent releases.

[seregamorph]

Well, I missed message about raised #439, sorry.

It covers some cases, that's true, but I have failures on my model.
As I mentioned, now I have generic bindings like .randomize(Serializable.class, random.nextInt(100)), where Serializable is the topmost generic type. If I remove it (like I did in my implementations), there are failures. I still can live with this as it does not break old agreements of 4.2.0.

Please check these test cases - a bit more complicated inheritance and cross-references with generic types. All three fail on your implementation (passes on both mine implementations ;) )

import static org.assertj.core.api.Assertions.assertThat;

import java.io.Serializable;
import org.junit.jupiter.api.Test;

public class GenericTest {

    @Test
    void genericFirstTypeShouldBeCorrectlyPopulated() {
        // given
        EasyRandom easyRandom = new EasyRandom();

        // when
        LongResourceFirstType longResource = easyRandom.nextObject(LongResourceFirstType.class);

        // then
        assertThat(longResource.getId())
                .isInstanceOf(Long.class);
        assertThat(longResource.stringResource.getId())
                .isInstanceOf(String.class);
    }

    @Test
    void genericSubShouldBeCorrectlyPopulated() {
        // given
        EasyRandom easyRandom = new EasyRandom();
        class Sub extends LongResourceFirstType {
        }

        // when
        Sub sub = easyRandom.nextObject(Sub.class);

        // then
        assertThat(sub.getId())
                .isInstanceOf(Long.class);
    }

    @Test
    void genericSecondTypeShouldBeCorrectlyPopulated() {
        // given
        EasyRandom easyRandom = new EasyRandom();

        // when
        StringResourceSecondType stringResource = easyRandom.nextObject(StringResourceSecondType.class);

        // then
        assertThat(stringResource.getId())
                .isInstanceOf(String.class);
        assertThat(stringResource.longResource.getId())
                .isInstanceOf(Long.class);
    }

    private static abstract class IdResourceFirstType<K, T extends IdResourceFirstType<K, ?>> {

        private K id;

        @SuppressWarnings("unchecked")
        public T setId(K id) {
            this.id = id;
            return (T) this;
        }

        public K getId() {
            return id;
        }
    }

    private static abstract class IdResourceSecondType<T extends IdResourceSecondType<?, K>, K extends Serializable> {

        private K id;

        @SuppressWarnings("unchecked")
        public T setId(K id) {
            this.id = id;
            return (T) this;
        }

        public K getId() {
            return id;
        }
    }

    private static abstract class IntermediateParent<K extends Serializable, T extends IntermediateParent<K, ?>>
            extends IdResourceFirstType<K, T> {
    }

    private static class LongResourceFirstType extends IntermediateParent<Long, LongResourceFirstType> {
        private StringResourceSecondType stringResource;
    }

    private static class StringResourceSecondType extends IdResourceSecondType<StringResourceSecondType, String> {
        private LongResourceFirstType longResource;
    }
}

If you are fine with these failures - it's up to you do decide.

P.S. still confused why you don't like to include code made by @vmware fellas ;)

[fmbenhassine]

Please check these test cases - a bit more complicated inheritance and cross-references with generic types.

I'm not expecting Easy Random to support types like:

class IdResourceSecondType<T extends IdResourceSecondType<?, K>, K extends Serializable>` {}
//and 
class IntermediateParent<K extends Serializable, T extends IntermediateParent<K, ?>> extends IdResourceFirstType<K, T> {}

out-of-the-box. That's not the goal in the first place. The goal is to cover basic use cases and give users the necessary extension points to customize/extend the base functionality. This is already the case in Easy Random with custom randomizers, RandomizerProvider, ObjectFactory, etc. And I think you would agree with me on that since you said you were able to cover your cases with .randomize(Serializable.class, random.nextInt(100)).

P.S. still confused why you don't like to include code made by @vmware fellas ;)

Let me clarify this confusion. It's not about the quality or whatever, it is because the amount of code you are suggesting to copy (ResolvableType, SerializableTypeWrapper, SpringReflectionUtils, ObjectUtils) is really significant for the added value (which is covering 20% of use cases that the average user might not need and which should not be handled by the library by design, those should be left to the user with custom extension points). So the cost/benefit ratio here is really high. Moreover, I'm not in favor of copying code in general because copies can quickly diverge, and the copy will immediately become a maintenance burden on our side.