Skip to content

Make explicit NULL easy – and break less API #12

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

Closed
wants to merge 3 commits into from
Closed

Make explicit NULL easy – and break less API #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8c0c03
Added an unset method for each nullable field to prevent them from be…
BenEmdon Sep 7, 2017
bbc43a0
Remove all instances of unset
BenEmdon Sep 7, 2017
e39314a
sperator => separator
BenEmdon Sep 7, 2017
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
21 changes: 13 additions & 8 deletions codegen/lib/graphql_swift_gen/templates/type.swift.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,36 +214,41 @@ extension <%= schema_name %> {
<% when 'INPUT_OBJECT' %>
open class <%= type.name %> {
<% type.required_input_fields.each do |field| %>
Copy link

Choose a reason for hiding this comment

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

What determines the set of required vs optional fields?

Do I understand correctly that optional means "field can be omitted from the mutation" rather than "field value is nullable", and that required means "field must be present in the mutation, whether it's nullable or not"?

If that's correct, isn't the change below this forcing all required fields to be non-nullable as well?

Would love some code documentation around this to clarify the implications.

Copy link
Contributor

Choose a reason for hiding this comment

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

The GraphQL schema doesn't distinguish between "field can be omitted from the mutation" rather than "field value is nullable". The name optional used to be more appropriate before graphql allowed a null input value to be specified. So required actually means the field is non-null in schema and optional that it isn't non-null in the schema. So it is correct for a required field to also no be nullable.

Copy link
Contributor Author

@BenEmdon BenEmdon Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In GraphQL there is no distinction between "the field can be omitted from the mutation" and "the field value is nullable". They are one and the same (in the type system) so there is no way we can tell the difference.
In practice you have fields that are implicitly required but in GraphQL are specified as nullable (not very nice right?). For example in a ProductInput the title property is nullable in GraphQL but if you tried to update a product with a null title we would send you a userError.

And yes the change below is forcing all required fields to be non nullable because if you make a mutation without a required field the mutation will always fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replied saying the same thing as Dylan 😂

Copy link

Choose a reason for hiding this comment

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

For example in a ProductInput the title property is nullable in GraphQL but if you tried to update a product with a null title we would send you a userError.

What about update mutations that omit the title field altogether? I thought that was allowed, and was what we wanted, so that we wouldn't overwrite fields that we didn't modify ourselves but that might have been changed by another mutation.

Copy link
Contributor Author

@BenEmdon BenEmdon Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That is still possible with this PR. If you want to omit the title field all together you can do it multiple ways.

// init with nil
let input1 = inputType(id: 1, title: nil)
print(input1.serialize) // { id: 1 }
// ☝️ this is because the default value for serializeTitle is false

// init with value but set serialize to false
let input2 = inputType(id: 2, title: "Great thing", serializeTitle: false)
print(input2.serialize) // { id: 2 }

// init with value but set serialize to false later
let input3 = inputType(id: 3, title: "Great thing")
input3.serializeTitle = false
print(input3.serialize) // { id: 3 }

Copy link

Choose a reason for hiding this comment

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

I thought title was a required field, and so wasn't generating serializeTitle at all? 😕

Copy link
Contributor Author

@BenEmdon BenEmdon Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry in the example I was treating title as a non required field. If Title were a required field it would have to be sent to the server.

Copy link

Choose a reason for hiding this comment

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

OK, after an in-person discussion with @BenEmdon I have a better handle on the rules now:

  • optional fields are nillable and expose serialize{Field}.

  • required fields are non-nil and do not expose serialize{Field}.

  • fields can only be required on Input objects that are creation-only.
    e.g. FulfillmentInput

  • Input objects that are used for both creation and update need all their fields marked as optional so that we can express minimal changesets.
    e.g. ProductInput

  • If a creation mutation omits a field which is optional, but which is implicitly required for creation, the mutation will fail with a user error.
    e.g. trying to create a product with a ProductInput that has no title

  • If an update mutation omits a field which is optional, but which is implicitly required for update, then the mutation will also fail with a user error (even though this is really a programmer error.)
    e.g. trying to edit an existing product with a ProductInput that has no id
    (This example will be solved once we refactor update mutations to pass id as a separate parameter instead of making it part of the Input object.)

open var <%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type) %>

open var <%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type, non_null: true) %>
<% end %>
<% type.optional_input_fields.each do |field| %>

