Skip to content

Add Input for supporting nulifying input object #30

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 18, 2017
Merged

Add Input for supporting nulifying input object #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions codegen/lib/graphql_java_gen/templates/APISchema.java.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import com.shopify.graphql.support.Error;
import com.shopify.graphql.support.Query;
import com.shopify.graphql.support.SchemaViolationError;
import com.shopify.graphql.support.TopLevelResponse;
import com.shopify.graphql.support.Input;
<% imports.each do |import| %>
import <%= import %>;
<% end %>
Expand Down Expand Up @@ -264,8 +265,7 @@ public class <%= schema_name %> {
private <%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>;
<% end %>
<% type.optional_input_fields.each do |field| %>
private <%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>;
private boolean <%= field.camelize_name %>Seen = false;
private Input<<%= java_input_type(field.type) %>> <%= escape_reserved_word(field.camelize_name) %> = Input.undefined();
<% end %>

<% unless type.required_input_fields.empty? %>
Expand All @@ -291,19 +291,23 @@ public class <%= schema_name %> {
<% type.optional_input_fields.each do |field| %>
<%= java_annotations(field) %>
public <%= java_input_type(field.type) %> get<%= field.classify_name %>() {
return <%= escape_reserved_word(field.camelize_name) %>.getValue();
}

public Input<<%= java_input_type(field.type) %>> get<%= field.classify_name %>Input() {
return <%= escape_reserved_word(field.camelize_name) %>;
}

public <%= type.name %> set<%= field.classify_name %>(<%= java_annotations(field, in_argument: true) %><%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>) {
this.<%= escape_reserved_word(field.camelize_name) %> = <%= escape_reserved_word(field.camelize_name) %>;
this.<%= field.camelize_name %>Seen = true;
this.<%= escape_reserved_word(field.camelize_name) %> = Input.value(<%= escape_reserved_word(field.camelize_name) %>);
return this;
}

// Unsets the <%= escape_reserved_word(field.camelize_name) %> property so that it is not serialized.
public <%= type.name %> unset<%= field.classify_name %>() {
this.<%= escape_reserved_word(field.camelize_name) %> = null;
this.<%= field.camelize_name %>Seen = false;
public <%= type.name %> set<%= field.classify_name %>Input(Input<<%= java_input_type(field.type) %>> <%= escape_reserved_word(field.camelize_name) %>) {
if (<%= escape_reserved_word(field.camelize_name) %> == null) {
throw new IllegalArgumentException("Input can not be null");
}
this.<%= escape_reserved_word(field.camelize_name) %> = <%= escape_reserved_word(field.camelize_name) %>;
return this;
}

Expand All @@ -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) %>.isDefined()) {
_queryBuilder.append(separator);
separator = ",";
_queryBuilder.append("<%= field.name %>:");
if (<%= escape_reserved_word(field.camelize_name) %> != null) {
<%= generate_build_input_code(escape_reserved_word(field.camelize_name), field.type) %>
if (<%= escape_reserved_word(field.camelize_name) %>.getValue() != null) {
<%= generate_build_input_code(escape_reserved_word(field.camelize_name).concat(".getValue()"), field.type) %>
} else {
_queryBuilder.append("null");
}
Expand Down
34 changes: 34 additions & 0 deletions support/src/main/java/com/shopify/graphql/support/Input.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.shopify.graphql.support;

import java.io.Serializable;

/**
* Created by henrytao on 9/7/17.
*/

public final class Input<T> implements Serializable {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. This is used internally in gen tool, what new features we are expecting to be added via inheritance and why?
  2. 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)
  3. Mocking what you want to mock, 2 values? Why not just use instance without mocking.
  4. 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

Copy link

Choose a reason for hiding this comment

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

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


private final T value;
private final boolean defined;

public static <T> Input<T> value(@Nullable T value) {
return new Input<>(value, true);
}

public static <T> Input<T> undefined() {
return new Input<>(null, false);
}

private Input(T value, boolean defined) {
this.value = value;
this.defined = defined;
}

public T getValue() {
return value;
}

public boolean isDefined() {
return defined;
}
}
76 changes: 43 additions & 33 deletions support/src/test/java/com/shopify/graphql/support/Generated.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.shopify.graphql.support.Query;
import com.shopify.graphql.support.SchemaViolationError;
import com.shopify.graphql.support.TopLevelResponse;
import com.shopify.graphql.support.Input;

import com.shopify.graphql.support.ID;

Expand Down Expand Up @@ -1076,14 +1077,11 @@ public static class SetIntegerInput implements Serializable {

private int value;

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

private Boolean negate;
private boolean negateSeen = false;
private Input<Boolean> negate = Input.undefined();

private String apiClient;
private boolean apiClientSeen = false;
private Input<String> apiClient = Input.undefined();

public SetIntegerInput(String key, int value) {
this.key = key;
Expand Down Expand Up @@ -1111,55 +1109,67 @@ public SetIntegerInput setValue(int value) {

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

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

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

// Unsets the ttl property so that it is not serialized.
public SetIntegerInput unsetTtl() {
this.ttl = null;
this.ttlSeen = false;
public SetIntegerInput setTtlInput(Input<LocalDateTime> ttl) {
if (ttl == null) {
throw new IllegalArgumentException("Input can not be null");
}
this.ttl = ttl;
return this;
}

@Nullable
public Boolean getNegate() {
return negate.getValue();
}

public Input<Boolean> getNegateInput() {
return negate;
}

public SetIntegerInput setNegate(@Nullable Boolean negate) {
this.negate = negate;
this.negateSeen = true;
this.negate = Input.value(negate);
return this;
}

// Unsets the negate property so that it is not serialized.
public SetIntegerInput unsetNegate() {
this.negate = null;
this.negateSeen = false;
public SetIntegerInput setNegateInput(Input<Boolean> negate) {
if (negate == null) {
throw new IllegalArgumentException("Input can not be null");
}
this.negate = negate;
return this;
}

@Nullable
public String getApiClient() {
return apiClient.getValue();
}

public Input<String> getApiClientInput() {
return apiClient;
}

public SetIntegerInput setApiClient(@Nullable String apiClient) {
this.apiClient = apiClient;
this.apiClientSeen = true;
this.apiClient = Input.value(apiClient);
return this;
}

// Unsets the apiClient property so that it is not serialized.
public SetIntegerInput unsetApiClient() {
this.apiClient = null;
this.apiClientSeen = false;
public SetIntegerInput setApiClientInput(Input<String> apiClient) {
if (apiClient == null) {
throw new IllegalArgumentException("Input can not be null");
}
this.apiClient = apiClient;
return this;
}

Expand All @@ -1177,34 +1187,34 @@ public void appendTo(StringBuilder _queryBuilder) {
_queryBuilder.append("value:");
_queryBuilder.append(value);

if (this.ttlSeen) {
if (this.ttl.isDefined()) {
_queryBuilder.append(separator);
separator = ",";
_queryBuilder.append("ttl:");
if (ttl != null) {
Query.appendQuotedString(_queryBuilder, ttl.toString());
if (ttl.getValue() != null) {
Query.appendQuotedString(_queryBuilder, ttl.getValue().toString());
} else {
_queryBuilder.append("null");
}
}

if (this.negateSeen) {
if (this.negate.isDefined()) {
_queryBuilder.append(separator);
separator = ",";
_queryBuilder.append("negate:");
if (negate != null) {
_queryBuilder.append(negate);
if (negate.getValue() != null) {
_queryBuilder.append(negate.getValue());
} else {
_queryBuilder.append("null");
}
}

if (this.apiClientSeen) {
if (this.apiClient.isDefined()) {
_queryBuilder.append(separator);
separator = ",";
_queryBuilder.append("api_client:");
if (apiClient != null) {
Query.appendQuotedString(_queryBuilder, apiClient.toString());
if (apiClient.getValue() != null) {
Query.appendQuotedString(_queryBuilder, apiClient.getValue().toString());
} else {
_queryBuilder.append("null");
}
Expand Down
1 change: 1 addition & 0 deletions support/src/test/java/com/shopify/graphql/support/GeneratedMinimal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.shopify.graphql.support.Query;
import com.shopify.graphql.support.SchemaViolationError;
import com.shopify.graphql.support.TopLevelResponse;
import com.shopify.graphql.support.Input;

import com.shopify.graphql.support.ID;

Expand Down
21 changes: 19 additions & 2 deletions support/src/test/java/com/shopify/graphql/support/IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.shopify.graphql.support;

import java.time.LocalDate;
import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -166,10 +167,26 @@ public void testOptionalFieldOnInput() throws Exception {
}

@Test
public void testUnsetOptionalFieldOnInput() throws Exception {
public void testOptionalFieldOnInputAsUndefined() throws Exception {
String queryString = Generated.mutation(mutation -> mutation
.setInteger(new Generated.SetIntegerInput("answer", 42).setTtl(null).unsetTtl())
.setInteger(new Generated.SetIntegerInput("answer", 42).setTtlInput(Input.<LocalDateTime>undefined()))
).toString();
assertEquals("mutation{set_integer(input:{key:\"answer\",value:42})}", queryString);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

).toString();
assertEquals("mutation{set_integer(input:{key:\"answer\",value:42,ttl:null})}", queryString);
}

@Test
public void testOptionalFieldOnInputAsInputValue() throws Exception {
String queryString = Generated.mutation(mutation -> mutation
.setInteger(new Generated.SetIntegerInput("answer", 42).setTtlInput(Input.<LocalDateTime>value(LocalDateTime.of(2017, 1, 31, 10, 9, 48))))
).toString();
assertEquals("mutation{set_integer(input:{key:\"answer\",value:42,ttl:\"2017-01-31T10:09:48\"})}", queryString);
}
}