Skip to content

Conversation

@martijnvg
Copy link
Contributor

Because of this change all model classes have now annotated constructors.

This allows geoip2 to be run in secured environments without enabling suppressAccessChecks permission.

PR for #51

Because of this change all model classes have now annotated constructors.

This allows geoip2 to be run in secured environments without enabling suppressAccessChecks permission.

Closes maxmind#51
@martijnvg martijnvg force-pushed the tell_jackson_databind_to_not_suppress_access_checking branch from 41de1c9 to 397ef8c Compare December 16, 2015 08:50
Copy link
Member

Choose a reason for hiding this comment

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

Was there a particular reason you want with static factory methods for the empty objects instead of a constructor with no arguments?

Also, please add some basic documentation for these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. I'll remove the static methods and add no args constructors.

Copy link
Member

Choose a reason for hiding this comment

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

I think this class has the same serialization issue.

Copy link
Member

Choose a reason for hiding this comment

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

As do all the ip_address getters.

@oschwald
Copy link
Member

Overall this looks great. I had a number of small comments.

@martijnvg
Copy link
Contributor Author

I've updated this PR. I didn't add java docs on the constructors, because that would contain duplicated information that can be found on the getters too.

@oschwald
Copy link
Member

Thanks! 👍

oschwald added a commit that referenced this pull request Dec 16, 2015
…ppress_access_checking

Configure jackson-databind to not suppress access checks.
@oschwald oschwald merged commit ca3c421 into maxmind:master Dec 16, 2015
@martijnvg
Copy link
Contributor Author

nice :)

@martijnvg
Copy link
Contributor Author

@oschwald Do you know when the next release is scheduled?

@oschwald
Copy link
Member

I'm hoping to get to it early next week. I am waiting for internal review on #53.

@martijnvg
Copy link
Contributor Author

@oschwald thanks, that is great!

@oschwald
Copy link
Member

This has been released. It should be on mirrors in a few hours.

@martijnvg
Copy link
Contributor Author

thanks @oschwald. We've upgraded to the latest version: elastic/elasticsearch#15599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants