Fix for #130 - Adds support for Boolean property accessors with 'get' prefix in PropertyDataFetcher #164

ayhanap · 2016-06-15T20:00:13Z

As mentioned in #130 some languages and frameworks uses "get" prefix for Booleantypes. This was also the case for me. I use Lombok for generating accessors which also uses this convention. Even IDE's generate getters for Boolean with "get"

The prefix 'is' generally used for primitive boolean, Lombok has a option to disable this but not for Boolean. So I think this functionality should be a part of PropertyDataFetcher. Otherwise it will be error prone or some unnecessary configuration/code for these fields.

…etcher

markphilpot · 2016-08-19T22:44:51Z

src/main/java/graphql/schema/PropertyDataFetcher.java

+        }
+    }
+
+    private Object getPropertyViaGetterUsingPrefix(Object object, GraphQLOutputType outputType, String prefix) throws NoSuchMethodException {
        String getterName = prefix + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1);
        try {
            Method method = object.getClass().getMethod(getterName);
            return method.invoke(object);

        } catch (NoSuchMethodException e) {


Can you just not have a catch block for NoSuchMethodException?

Right, just removed the unnecessary catch block for NoSuchMethodException

markphilpot · 2016-08-19T22:48:13Z

Thanks for the PR! Let me know if the comments make sense.

aschrijver · 2016-08-19T23:04:11Z

It could also have an additional clause with a search for any property that contains propertyName, and return the first match. This way you will catch any reasonable naming convention (only not the completely unrelated names).

(BTW Many new codes do not use is and get prefixes any more, but what fits with the domain language and fluent coding e.g. shouldUpdateAccount, succeeded, canSubtractValue, etc.)

dminkovsky · 2016-08-19T23:17:32Z

Yes, thank you for this PR! Let's fix this, given the demand.

What do you think about this: https://github.com/dminkovsky/graphql-java/blob/8efb230b58224488c335cc8409c17a73f0657b51/src/main/java/graphql/schema/PropertyDataFetcher.java

I think it might be clearer?

ayhanap · 2016-08-19T23:17:39Z

Thanks for review @markphilpot The current flow to find property data is as follows

Try accessor methods. If the property is boolean use is getter methods instead of get.
If no method exists then .. If the property is boolean, try get accessor method also.
If no method exists then .. Get property data with field access.
Before this PR the current flow was 1 then 3, without 2.
So the code in your inline comments does not fulfill the requirements. Therefore I suggest you to check working code and let me know if this makes sense.

ayhanap · 2016-08-19T23:21:05Z

@dminkovsky Looks simpler and more explanatory indeed.

dminkovsky · 2016-08-19T23:21:09Z

Whoops didn't mean to overload getPropertyViaGetter there. One of those should be renamed.

dminkovsky · 2016-08-19T23:23:10Z

@ayhanap cool. If you like that more, feel free to update your PR with that variant and we can see what other people think. Will be glad to close this one.

ayhanap · 2016-08-19T23:49:17Z

Updated the PR with @dminkovsky's cleaner suggestion.

dminkovsky · 2016-08-20T00:05:16Z

@ayhanap cool, thanks. I think there's also @markphilpot's question regarding that caught-and-then-immediately-thrown NoSuchMethodException in getPropertyViaGetterUsingPrefix(). It does seem that it's not necessary?

ayhanap · 2016-08-20T11:59:50Z

Sorry missed that. Fixed that.

aschrijver · 2016-08-20T12:01:32Z

How do you think about earlier suggestion to allow any kind of prefix or postfix #164 (comment) ?

I know classes with conventions like can, should, with, on, after, etc.

aschrijver · 2016-08-20T12:33:32Z

Yet another option, and easily added is a public overload that takes the prefix.

ayhanap · 2016-08-20T12:38:35Z

I think searching for method names containing fielddefinition names could lead to some unexpected results.
All I can think is to search for a method name as exact field definition name. So that way you can define field definitions on your getter method name. And the example scenario I mentioned is as follows.

newFieldDefinition()
            .name("shouldUpdateAccount") // instead of using updateAccount
            .description("Should Update Account Details")
            .type(GraphQLString)
            .build()

And a POJO like this

public class Account{
    boolean updateAccount;
    public shouldUpdateAccount(){
        return this.uppdateAccount;
    }
}

What do you think @aschrijver.

aschrijver · 2016-08-20T13:37:04Z

Yes, I agree, you are right with the search. It would be too much of a burden on the developer to get it right, and don't forget when adding a similarly-named member in future.

Passing a prefix parameter could be solution.

On a different note, related but not the same, I have a need to be able to define field aliases. I am discussing that on the metadata-related issues I created earlier.

dminkovsky · 2016-08-20T14:07:40Z

I kind of like the idea of keeping the surface of this class small, like it currently is. Property fetchers, after all, are a core public API. If people need some specific functionality they can write their own property fetcher and with lambda notation this is trivial for a simple property reference. To me, this PR is about responding to a specific repeated request from multiple users.

So, let's revisit adding more functionality to this property fetcher when there is repeated, specific demand? I like how it looks right now as of this PR, especially with the removed unused fields.

aschrijver · 2016-08-20T14:15:02Z

Yeah, that's no problem. Let's close this one. Given how busy I am it will be some time before I revisit this, maybe then I'll come up with a more elegant PR for other prefixes. We'll see..

ayhanap · 2016-08-20T14:15:31Z

Agree, those other requirements need more discussion and evaluation. Keeping in mind that this class is responsible of most of the data fetching, it should be fast and simple.

dminkovsky · 2016-08-21T00:04:09Z

Thanks Mark

суббота, 20 августа 2016 г. пользователь Mark Philpot написал:

Merged #164 #164.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#164 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANWZYLq6dN71JDtpZLJ6U1ls95S2VlXks5qh4vQgaJpZM4I2vHN
.

aschrijver · 2016-08-21T00:10:50Z

@dminkovsky Hailing from Russia or Ukrain? Then you are making even more late-nighters than me 😉

markphilpot · 2016-08-21T00:47:15Z

пожалуйста

dminkovsky · 2016-08-21T01:54:51Z

Oh no thankfully on the east coast of the US. Have my phone in Russian
though, so it does this. I try to get to sleep at a reasonable hour :)

