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

Current object as Randomizer#getRandomValue's parameter #6

Closed
eric-taix opened this issue Aug 11, 2014 · 13 comments
Closed

Current object as Randomizer#getRandomValue's parameter #6

eric-taix opened this issue Aug 11, 2014 · 13 comments
Assignees
Labels
Milestone

Comments

@eric-taix
Copy link

It could be useful if Randomizer can access the current object.

A typical usecase is an email Randomizer for a Person:

  • The firstname uses FirstNameRandomizer
  • The lastname uses LastNameRandomizer
  • The email uses EmailRandomizer but generated emails have no relationship with the firstname nor the lastname.

If Randomizer can have access to the current objet then we can provide another EmailRandomizer which instead of generating the email address from a list, takes into account the current firstname and lastname and can (for example) retrieve the domain name from a list.

firstname: eric
lastname: taix
=> email: eric.taix@

@fmbenhassine
Copy link
Member

fmbenhassine commented Aug 15, 2014

This point has been already raised and we are trying to find the best way to make randomizers collaborate together to add some "quality" to the generated data.

Another typical usecase is the country and city randomizers: If the country randomizer generates "France" for the country, the city randomizer should only generates french cities.

I think we should define a clear contract of what the context object should contain (beside the currently populated object).

@fmbenhassine fmbenhassine added this to the 1.2 milestone Dec 20, 2014
@fmbenhassine fmbenhassine modified the milestones: 1.3, 1.2 Jan 29, 2015
@fmbenhassine fmbenhassine self-assigned this Dec 21, 2015
@fmbenhassine fmbenhassine modified the milestones: 2.1.0, 2.0.0 Dec 21, 2015
@fmbenhassine fmbenhassine modified the milestones: 3.1.0, 3.0.0 Apr 6, 2016
@fmbenhassine fmbenhassine modified the milestones: 3.2.0, 3.1.0 May 30, 2016
@fmbenhassine fmbenhassine modified the milestones: 3.3.0, 3.2.0 Jul 5, 2016
@fmbenhassine fmbenhassine removed this from the 3.3.0 milestone Oct 1, 2016
@muhammedf
Copy link

Hey!
I was thinking about this feature's implementation.

There could be another interface that extends current Randomizer interface because context object will not be needed for every Randomizer. And it could have a setContext method. Context object that will be passing to setContext method should contain currently populated object and needed fields for randomization.

Example:

firstname: eric
lastname: taix
=> email: eric.taix@

firstname and lastname fields will be passed for this situation.

Another typical usecase is the country and city randomizers: If the country randomizer generates "France" for the country, the city randomizer should only generates french cities.

And country field will be passed for this.

I wonder what kind of implemantation do you have in your mind and what do you think about this?

@fmbenhassine
Copy link
Member

fmbenhassine commented Jun 26, 2017

Hi @muhammedf

Thank you for this suggestion! This is what I had in mind, some ContextAwareRandomizer that extends Randomizer with a setter for the context object.

Context object that will be passing to setContext method should contain currently populated object and needed fields for randomization.

This is the most important decision in the story, what to put in the context object.

Another interesting use case is issue 188, where we need to have access to the object graph to be able to implement the 'dotted' notation.

The goal is to define a clear contract of what the population context contains.
But as I said in issue #188, this requires a major API change. I want to keep this change to be included in the rewrite of the library's core as explained here. But I never got time (yet) to work on this..

@eric-taix This feature request is very interesting that's why I didn't want to close it even though it took time.. I'm really sorry for this. If only I could work full time on OSS projects 😡

Kind regards
Mahmoud

@eric-taix
Copy link
Author

@benas Don't worry, working on OSS projects as part-time is always difficult and unfortunately I have not enough time to help you right now.

I like your ContextAwareRandomizer suggestion

BTW you already did a very good job with Random-bean project

@muhammedf
Copy link

muhammedf commented Jul 5, 2017

@benas I examined the issue #188. As I understand it is about different treatments for objects with same type and same hierarchy but different depths. I think in that situation, object graph is really needed.
But if dotted notations were relative to the current object then there is no need for object graph. And of course in this case only current object's fields or their childs (and their childs ...) can be dependent fields for randomization.

Example scenario for email randomization:

class Name{
String name;
}
class Person{
Name firstname; 
Name lastname;
@Randomizer(value=EmailRandomizer.class, dependentFields={"firstname.name", "lastname.name"})
String email;
}
class Department{
Person manager;
}

Randomizing a Department instance: (for showing that dotted notations are relative to the current object)

Department department = EnhancedRandomBuilder.aNewEnhancedRandom().nextObject(Department.class);

And again of course dependent fields must be randomized before email. (I could not find a term for this :)

@fmbenhassine
Copy link
Member

Hi,

I (finally) managed to work on this feature and added a couple interfaces:

  • RandomizerContext: containing relevant information about the randomization context including the currently randomized object as request in this issue.
  • ContextAwareRandomizer: to be implemented by any randomizer that wants to be aware of the randomization context.

Random Beans will inject the randomization context in any context aware randomizer before calling its getRandomValue method, so that the randomizer can generate the value accordingly. Here is a quick example, given the following class:

@lombok.Data
public class Person {
    private String firstName;
    private String lastName;
}

Let's say we want to generate a last name depending on the first name (this is basically the use case in the first comment of this issue). A context aware LastNameRandomizer would be:

import io.github.benas.randombeans.api.ContextAwareRandomizer;
import io.github.benas.randombeans.api.RandomizerContext;

/**
 * A last name randomizer that depends on the first name of the currently randomized object.
 * The currently randomized object can be retreived from the randomization context.
 */
public class LastNameRandomizer implements ContextAwareRandomizer<String> {

    private RandomizerContext context;

    @Override
    public void setRandomizerContext(RandomizerContext context) {
        this.context = context;
    }

    @Override
    public String getRandomValue() {
        if (context.getRandomizedType().equals(Person.class)) {
            Person randomizedObject = (Person) context.getRandomizedObject();
            String firstName = randomizedObject.getFirstName();
            if (firstName != null && firstName.equalsIgnoreCase("james")) {
                return "bond";
            }
            if (firstName != null && firstName.equalsIgnoreCase("daniel")) {
                return "craig";
            }
        }
        return null;
    }
}

and how to use it:

import io.github.benas.randombeans.EnhancedRandomBuilder;
import io.github.benas.randombeans.api.EnhancedRandom;
import org.junit.jupiter.api.Test;

import static io.github.benas.randombeans.FieldDefinitionBuilder.field;
import static org.assertj.core.api.Assertions.assertThat;

public class ContextAwareRandomizationTests {

    @Test
    void testContextAwareRandomization() {
        // given
        String[] names = {"james", "daniel"};
        EnhancedRandom enhancedRandom = new EnhancedRandomBuilder()
                .randomize(field().named("firstName").ofType(String.class).inClass(Person.class).get(), new FirstNameRandomizer(names))
                .randomize(field().named("lastName").ofType(String.class).inClass(Person.class).get(), new LastNameRandomizer())
                .build();

        // when
        Person person = enhancedRandom.nextObject(Person.class, "nickname");

        // then
        String firstName = person.getFirstName();
        String lastName = person.getLastName();
        assertThat(firstName).isIn(names);
        assertThat(lastName).isNotNull();
        if (firstName.equalsIgnoreCase("james")) {
            assertThat(lastName.equalsIgnoreCase("bond"));
        }
        if (firstName.equalsIgnoreCase("daniel")) {
            assertThat(lastName.equalsIgnoreCase("craig"));
        }
    }
}

FirstNameRandomizer picks up a random name from a given list and the LastNameRandomizer returns the corresponding last name according to the implemented rule.

I deployed version 3.9.0-SNAPSHOT to maven central so you can play with these new APIs. Note that I was able to implement the feature without breaking any API (programming to interfaces FTW!), so I planned this addition to be included in v3.9 instead of v4.0 as announced initially.

That's only a first effort towards implementing this new feature, so any idea/suggestion of improvement is welcome!

@PascalSchumacher All changes are in issue-6 branch. Please don't hesitate to do a review.

@eric-taix @muhammedf It would be great if you give it a try (see how to import a snapshot version here) and let me know if it works for you. Looking forward for your feedback!

Kr,
Mahmoud

@PascalSchumacher
Copy link
Collaborator

Looks good!

I took a short look at changing the interface to

public interface ContextAwareRandomizer<ContextType, RandomizedType> extends Randomizer<RandomizedType> {

    void setRandomizerContext(RandomizerContext<ContextType> context);
}

which would allow a bit shorter implementations like:

public class LastNameRandomizer implements ContextAwareRandomizer<Person, String> {

    private RandomizerContext<Person> context;

    @Override
    public void setRandomizerContext(RandomizerContext<Person> context) {
        this.context = context;
    }

    @Override
    public String getRandomValue() {
        Person randomizedObject = context.getRandomizedObject();
        String firstName = randomizedObject.getFirstName();
        if (firstName != null && firstName.equalsIgnoreCase("james")) {
            return "bond";
        }
        if (firstName != null && firstName.equalsIgnoreCase("daniel")) {
            return "craig";
        }
    }
}

but this is probably over-engineering for dubious gain.

@fmbenhassine
Copy link
Member

@PascalSchumacher Thank you for the feedback! Indeed, this will eliminate the test if (context.getRandomizedType().equals(Person.class)) and the corresponding cast from client's code (your implementation should still return null in other cases BTW).

@fmbenhassine
Copy link
Member

fmbenhassine commented Feb 12, 2019

@PascalSchumacher I tested the approach you suggested in branch issue-6-generic-context. Even though the cast in client code is not needed anymore, it makes the RandomizerContext<T> misleading by making it generic. In the example, LastNameRandomizer has a setter for RandomizerContext<Person>, however, the randomization context may contain other types than Person during the randomization, which leads to class cast exceptions (See testContextAwareRandomizerWithMultipleTypes). This is also true when the ContextAwareRandomizer is matched and used in multiple types, here is an example:

@Data
public class Person {
    private String firstName;
    private String lastName;
    private String nickname;
    private Foo foo;
}

@Data
public class Foo {
    private String firstName;
    private String lastName;
}

// then use the following field definitions
String[] names = {"james", "daniel"};
EnhancedRandom enhancedRandom = new EnhancedRandomBuilder()
        .randomize(field().named("firstName").ofType(String.class).get(), new FirstNameRandomizer(names))
        .randomize(field().named("lastName").ofType(String.class).get(), new LastNameRandomizer())
        .build();

So I think we can stick to the current API for now (which I polished after further testing).

@PascalSchumacher
Copy link
Collaborator

PascalSchumacher commented Feb 13, 2019

Yes, the current API is fine. Let's stick to it. 👍

@fmbenhassine
Copy link
Member

After further testing of these new APIs, I noticed that the randomization context should contain a reference to the root object being randomized in addition to the currently randomized object in the object graph. Here is an example:

untitled

When RB tries to create a random person, it will iterate through its fields. At some point, it will find that Pet is a user defined type and will recursively try to randomize it. At this point, the currently randomized object is of type Pet and not Person. With this in mind, here is the use case: let's suppose we want to have a CityRandomizer that is based on CountryRandomizer, for example if country = 'france' then city = 'paris'. Since city and country are at the same level under Person, it is not possible to implement this requirement without having access to the root object (of type Person), because the currently randomized object in the context is of type Pet (which is the last type that has been saved in the context while randomizing the person). Here is a sample client code for CityRandomizer with the current API:

/**
 * A city randomizer that depends on the country of the currently randomized object.
 * The currently randomized object can be retrieved from the randomization context.
 */
public class CityRandomizer implements ContextAwareRandomizer<City> {

    private RandomizerContext context;

    @Override
    public void setRandomizerContext(RandomizerContext context) {
        this.context = context;
    }

    @Override
    public City getRandomValue() {
        if (context.getCurrentObject() instanceof Person) {
            Person person = (Person) context.getCurrentObject();
            Country country = person.getCountry();
            if (country == null) {
                return null;
            }
            String countryName = country.getName();
            if (countryName != null && countryName.equalsIgnoreCase("france")) {
                return new City("paris");
            }
        }
        return null;
    }
}

This won't work as context.getCurrentObject() instanceof Person is never true when this randomizer is invoked (context.getCurrentObject() is actually a Pet as this is the last object saved in the context). However, with something like context.getRootObject() instanceof Person, we can get access to the top level type and randomize its fields as needed. I hope I made the distinction clear. The previous example is implemented here.

This new change is pushed in the latest v3.9.0-SNAPSHOT. Looking forward to getting feedback on this feature request soon.

@fmbenhassine
Copy link
Member

@eric-taix Have you got a chance to take a look at this new feature? The new APIs should cover the use case you mentioned. Here is ready to use maven project to test the feature:

test.zip

Looking forward for your feedback.

fmbenhassine added a commit that referenced this issue Feb 27, 2019
@fmbenhassine
Copy link
Member

Issue #188 was another good use case for this feature as well: the question is how to know the current field being randomized in the object graph (See example here). This additional use case allowed me to polish the API with a couple of new methods to get the current field being randomized (with dotted notation) as well as the current depth level in the hierarchy.

I think the current API is quite ok after all these tests. But let's iterate on this. I merged the current status to the master branch for v3.9 and we can improve things in later versions.

@eric-taix I'm closing this issue for now but don't hesitate to comment here if you want.

As a side note: This is definitely one of the best issues I've worked on in the history of the project.

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

4 participants