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

Bean validation annotation handlers don't use the defined registry #401

Closed
andrei-ivanov opened this issue Mar 26, 2020 · 10 comments
Closed
Labels
Milestone

Comments

@andrei-ivanov
Copy link

andrei-ivanov commented Mar 26, 2020

Having some classes annotated with bean validation annotations, I've imported the specific modules just to discover that @Digits is not supported.
Seing that, I've defined an explicit randomizer:

CustomRandomizerRegistry registry = new CustomRandomizerRegistry();
// we usually use a scale of 3 for BigDecimal
registry.registerRandomizer(BigDecimal.class, new BigDecimalRandomizer(Integer.valueOf(3)));

The problem seems to be that the annotation handlers don't use the registries, but create a new EasyRandom instance and configure that one.

Any way they can be made to use my registry?

Using version 4.2.0

fmbenhassine added a commit that referenced this issue Mar 26, 2020
This change is required to be able to extend the default bean validation
registry with custom annotation handlers.

Issue #401
@fmbenhassine
Copy link
Member

fmbenhassine commented Mar 26, 2020

Thank you for opening this issue. Indeed, @Digits is not supported (this is mentioned in the wiki as you might have seen).

Any reason for using CustomRandomizerRegistry and not BeanValidationRandomizerRegistry? Looking at your code snippet, I see it no different than:

EasyRandomParameters parameters = new EasyRandomParameters()
         .randomize(BigDecimal.class, new BigDecimalRandomizer(Integer.valueOf(3)));

Bean validation annotation handlers are mapped by annotation, not by field type (see here). So if you want to add support for @Digits, you should be able to extend BeanValidationRandomizerRegistry and map your custom handler for the @Digits annotation. Unfortunately, this is not possible with v4.2 because the field BeanValidationRandomizerRegistry#annotationHandlers is private. I changed it to protected in 1598695 where you can find an example of the described approach.

Is this what you are looking for? If yes, you can give it try with 4.3.0-SNAPSHOT and share your feedback. Otherwise, please share an example of what you are trying to achieve. Thank you upfront!

@andrei-ivanov
Copy link
Author

I've made a RandomUtils class at some point, to restore some of the lost functionality that we've had with the excludedFields part:
static <T> T random(Class<T> type) and static <T> T random(Class<T> type, String... excludedFields), so using a CustomRandomizerRegistry seemed the best way to add some limits to the randomizers.
I've come to this one by inspecting the code, so before this conversation, wasn't sure if it's the right decision.
Regarding BeanValidationRandomizerRegistry, I probably didn't discover it, since it's in a different module, but also not all of our modules use it, so I would have wanted to use something more generic anyway.

The current code looks like this:

import org.jeasy.random.EasyRandom;
import org.jeasy.random.EasyRandomParameters;
import org.jeasy.random.FieldPredicates;
import org.jeasy.random.api.RandomizerRegistry;
import org.jeasy.random.randomizers.number.BigDecimalRandomizer;
import org.jeasy.random.randomizers.range.LongRangeRandomizer;
import org.jeasy.random.randomizers.registry.CustomRandomizerRegistry;

import java.math.BigDecimal;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static java.util.Arrays.asList;
import static java.util.stream.Collectors.toList;
import static org.apache.commons.lang3.ArrayUtils.isEmpty;

public class RandomUtils {

	private static final RandomizerRegistry DEFAULT_REGISTRY;
	private static final EasyRandom RANDOM;
	private static final Map<Set<String>, EasyRandom> EXCLUDED_FIELDS_RANDOMS = new HashMap<>();

	static final long MIN_UNSIGNED_INT = 0L;
	static final long MAX_UNSIGNED_INT = Integer.MAX_VALUE * 2L + 1;

	private RandomUtils() {
		throw new IllegalStateException("Thou shalt not instantiate!");
	}

	static {
		CustomRandomizerRegistry registry = new CustomRandomizerRegistry();
		// we usually have a Long for the version property of DTOs and that's generated by JAXB for xs:unsignedInt, which actually is limited to these values
		registry.registerRandomizer(Long.class, new LongRangeRandomizer(MIN_UNSIGNED_INT, MAX_UNSIGNED_INT));
		// we usually use a scale of 3 for BigDecimal and since @Decimals is ignored, let's hardcode it here 
		registry.registerRandomizer(BigDecimal.class, new BigDecimalRandomizer(3));

		DEFAULT_REGISTRY = registry;

		RANDOM = new EasyRandom(randomParameters());
	}

	public static EasyRandomParameters randomParameters() {
		return new EasyRandomParameters().randomizerRegistry(DEFAULT_REGISTRY);
	}

