Add Input for supporting nulifying input object #30

henrytao-me · 2017-09-14T20:16:40Z

#This PR supports nulifying for optional input type fields. It also removes seen flag.

Sample generated code

public static class SetIntegerInput implements Serializable {

    private String key;

    private int value;

    private Input<LocalDateTime> ttl = Input.undefined();

    public SetIntegerInput(String key, int value) {
        this.key = key;
        this.value = value;
    }

    public String getKey() {
        return key;
    }

    public SetIntegerInput setKey(String key) {
        this.key = key;
        return this;
    }

    public int getValue() {
        return value;
    }

    public SetIntegerInput setValue(int value) {
        this.value = value;
        return this;
    }

    @Nullable
    public LocalDateTime getTtl() {
        return ttl.value;
    }

    public Input<LocalDateTime> getTtlInput() {
        return ttl;
    }

    public SetIntegerInput setTtl(@Nullable LocalDateTime ttl) {
        this.ttl = Input.value(ttl);
        return this;
    }

    public SetIntegerInput setTtlInput(Input<LocalDateTime> ttl) {
        if (ttl == null) {
            throw new IllegalArgumentException("Input can not be null");
        }
        this.ttl = ttl;
        return this;
    }

    public void appendTo(StringBuilder _queryBuilder) {
        String separator = "";
        _queryBuilder.append('{');

        _queryBuilder.append(separator);
        separator = ",";
        _queryBuilder.append("key:");
        Query.appendQuotedString(_queryBuilder, key.toString());

        _queryBuilder.append(separator);
        separator = ",";
        _queryBuilder.append("value:");
        _queryBuilder.append(value);

        if (this.ttl.defined || this.ttl.value != null) {
            _queryBuilder.append(separator);
            separator = ",";
            _queryBuilder.append("ttl:");
            if (ttl.value != null) {
                Query.appendQuotedString(_queryBuilder, ttl.value.toString());
            } else {
                _queryBuilder.append("null");
            }
        }

        _queryBuilder.append('}');
    }
}

DanielJette · 2017-09-14T20:17:50Z

Could you please include a snippet of the generated code in your PR description?

BenEmdon

You need to run rake generate . Also you should probably add and update tests 🛠 Take a look at #28 and #29 for some context.

henrytao-me · 2017-09-14T20:36:55Z

@BenEmdon I just fixed the tests

BenEmdon · 2017-09-14T20:38:22Z

Might be worth adding some extra tests with the introduction of this new wrapper type. We did the same in https://github.com/Shopify/graphql_swift_gen/pull/15/files#diff-421decf1011f80ddb8a434c8e7d13e17R71.

henrytao-me · 2017-09-14T22:21:42Z

@BenEmdon Is this what you need?

graphql_java_gen/support/src/test/java/com/shopify/graphql/support/IntegrationTest.java

Line 170 in 8cd61aa

