j-easy / easy-random

The simple, stupid random Java beans/records generator
https://github.com/j-easy/easy-random/wiki
MIT License
1.61k stars 232 forks source link

Add parameter to silently ignore randomization exceptions #340

Closed fmbenhassine closed 5 years ago

fmbenhassine commented 5 years ago

While working on replacing JPopulator with RB in Netflix's AWSObjectMapper (I'm happy to see RB used in Netflix OSS BTW 😄 ), I noticed a difference in behaviour between JPopulator and RB.

JPopulator used to return null for types that it cannot instantiate (mainly abstract classes and interfaces). This has been changed in RB in issue #39 to throw an exception by default. I have suggested at that time to add a "silentMode" but looks like I chose to declare the exception in the API instead. However, this exception has been made a RuntimeException in #63.

So IMO, making this behaviour configurable through a silent mode parameter makes sense, especially for backward compatibility regarding people upgrading from JPopulator to RB.

@PascalSchumacher WDTY? Please see branch silent-mode for details.

PascalSchumacher commented 5 years ago

Good idea! 👍

Not sure if the name silentMode is a bit too general/generic, but I can not think of a better name at the moment (it's too late).

fmbenhassine commented 5 years ago

@PascalSchumacher Thanks for the feedback! Indeed that's what I also thought about the parameter name (See code comment here). But if we agree on the idea, it is already great! For naming, we know it's hard.. Especially when I work after 🕙 pm, I don't expect to come up with a good name 😄

More seriously, what would be a good name for this parameter? I was thinking of something similar to the expressive scanClasspathForConcreteTypes, like failFastForAbstractTypes, which means throw an exception and fail the randomization process fastly when an abstract type is encountered instead of doing an effort and fall back to objenesis and fail lately if objenesis still can't create the type. Or may be simply ignoreAbstractTypes. WDYT? Don't hesitate to suggest any other name.

PS: As a side note, I do apologise if I'm bothering you too much lately, I'm really focusing on closing all issues for RB. After closing all issues, I will release v3.9 and declare the project as being in maintenance mode (only dependency updates and bug fixes, but no more features). I will probably move the project to jeasy.org and rename it to easy-random (with a bump of version to v4).

PascalSchumacher commented 5 years ago

I like ignoreAbstractTypes.

P.S.: You are not bothering me. 😃 If I do have the time I will just delay/skip replying.

fmbenhassine commented 5 years ago

@PascalSchumacher Thank you! I really appreciate all your efforts!

I renamed the parameter to ignoreAbstractTypes. Merged in 06fc5b5670fc6ab9846646da4289f802209e386d.

fmbenhassine commented 5 years ago

@PascalSchumacher I was doing a last review of v3.9 and in hindsight, I'm not sure ignoreAbstractTypes is a good name for the parameter. This is because it is more general than ignoring abstract types. The code catches any kind of InstantiationError or Exception which obviously could be due to something different than the type being abstract.

Moreover, the documentation of the parameter is confusing:

With this parameter, random beans will not try to randomize abstract and interface types.

That's not true. RB will still try to randomize abstract types, and when it fails it will swallow the exception and set the field to null.

Lastly, the name can be confusing to users that might think that setting ignoreAbstractTypes to true would be equivalent to excluding abstract types with .excludeType(TypePredicates.isInterface().or(TypePredicates.isAbstract())) which is not the case.

Not sure if the name silentMode is a bit too general/generic, but I can not think of a better name at the moment (it's too late).

Probably my first intuition for silentMode was not that bad in the end, since the parameter is indeed generic for all failure cases (we can even change catch (InstantiationError | Exception e) to catch (Throwable t) IMO). If it's not too late and you can think of a better name, I would take it 🙃

PascalSchumacher commented 5 years ago

Well, then let's go with silentMode (or maybe something in the vein of ignoreRandomizationErrors would be better?)

fmbenhassine commented 5 years ago

Thanks! ignoreRandomizationErrors is better indeed. Fixed in d36846c.