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

Replace jPopulator with Easy Random #50

Merged
merged 1 commit into from
May 6, 2019
Merged

Replace jPopulator with Easy Random #50

merged 1 commit into from
May 6, 2019

Conversation

fmbenhassine
Copy link
Contributor

@fmbenhassine fmbenhassine commented Nov 1, 2018

Hi,

I'm the author of jPopulator (and its successor Easy Random). This PR replaces the outdated jPopulator with Easy Random.

Please let me know if you need any support in using the library, I would love to help!

Best regards,
Mahmoud

Copy link
Contributor

@brharrington brharrington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The changes LGTM other than the test failure.

@fmbenhassine
Copy link
Contributor Author

Hi,

Indeed, the mapRandomAwsObjects test is failing. I somehow missed the AwsMixinGenerator plugin.. Sorry for that.

Nevertheless, while working on this PR, I noticed a change in behaviour between jPopulator and Random Beans (which I detailed here). After fixing the issue, Random Beans is able to randomize all classes from the AWS model generated in this project. However, out of the 12575 types, Jackson is not able to deserialize the following 13 types:

com.amazonaws.services.dynamodbv2.model.AttributeValue
com.amazonaws.services.ec2.model.AuthorizeSecurityGroupEgressRequest
com.amazonaws.services.ec2.model.AuthorizeSecurityGroupIngressRequest
com.amazonaws.services.ec2.model.DescribeSecurityGroupsResult
com.amazonaws.services.ec2.model.IpPermission
com.amazonaws.services.elasticmapreduce.model.AddInstanceFleetRequest
com.amazonaws.services.elasticmapreduce.model.AddInstanceGroupsRequest
com.amazonaws.services.elasticmapreduce.model.Cluster
com.amazonaws.services.elasticmapreduce.model.Configuration
com.amazonaws.services.organizations.model.AcceptHandshakeResult
com.amazonaws.services.organizations.model.CancelHandshakeResult
com.amazonaws.services.organizations.model.DeclineHandshakeResult
com.amazonaws.services.organizations.model.DescribeHandshakeResult
com.amazonaws.services.organizations.model.EnableAllFeaturesResult
com.amazonaws.services.organizations.model.Handshake
com.amazonaws.services.organizations.model.HandshakeResource
com.amazonaws.services.simplesystemsmanagement.model.GetInventoryRequest
com.amazonaws.services.simplesystemsmanagement.model.InventoryAggregator

Those types are either recursive in structure, or having fields of a particular type that does not provide a default constructor, etc. Each of them makes it impossible to deserialize from Json using jackson. I tested those types one by one and here are some errors I encountered:

com.fasterxml.jackson.databind.JsonMappingException: Infinite recursion (StackOverflowError)

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `com.amazonaws.ResponseMetadata` (no Creators, like default construct, exist)

Unrecognized field "readLimit" (class com.amazonaws.services.ec2.model.AuthorizeSecurityGroupEgressRequest), not marked as ignorable

If you are ok with excluding these types from the test, I will update the PR accordingly (in addition to upgrading RB to the latest version). Otherwise, I will close this PR and let you continue with jPopulator. WDYT?

Best regards,
Mahmoud

@brharrington
Copy link
Contributor

Most of those aren't a big concern, the only ones that I would be a bit worried about are the EC2 objects. For the ones I looked at the problem was missing fields due to having deprecated getter/setter that would have a subset of information. I put in a patch to ignore the deprecated getter methods (#55) and tested with the current master of random beans. The EC2 objects worked fine at that point. Some of the others still fail, but that should be fine.

It does look like we would need a 3.9 release with some of the recent changes to make it work though.

@fmbenhassine
Copy link
Contributor Author

Thank you for your feedback! I will cut a release soon and update the PR accordingly. FYI, I recently decided that v3.9 will be the last version in the 3.x line (which will be EOL), so I will update the PR with v4 (also to be released soon, probably the same day as v3.9). It would be wiser to update you with a maintained version rather than an EOLed one. I'll keep you posted.

Thank you!

@fmbenhassine
Copy link
Contributor Author

Hi @brharrington ,

I released v4 and updated the PR accordingly. With your patch in #55 , the deserialization error for the IpPermission type does not happen anymore 👍

You will see that I excluded a couple of fields/types that do not provide a default constructor, which makes the deserialization with jackson to fail, but as you mentioned, that should be fine.

Please let me know if the changes are ok now or if the PR needs to be updated.

Best regards,
Mahmoud

@brharrington brharrington merged commit 31a8163 into Netflix:master May 6, 2019
@fmbenhassine fmbenhassine changed the title Replace jPopulator with Random Beans Replace jPopulator with Easy Random May 8, 2019
@fmbenhassine fmbenhassine deleted the replace-jpopulator-with-randombeans branch May 8, 2019 13:51
@fmbenhassine
Copy link
Contributor Author

@brharrington Thank you for accepting the PR. I'm glad to see jPopulator Easy Random used by Netflix OSS! If you need any kind of support, I would be happy to help.

Best regards,
Mahmoud

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

Successfully merging this pull request may close these issues.

2 participants