суббота, 20 августа 2016 г. пользователь Mark Philpot написал:

пожалуйста

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#164 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANWZRFzIo2I7fRWpEB5tFDen3vukEmgks5qh6AUgaJpZM4I2vHN
.

aschrijver · 2016-08-21T02:01:45Z

Lucky you, ha ha. I should be horizontal now at 04:00PM on the clock, not in S shape over laptop 😁

support Boolean property accessors with 'get' prefix in PropertyDataF…

3d5c1b1

…etcher

ayhanap mentioned this pull request Jun 15, 2016

Boolean fields *must* be accessed using an isX method #130

Closed

dminkovsky added the enhancement label Aug 18, 2016

markphilpot reviewed Aug 19, 2016
View reviewed changes

made getPropertyViaGetter cleaner

c25ed3e

removed unnecessary catch block in getPropertyViaGetterUsingPrefix

97b6358

removed unused parameters from methods

8c0bd48

markphilpot merged commit ad26987 into graphql-java:master Aug 20, 2016

dminkovsky mentioned this pull request Sep 1, 2016

PropertyDataFetcher supports AutoValue style classes #149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #130 - Adds support for Boolean property accessors with 'get' prefix in PropertyDataFetcher #164

Fix for #130 - Adds support for Boolean property accessors with 'get' prefix in PropertyDataFetcher #164

ayhanap commented Jun 15, 2016

markphilpot Aug 19, 2016

ayhanap Aug 20, 2016

markphilpot commented Aug 19, 2016

aschrijver commented Aug 19, 2016 •

edited

dminkovsky commented Aug 19, 2016

ayhanap commented Aug 19, 2016

ayhanap commented Aug 19, 2016

dminkovsky commented Aug 19, 2016

dminkovsky commented Aug 19, 2016

ayhanap commented Aug 19, 2016

dminkovsky commented Aug 20, 2016 •

edited

ayhanap commented Aug 20, 2016

aschrijver commented Aug 20, 2016 •

edited

aschrijver commented Aug 20, 2016

ayhanap commented Aug 20, 2016

aschrijver commented Aug 20, 2016

dminkovsky commented Aug 20, 2016 •

edited

aschrijver commented Aug 20, 2016 •

edited

ayhanap commented Aug 20, 2016

dminkovsky commented Aug 21, 2016

aschrijver commented Aug 21, 2016

markphilpot commented Aug 21, 2016

dminkovsky commented Aug 21, 2016

aschrijver commented Aug 21, 2016

Fix for #130 - Adds support for Boolean property accessors with 'get' prefix in PropertyDataFetcher #164

Fix for #130 - Adds support for Boolean property accessors with 'get' prefix in PropertyDataFetcher #164

Conversation

ayhanap commented Jun 15, 2016

markphilpot Aug 19, 2016

Choose a reason for hiding this comment

ayhanap Aug 20, 2016

Choose a reason for hiding this comment

markphilpot commented Aug 19, 2016

aschrijver commented Aug 19, 2016 • edited

dminkovsky commented Aug 19, 2016

ayhanap commented Aug 19, 2016

ayhanap commented Aug 19, 2016

dminkovsky commented Aug 19, 2016

dminkovsky commented Aug 19, 2016

ayhanap commented Aug 19, 2016

dminkovsky commented Aug 20, 2016 • edited

ayhanap commented Aug 20, 2016

aschrijver commented Aug 20, 2016 • edited

aschrijver commented Aug 20, 2016

ayhanap commented Aug 20, 2016

aschrijver commented Aug 20, 2016

dminkovsky commented Aug 20, 2016 • edited

aschrijver commented Aug 20, 2016 • edited

ayhanap commented Aug 20, 2016

dminkovsky commented Aug 21, 2016

aschrijver commented Aug 21, 2016

markphilpot commented Aug 21, 2016

dminkovsky commented Aug 21, 2016

aschrijver commented Aug 21, 2016

aschrijver commented Aug 19, 2016 •

edited

dminkovsky commented Aug 20, 2016 •

edited

aschrijver commented Aug 20, 2016 •

edited

dminkovsky commented Aug 20, 2016 •

edited

aschrijver commented Aug 20, 2016 •

edited