	public static <T> T random(Class<T> type) {
		return random(RANDOM, type);
	}

	public static <T> T random(Class<T> type, String... excludedFields) {
		return random(getRandom(excludedFields), type);
	}

	private static EasyRandom getRandom(String[] excludedFields) {
		if (isEmpty(excludedFields)) {
			return RANDOM;
		}

		return EXCLUDED_FIELDS_RANDOMS.computeIfAbsent(new HashSet<>(asList(excludedFields)), RandomUtils::buildEasyRandom);
	}

	private static EasyRandom buildEasyRandom(Set<String> excludedFields) {
		EasyRandomParameters parameters = randomParameters();
		for (String field : excludedFields) {
			parameters.excludeField(FieldPredicates.named(field));
		}
		return new EasyRandom(parameters);
	}

	public static <T> T random(EasyRandom random, Class<T> type) {
		return random.nextObject(type);
	}

        // this one was also lost in the fire
	public static <T> List<T> randomListOf(int amount, Class<T> type) {
		return RANDOM.objects(type, amount).collect(toList());
	}
}

I'll try your suggestion a bit later today, thank you 🙂

@andrei-ivanov
Copy link
Author

Thinking about your suggestion, I think there's still a problem that the annotation handlers don't get a reference to the existing EasyRandom instance or RandomizerRegistry and they create an internal one.

@andrei-ivanov
Copy link
Author

andrei-ivanov commented Mar 27, 2020

The test classes that I use looks something like:

class Amount {
    @XmlElement(name = "Amount", required = true)
    @NotNull
    @Digits(integer = 12, fraction = 3)
    protected BigDecimal amount;
    @XmlElement(name = "Currency", required = true)
    @XmlSchemaType(name = "string")
    @NotNull
    // this is an enum
    protected Currency currency;
}
class DiscountEffect {
    @XmlElement(name = "Percentage")
    @Digits(integer = 6, fraction = 4)
    protected BigDecimal percentage;
    @XmlElement(name = "Amount")
    @Valid
    protected Amount amount;
    @XmlElement(name = "Quantity")
    @Digits(integer = 12, fraction = 3)
    protected BigDecimal quantity;
    @XmlElement(name = "SetSize")
    @NotNull
    @DecimalMax("65535")
    @DecimalMin("1")
    protected Integer setSize;
}
class Discount {
    @XmlElement(name = "DiscountEffects", required = true)
    @NotNull
    @Size(min = 1)
    @Valid
    protected List<DiscountEffect> discountEffects;
    // others too
    protected String href;
}

And it gets invoked as RandomUtils.random(Discount.class, "href")

@unconditional
Copy link
Contributor

Just stumbled upon this very issue: had a prepared EasyRandom instance with cusom randomizers specified through EasyRandomParameters in my test fixture and started to get randomization exceptions when trying to generate a bean validation annotated object graph for the fields that should be processed by the mentioned custom randomizers.

Checked out the code for some annotation handlers and was astonished to see that bean validation module completely ignores whatever was configured and instead just uses new EasyRandom instances in its annotation handlers. As if easy-random-bean-validation is just a separate project with no connection to the structure of the main one.

This is very counter-intuitive, since the wiki leaves a complete impression that upon adding the dependency the bean validation functionality will be added to a user's EasyRandom config, not replace it.

There should be a way to make any new EasyRandom instance share the same user-defined parameters and customRegistry.

@fmbenhassine
Copy link
Member

@andrei-ivanov

It seems that you are passing the value 3 as a seed instead of a scaling here:

// we usually use a scale of 3 for BigDecimal and since @Decimals is ignored, let's hardcode it here 
registry.registerRandomizer(BigDecimal.class, new BigDecimalRandomizer(3));

This should be:

registry.registerRandomizer(BigDecimal.class, new BigDecimalRandomizer(Integer.valueOf(3)));

Is this the cause of your issue?

Probably I'm missing something, but I'm not able to reproduce the issue based on my understanding. Here is a test that passes (while it should fail according to the issue description):

@Test
void customRegistryTest() {
    // given
    class Amount {
        @NotNull
        @Digits(integer = 12, fraction = 3)
        protected BigDecimal amount;
    }
    class DiscountEffect {
        @Digits(integer = 6, fraction = 4)
        protected BigDecimal percentage;
        protected Amount amount;
        @Digits(integer = 12, fraction = 3)
        protected BigDecimal quantity;
        @NotNull
        @DecimalMax("65535")
        @DecimalMin("1")
        protected Integer setSize;
    }

    CustomRandomizerRegistry registry = new CustomRandomizerRegistry();
    registry.registerRandomizer(BigDecimal.class, new BigDecimalRangeRandomizer(new Double(5d), new Double(10d), Integer.valueOf(3)));
    registry.registerRandomizer(Integer.class, new IntegerRangeRandomizer(5, 10));
    
    EasyRandomParameters parameters = new EasyRandomParameters()
            .randomizerRegistry(registry);
    EasyRandom easyRandom = new EasyRandom(parameters);

    // when
    DiscountEffect discountEffect = easyRandom.nextObject(DiscountEffect.class);

    // then
    assertThat(discountEffect).isNotNull();
    assertThat(discountEffect.percentage).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
    assertThat(discountEffect.quantity).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
    assertThat(discountEffect.amount.amount).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
    assertThat(discountEffect.setSize).isBetween(5, 10);
}

