Skip to content

Added fetching from raw fields.#26

Merged
andimarek merged 1 commit into
graphql-java:masterfrom
erikogenvik:fieldfetch
Sep 17, 2015
Merged

Added fetching from raw fields.#26
andimarek merged 1 commit into
graphql-java:masterfrom
erikogenvik:fieldfetch

Conversation

@erikogenvik
Copy link
Copy Markdown
Contributor

The FieldDataFetcher will fetch data directly from raw fields.
It can be enabled by calling the "fetchField()" method on the field definition builder.

The FieldDataFetcher will fetch data directly from raw fields.
It can be enabled by calling the "fetchField()" method on the field definition builder.
@andimarek
Copy link
Copy Markdown
Member

Thanks a lot for your PR and especially adding also a test!

What I'm not so sure about is the fetchField in the builder. Don't you think it would be better just to set the DataFetcher instead having extra field? This also doesn't scale very well: Imagine we add a third or fourth provided DataFetcher.

What do you think?

Thanks

@erikogenvik
Copy link
Copy Markdown
Contributor Author

Yeah, the builder methods aren't necessary. However, I liked the way you don't have to specify the name of the property twice. It removes a failure possibility.
Another option would be to have a "DataFetcherStrategy" field in the builder containing an enum. It would default to "Property", but could be set to "Field". If more provided DataFetcher's are added they would be represented by additional enums.

@andimarek
Copy link
Copy Markdown
Member

Yes, you are right. Not having to specify the name twice is a good thing. I will merge this now. But probably I will change it to something more generic like your suggested "DataFetcherStrategy" field to make it more stable for other DataFetchers in the future.

Thanks again,
Andi

andimarek added a commit that referenced this pull request Sep 17, 2015
Added fetching from raw fields.
@andimarek andimarek merged commit 78af222 into graphql-java:master Sep 17, 2015
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.

3 participants