public void testUnsetOptionalFieldOnInput() throws Exception {

wesleyjellis · 2017-09-15T18:09:43Z

support/src/main/java/com/shopify/graphql/support/Input.java

+
+public final class Input<T> implements Serializable {
+
+  public final T value;


I think I would prefer to use a getter here instead of a public field. Give us more flexibility in the future.

Also, should this be final? Do you have to construct a new input object if you want to change the wrapped value?

Yes it was an intention to make it immutable, following by best practises and similar to java Optional, guava Optional types.

And as for the getter 👍

wesleyjellis · 2017-09-15T18:11:58Z

support/src/test/java/com/shopify/graphql/support/Generated.java

+            return ttl.value;
+        }
+
+        public Input<LocalDateTime> getTtlInput() {


Should we unrwap these automatically to the underlying type automatically?

we have a separate getter, look line 1112

wesleyjellis · 2017-09-15T18:14:36Z

codegen/lib/graphql_java_gen/templates/APISchema.java.erb

@@ -319,12 +323,12 @@ public class <%= schema_name %> {
                    <%= generate_build_input_code(escape_reserved_word(field.camelize_name), field.type) %>
                  <% end %>
                  <% type.optional_input_fields.each do |field| %>
-                    if (this.<%= field.camelize_name %>Seen) {
+                    if (this.<%= escape_reserved_word(field.camelize_name) %>.defined || this.<%= escape_reserved_word(field.camelize_name) %>.value != null) {


Does the second part of this conditional make sense? If defined is false but there is a value I would argue that we shouldn't serialize it

~~what do you mean? if defined is not false OR value not null then we serialize. There won't be case when we have defined == false and it has a value as there is no such constructor in Input~~

Good point @wesleyjellis. The second part does not make sense anymore after we simplified the Input class. Just have quick discussion with @sav007 and we agreed 👍

sav007 · 2017-09-18T14:44:06Z

@BenEmdon @dylanahsmith @see-mack @DanielJette any other comments / suggestions?

wesleyjellis

Small comment, but otherwise this looks good

wesleyjellis · 2017-09-18T15:48:38Z

support/src/main/java/com/shopify/graphql/support/Input.java

+ * Created by henrytao on 9/7/17.
+ */
+
+public final class Input<T> implements Serializable {


Personally, I don't like final classes because it restricts what future developers can do and it makes mocking impossible

Agree that in some cases it makes more sense to make them open, but not here as:

This is used internally in gen tool, what new features we are expecting to be added via inheritance and why?

This class is implemented in a way that explicitly restricts the inheritance as we it always adds complexity of correctly supporting inheritance. Plus it's a value type (like sealed classes in Kotlin or enum in swift)

Mocking what you want to mock, 2 values? Why not just use instance without mocking.

From best practises by default classes should be closed (look for Kotlin, or Swift). And only make them open when you really need it and know what are you doing

I guess it is mostly just a value class, I just don't see the value of restricting future developers from extending this. It's not like this is a super complicated class or is super critical.

I tend to default to trusting (future) developers to do the right thing.

Anyway, I am turning into a 🚲 🏠 discussion. This works fine

BenEmdon

Code looks good 👍 Like the implementation. It's very similar to what's happinging over at Shopify/graphql_swift_gen.

Just one comment about tests that I feel is worth addressing. 👇

BenEmdon · 2017-09-18T15:52:44Z

support/src/test/java/com/shopify/graphql/support/IntegrationTest.java

@@ -168,7 +169,7 @@ public void testOptionalFieldOnInput() throws Exception {
    @Test
    public void testUnsetOptionalFieldOnInput() throws Exception {


This test should probably be renamed to testOptionalFieldOnInputAsUndefined. It would also be worth having tests testOptionalFieldOnInputAsExplicitNull and testOptionalFieldOnInputAsInputValue.

Okie. I will add them.

dylanahsmith · 2017-09-18T17:37:22Z

Doesn't #29 already provide support for nullifying an input object? Is this just about changing the way that is done to make it consistent with graphql_swift_gen?

dylanahsmith · 2017-09-18T17:39:53Z

support/src/test/java/com/shopify/graphql/support/IntegrationTest.java

+    @Test
+    public void testOptionalFieldOnInputAsExplicitNull() throws Exception {
+        String queryString = Generated.mutation(mutation -> mutation
+          .setInteger(new Generated.SetIntegerInput("answer", 42).setTtlInput(Input.<LocalDateTime>value(null)))


Why do this instead of what is done in testOptionalFieldOnInput which uses .setTtl(null)) instead of .setTtlInput(Input.<LocalDateTime>value(null)))?

It tests different way to set null as explicit input value to be sent to the server

What I don't understand is why we are adding another way to do this when we already have a way.

You mean why do we have another setter like setTtlInput(Input<LocalDateTime>) ?

To be able to do this: setTtlInput(Input.undefined()) that means ttl is undefined don't send it to the server at all (don't send null).

Why do we have 2 of them? Because of MBSDK depends on this and if we remove existing one it means breaking changes. So we keep 2 of them for now.

sav007 · 2017-09-18T17:45:54Z

@dylanahsmith we decided to go with this approach for different reasons, and to be aligned with swift one of them.

dylanahsmith · 2017-09-18T17:56:43Z

we decided to go with this approach for different reasons

What reasons? None have been given in this PR other than alignment with swift

to be aligned with swift one of them.

Specifying input arguments is already different in java since the builder pattern was needed to specify arguments due to the lack of support for keyword arguments in java. However, java doesn't have the problem that swift had where we couldn't distinguish between a nil value explicitly being passed in as a keyword argument and one coming from the argument's default value.

It feels like we are working around a problem that the java implementation doesn't have.

sav007 · 2017-09-18T18:02:40Z

Sorry if we didn't let you know but we had a lot of discussion in Slack and in this issue: https://github.com/Shopify/android/pull/721

And decision been made to go with this PR.

sav007 · 2017-09-18T18:08:43Z

Specifying input arguments is already different in java since the builder pattern was needed to specify arguments due to the lack of support for keyword arguments in java. However, java doesn't have the problem that swift had where we couldn't distinguish between a nil value explicitly being passed in as a keyword argument and one coming from the argument's default value.
It feels like we are working around a problem that the java implementation doesn't have.

Sorry I'm not following you. The problem is that we have to differentiate 2 meanings of null value:
1 null was set explicitly (send it to the server as field: null), 2 null means value is not defined (we shouldn't send field: null to the server, otherwise it will nullify the field). So here we actually have 3 boolean logic:

value set to some != null (send to server)
value set to null (send to server)
value is unset (DON"T send to server)

dylanahsmith · 2017-09-18T18:25:29Z

The query builder pattern represents all three of those states. Using ttl as an example:

.setTtl(ttl) to send some non-null value to the server
.setTtl(null) to send a null value to the server
omit the call to .setTtl to not send the value to the server or use .unsetTtl()

so again, this PR doesn't add the ability to support sending null or not specify a value for a field.

Perhaps that approach was confusing. Also, it looks like handling input that may or may not be specified would be more annoying since it would require a conditional to either avoid the call to .setTtl or to conditionally use .unsetTtl()

sav007 · 2017-09-18T18:47:07Z

the API was really confusing, like set to null what it really means? Will it be sent or not. And by default all values are null but they won't be sent to server. Then how to reset field to null that is not going to be sent. With Input value type the API more explicit.

dylanahsmith · 2017-09-18T18:56:31Z

the API was really confusing, like set to null what it really means?

Yes, I would say the GraphQL API is confusing to distinguish between these. It isn't clear what it means. However, it does make sense that .setTtl(null) would set the value to null instead of it not being set.

And by default all values are null but they won't be sent to server.

Oh right, those getters would be very confusing and didn't provide a way to find out if a field was unset or set to null.

Then how to reset field to null that is not going to be sent.

.setTtl(null) seems clear that it would set something that wasn't set or was unset.

henrytao-me requested review from dylanahsmith, see-mack, DanielJette, BenEmdon and wesleyjellis September 14, 2017 20:16

BenEmdon suggested changes Sep 14, 2017

View reviewed changes

henrytao-me mentioned this pull request Sep 14, 2017

Add support of nulifying input object property values Shopify/mobile-buy-sdk-android#444

Merged

wesleyjellis reviewed Sep 15, 2017

View reviewed changes

wesleyjellis approved these changes Sep 18, 2017

View reviewed changes

BenEmdon approved these changes Sep 18, 2017

View reviewed changes

dylanahsmith reviewed Sep 18, 2017

View reviewed changes

Add Input class for supporting nulifying input object

743b016

henrytao-me force-pushed the buysdk branch from 38187ee to 743b016 Compare September 18, 2017 18:08

henrytao-me force-pushed the buysdk branch from f2a2bba to 743b016 Compare September 18, 2017 18:27

dylanahsmith approved these changes Sep 18, 2017

View reviewed changes

henrytao-me merged commit 9b4eb32 into master Sep 18, 2017

henrytao-me deleted the buysdk branch September 18, 2017 19:05


		public final class Input<T> implements Serializable {

		public final T value;

		@@ -168,7 +169,7 @@ public void testOptionalFieldOnInput() throws Exception {
		@Test
		public void testUnsetOptionalFieldOnInput() throws Exception {

Add Input for supporting nulifying input object #30

Add Input for supporting nulifying input object #30

Uh oh!

Conversation

henrytao-me commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielJette commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenEmdon left a comment

Choose a reason for hiding this comment

Uh oh!

henrytao-me commented Sep 14, 2017

Uh oh!

BenEmdon commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrytao-me commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sav007 Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sav007 Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sav007 commented Sep 18, 2017

Uh oh!

wesleyjellis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenEmdon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanahsmith commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sav007 commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylanahsmith commented Sep 18, 2017

Uh oh!

sav007 commented Sep 18, 2017

Uh oh!

sav007 commented Sep 18, 2017

Uh oh!

dylanahsmith commented Sep 18, 2017

Uh oh!

henrytao-me commented Sep 14, 2017 •

edited

Loading

DanielJette commented Sep 14, 2017 •

edited

Loading

BenEmdon commented Sep 14, 2017 •

edited

Loading

sav007 Sep 15, 2017 •

edited

Loading

sav007 Sep 15, 2017 •

edited

Loading

BenEmdon left a comment •

edited

Loading

sav007 commented Sep 18, 2017 •

edited

Loading