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

PropertyDataFetcher supports AutoValue style classes #149

Merged

Conversation

iancw
Copy link
Contributor

@iancw iancw commented May 16, 2016

Google's AutoValue library (https://github.com/google/auto/)
generates package-protected implementations of abstract
data classes with abstract public getters. These classes
weren't supported by the PropertyDataFetcher, since invoking
the public method on a package-protected class causes
InvocationTargetException. However, invoking the method
on the public parent class works fine.

These changes look for a public parent class with the applicable
getter method. Behavior with normal style classes should not be
affected.

 Google's AutoValue library (https://github.com/google/auto/)
 generates package-protected implementations of abstract
 data classes with abstract public getters. These classes
 weren't supported by the PropertyDataFetcher, since invoking
 the public method on a package-protected class causes
 InvocationTargetException. However, invoking the method
 on the public parent class works fine.

 These changes look for a public parent class with the applicable
 getter method. Behavior with normal style classes should not be
 affected.
@iancw iancw force-pushed the autovalue-compatible-propertyfetcher branch from 9baff4b to c278bc7 Compare May 19, 2016 02:26
@danielkwinsor
Copy link
Contributor

I did something like this in my repo, I'll look at this.

@dminkovsky
Copy link
Contributor

Hi @iancw. Thanks for this PR!

This appears to be related to #164, which was recently merged.

The conversation on #164 touched upon the extent to which we want to make PropertyDataFetcher support various use-cases at the expense of being a very simple built-in data fetcher. google/auto is probably used quite commonly, but given how easy it is to make one's own data fetchers, I'm not sure it makes sense to extend the very simple built in to cover this scenario. The decision to support various boolean prefixes on #164 was based on repeated community demand.

@bbakerman
Copy link
Member

I dont think this is an particularly unusual case. Just more forgiving.

It ascends the class hierarchy and finds the first public method on a the object that comes from a public class. It exactly matches we have today and more.

I think we should merge this.

@bbakerman bbakerman merged commit e108ad0 into graphql-java:master Mar 9, 2017
@iancw iancw deleted the autovalue-compatible-propertyfetcher branch March 16, 2017 01:33
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.

None yet

4 participants