@andrei-ivanov @unconditional Can you please provide a failing test that reproduces the issue?

@andrei-ivanov
Copy link
Author

Hi,
Had to look closely again at the code, since I've forgotten what this thing was about 😀

Using your test above, add this snippet to the end:

class Discount {
	@NotNull
	@Size(min = 1)
	@Valid
	protected List<DiscountEffect> discountEffects;
}
Discount discount = easyRandom.nextObject(Discount.class);
assertThat(discount.discountEffects)
	.isNotEmpty()
	.allSatisfy(discountEffectElement -> {
		assertThat(discountEffectElement).isNotNull();
		assertThat(discountEffectElement.percentage).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
		assertThat(discountEffectElement.quantity).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
		assertThat(discountEffectElement.amount.amount).isBetween(new BigDecimal("5.000"), new BigDecimal("10.000"));
		assertThat(discountEffectElement.setSize).isBetween(5, 10);
	});

With this I get this kind of errors:

  <1DiscountEffect@b07ae662> error: 
Expecting:
 <0.723174202997146853277854461339302361011505126953125>
to be between:
 [5.000, 10.000]

@fmbenhassine
Copy link
Member

Thank you for your feedback, indeed the test fails with those changes.

Bean validation handlers have been made aware of custom registries and the test is now passing.

The fix has been deployed in 4.3.0-SNAPSHOT, and I would be grateful if you could give it a try and share your feedback before the upcoming 4.3 release. Thank you.

@andrei-ivanov
Copy link
Author

Initial test with the dependencies defined like

<dependency>
    <groupId>org.jeasy</groupId>
    <artifactId>easy-random-core</artifactId>
    <version>${randombeans.version}</version>
</dependency>

<dependency>
    <groupId>org.jeasy</groupId>
    <artifactId>easy-random-bean-validation</artifactId>
    <version>${randombeans.version}</version>
    <exclusions>
        <!-- somehow the version cannot be overridden, so excluded it since it's not used anyway -->
        <exclusion>
            <groupId>org.yaml</groupId>
            <artifactId>snakeyaml</artifactId>
        </exclusion>
        <exclusion>
            <groupId>javax.validation</groupId>
            <artifactId>validation-api</artifactId>
        </exclusion>
    </exclusions>
</dependency>

<dependency>
    <groupId>jakarta.validation</groupId>
    <artifactId>jakarta.validation-api</artifactId>
    <optional>true</optional>
</dependency>

leads me to a missing dependency:

Caused by: java.lang.NoClassDefFoundError: org.apache.commons.beanutils.BeanIntrospector
	at org.jeasy.random.EasyRandom.doPopulateBean(EasyRandom.java:137)

Not sure how it worked before as I think this was required anyway 🙂

After adding the dependency manually, I see the test still fails with the same errors... not sure why.
The dependency tree shows it should use the new version:

[INFO] +- org.jeasy:easy-random-core:jar:4.3.0-SNAPSHOT:compile
[INFO] |  \- io.github.classgraph:classgraph:jar:4.8.65:compile
[INFO] +- org.jeasy:easy-random-bean-validation:jar:4.3.0-SNAPSHOT:compile
[INFO] |  \- org.jeasy:easy-random-randomizers:jar:4.3.0-SNAPSHOT:compile
[INFO] |     \- com.github.javafaker:javafaker:jar:1.0.2:compile
[INFO] |        \- com.github.mifmif:generex:jar:1.0.2:compile
[INFO] |           \- dk.brics.automaton:automaton:jar:1.11-8:compile
[INFO] +- jakarta.validation:jakarta.validation-api:jar:2.0.2:compile (optional) 
[INFO] +- commons-beanutils:commons-beanutils:jar:1.9.4:runtime
[INFO] |  \- commons-collections:commons-collections:jar:3.2.2:runtime

@andrei-ivanov
Copy link
Author

With the final release everything seems fine :)
I think the snapshots had some weird issues somehow.

Thank you

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

No branches or pull requests

3 participants