open var <%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type) %> {
didSet {
<%= escape_reserved_word(field.camelize_name) %>Seen = true
serialize<%= escape_reserved_word(field.classify_name) %> = true
}
}
private var <%= escape_reserved_word(field.camelize_name) %>Seen: Bool = false
// Observes if the <%= escape_reserved_word(field.camelize_name) %> property was set, and indicates whether it should be serialized.
open var serialize<%= escape_reserved_word(field.classify_name) %>: Bool
<% end %>

public init(
<% input_fields = type.required_input_fields + type.optional_input_fields # manually ordered with required fields first %>
<% input_fields.each do |field| %>
<% seperator = field == input_fields.last ? "" : "," %>
<% separator = field == input_fields.last ? "" : "," %>
<% if field.type.non_null? %>
<%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type) %><%= seperator %>
<%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type, non_null: true) %><%= separator %>
<% else %>
<%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type) %>? = nil<%= seperator %>
<%= escape_reserved_word(field.camelize_name) %>: <%= swift_input_type(field.type) %> = nil,
serialize<%= escape_reserved_word(field.classify_name) %>: Bool = false<%= separator %>
<% end %>
<% end %>
) {
<% type.required_input_fields.each do |field| %>
self.<%= escape_reserved_word(field.camelize_name) %> = <%= escape_reserved_word(field.camelize_name) %>
<% end %>
<% type.optional_input_fields.each do |field| %>
self.serialize<%= escape_reserved_word(field.classify_name) %> = serialize<%= escape_reserved_word(field.classify_name) %>
<% field_name = escape_reserved_word(field.camelize_name) %>
if let <%= field_name %> = <%= field_name %> {
self.<%= field_name %>Seen = true
self.<%= field_name %> = <%= field_name %>
self.serialize<%= escape_reserved_word(field.classify_name) %> = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the self.<%= field_name %> = <%= field_name %> line already set the serialize value to true in the properties didSet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, didSet observers arent triggered in initializers

}
<% end %>
}
Expand All @@ -254,7 +259,7 @@ extension <%= schema_name %> {
fields.append("<%= field.name %>:<%= generate_build_input_code(field.camelize_name, field.type.unwrap_non_null) %>")
<% end %>
<% type.optional_input_fields.each do |field| %>
if <%= escape_reserved_word(field.camelize_name) %>Seen {
if serialize<%= escape_reserved_word(field.classify_name) %> {
if let <%= escape_reserved_word(field.camelize_name) %> = <%= escape_reserved_word(field.camelize_name) %> {
fields.append("<%= field.name %>:<%= generate_build_input_code(field.camelize_name, field.type.unwrap_non_null) %>")
} else {
Expand Down
12 changes: 10 additions & 2 deletions support/Tests/GraphQLSupportTests/IntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,17 @@ class IntegrationTests: XCTestCase {
XCTAssertEqual(queryString, "mutation{set_integer(input:{key:\"answer\",value:42,negate:true})}")
}

func testInputObjectExplictNilConstructor() {
func testInputObjectOptionalFieldNilConstructor() {
let query = Generated.buildMutation { $0
.setInteger(input: Generated.SetIntegerInput(key: "answer", value: 42, negate: .some(nil)))
.setInteger(input: Generated.SetIntegerInput(key: "answer", value: 42, negate: nil))
}
let queryString = String(describing: query)
XCTAssertEqual(queryString, "mutation{set_integer(input:{key:\"answer\",value:42})}")
}

func testInputObjectOptionalFieldExplictNilConstructor() {
let query = Generated.buildMutation { $0
.setInteger(input: Generated.SetIntegerInput(key: "answer", value: 42, negate: nil, serializeNegate: true))
}
let queryString = String(describing: query)
XCTAssertEqual(queryString, "mutation{set_integer(input:{key:\"answer\",value:42,negate:null})}")
Expand Down