Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generation fails for generic enums #242

Closed
reitzmichnicht opened this issue Feb 23, 2017 · 18 comments
Closed

Generation fails for generic enums #242

reitzmichnicht opened this issue Feb 23, 2017 · 18 comments

Comments

@reitzmichnicht
Copy link
Contributor

reitzmichnicht commented Feb 23, 2017

Here is a unit test for the error:

	@Test
	public void genericEnumTest() {
		FooBar dto = EnhancedRandom.random(FooBar.class);
	}

	public class BarDto<T extends Enum<T>> implements Serializable {
		private T status;

		public T getStatus() {
			return status;
		}

		public void setStatus(T status) {
			this.status = status;
		}
	}
	
	public class FooBar extends BarDto<FooEnum> {
	}
	
	public enum FooEnum {
		DEFAULT
	}

The test fails with this message:
io.github.benas.randombeans.api.ObjectGenerationException: Unable to generate a random instance of type class $FooBar
...
Caused by: io.github.benas.randombeans.api.ObjectGenerationException: Unable to create type: java.lang.Enum for field: status of class: $FooBar
...

@PascalSchumacher
Copy link
Collaborator

Thanks for reporting.

I believe due to type erasure it is not possible to figure out what type should be stored in the status field.

I would love to be proven wrong. :)

@PascalSchumacher
Copy link
Collaborator

Of course you can help random-beans figure out what to do. For example with:

@Test
public void genericEnumTest() {
  EnhancedRandom enhancedRandom = EnhancedRandomBuilder.aNewEnhancedRandomBuilder()
      .randomize(new FieldDefinition("status", Enum.class, BarDto.class), new EnumRandomizer(FooEnum.class)).build();
  FooBar dto = enhancedRandom.nextObject(FooBar.class);
}

@reitzmichnicht
Copy link
Contributor Author

reitzmichnicht commented Feb 24, 2017

Hi Pascal,

thanks for your answers and the example. This is a part where java really sucks compared to other generic concepts like c#
I'm wondering why this also doesn't work:

public class FooBar extends BarDto<FooEnum> {
		@Override
		public FooEnum getStatus() {
			return super.getStatus();
		}

		@Override
		public void setStatus(FooEnum status) {
			super.setStatus(status);
		}
	}

Btw: Great library that is a good java counterpart to the famous C# AutoFixture

@PascalSchumacher
Copy link
Collaborator

Hi reitzmichnicht,

thanks for your kind words. :-)

Random-Beans does work directly on fields (by-passing setters/getters). This means for Random-Beans your new example is the same as the old one (it still can not identify the actual type of the status field).

I think reified generics are on the roadmap for java 10.

@fmbenhassine
Copy link
Member

fmbenhassine commented Feb 24, 2017

Hi @reitzmichnicht

Thank you for reporting this. As Pascal said, with type erasure it is a bit hard and tricky to get the actual type at runtime. I remember having some hair pulling sessions with that early in this project. It all started in issue #19 (I was invited to write about it here).

Anyway, I think It's still possible to randomize generic enums (This test is passing in branch issue-242). But I'm not sure random beans should cover all possible types configurations. For specific needs, we can help random beans by registering a custom randomizer as Pascal suggests.

@PascalSchumacher Thank you for the follow up. What do you think of the fix in branch issue-242 ? I still have to update all tests.

Kind regards
Mahmoud

@PascalSchumacher
Copy link
Collaborator

Hi @benas

glad you proved me wrong. 😄

While I was aware it is possible to get the type parameter from the class, I did not think that adding this to random-beans would be (so) easy.

As parameterizing a class like this is not limited to enums, I believe generalizing this could be helpful when used in combination with classpath scanning.

@fmbenhassine
Copy link
Member

glad you proved me wrong. 😄

Oh sorry, not intended. But you asked for it right? 😄

As parameterizing a class like this is not limited to enums, I believe generalizing this could be helpful when used in combination with classpath scanning.

Indeed. But still need some more work and more importantly additional tests (because I'm not sure the current patch would work for deep hierarchies..)

@PascalSchumacher
Copy link
Collaborator

But you asked for it right? 😄

Indeed I did. 😄

Indeed. But still need some more work and more importantly additional tests (because I'm not sure the current patch would work for deep hierarchies..)

Of course. I did not want to suggest that the patch is not good enough as it is. It was just an idea that occurred to me while writing the reply. I probably try to get some work done on this in a few days.

@fmbenhassine
Copy link
Member

fmbenhassine commented Feb 24, 2017

I did not want to suggest that the patch is not good enough as it is

Yes yes of course no worries. I was talking about the current status of the fix: a quick and dirty PoC to see if we are able to tackle this issue. As usual, if I can come up with a working solution, I'll open a pull request and ask you for a review. If you have any idea of improvement, go for it!

My best regards
Mahmoud

@fmbenhassine fmbenhassine added this to the 3.7.0 milestone Mar 5, 2017
@PascalSchumacher
Copy link
Collaborator

@benas What about polishing the patch and releasing 3.7.0. I think this is would be a nice feature to have. 👍

@fmbenhassine
Copy link
Member

Sorry for not being active here for a while, I am busy migrating projects to j-easy. There still two projects to release and I'll focus on v3.7 of random-beans.

@fmbenhassine fmbenhassine modified the milestones: v3.8.0, 3.7.0 Jun 18, 2017
@fmbenhassine
Copy link
Member

I'm releasing v3.7.0 since it was asked by two other users.

This issue will be included in v3.8 until the patch is correctly polished and well tested.

@mcs
Copy link

mcs commented May 3, 2018

Any chance there will be a 3.8.0 release including this fix? :-)

@fmbenhassine
Copy link
Member

fmbenhassine commented May 8, 2018

Not sure I'll continue on my dirty hack to fix this issue. I would like to wait for a clean solution with reified generics.

The best answer to this issue is the one given by @PascalSchumacher here: #242 (comment)

@fmbenhassine
Copy link
Member

fmbenhassine commented Nov 25, 2018

After digging deeper into this issue, I came to the conclusion that the solution I suggested in
https://github.com/benas/random-beans/blob/035c378b227b9bbd9a811a86b73ba5b09ca4ef88/random-beans/src/main/java/io/github/benas/randombeans/FieldPopulator.java#L133-L149 is not correct: it assumes that when a field is generic, its generic type comes from the root type (which is saved in the randomization context). This is obviously not always the case, as shown in the following example:

class Foo<T> {
    T type;
    Object status;

    public <E> E getStatus() {
        return (E) status;
    }
}

Moreover, my solution is fighting against type erasure, which is battle lost upfront. The information we are looking for is simply not there at runtime. I wish it were possible to do something like: Field.isGenericType() and then if true doField.getActualType(), but this is not possible until reified generics are added to Java.

That said, I will revert my attempt and wait for a clean way to address this limitation. I will add a section in the documentation about the "Known limitations" when using random beans like the current issue. For now, the way to go is to register a custom randomizer as explained in
#242 (comment).

@reitzmichnicht @PascalSchumacher Does this make sense?

@reitzmichnicht
Copy link
Contributor Author

Makes sense

@PascalSchumacher
Copy link
Collaborator

Does this make sense?

sure

@fmbenhassine
Copy link
Member

Great, we all agree! I created a section in the wiki about "Known limitations". I'm closing this issue for now.

Thank you @reitzmichnicht for raising the issue and thank you @PascalSchumacher for the follow up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants