Make explicit NULL easy – and break less API #12

BenEmdon · 2017-09-07T01:48:37Z

This PR

The mirror of Shopify/graphql_java_gen#29
In this PR I am adding a way to unset properties so that they aren't serialized. This change is required because currently there is no way to tell a property to not serialize.
This is aimed to help consumers of this generator that were effected by the change #8 that used to reset their input types with inputType.property = null so that they can use inputType.unsetProperty() instead.

This PR has pivoted to be about making #8 cause less API breakage.

~~adds a public unset{property name} method~~
makes required properties required non optional arguments in the init

The new input type looks like this

class SomeClass {

  // required
  var id: Int

  // nullable
  var negate: Bool? {
    didSet {
      serailzeNegate = true
    }
  }

  var serialzeNegate: Bool

  init(
    id: int,
    negate: Bool? = nil
    serialzieNegate: Bool = false) {

    self.id = id

    self.serializeNegate = serailizeNegate
    if let negate = negate {
      self.negate = negate
      self.serializeNegate = true
    }
  }
  ...

Added tests
Minimize API breakage

…ing serialized if explicitly called

alinebee · 2017-09-07T04:14:52Z

Given that swift supports unicode in function and variable names, I humbly suggest that unset{property}() be renamed to 🙈{property}().

dylanahsmith · 2017-09-07T14:25:21Z

It seems like the API is kind of confusing with negate: nil in an initializer meaning don't set the input and input.negate = nil meaning send a null.

If sending an explicit null is the edge case, considering that we didn't need it until recently, would it make more sense to have a special method to set the input value to null rather than having a special method to unset it. At least that way negate: nil and input.negate = nil would consistently mean unset the input field's value.

Change {field name}Seen => serialize{field name} Expose serialize{field name} publicly

BenEmdon · 2017-09-07T18:36:52Z

Updated the description about the new proposal. Can I get a rereview?

dylanahsmith · 2017-09-07T19:23:38Z

codegen/lib/graphql_swift_gen/templates/type.swift.erb

            self.<%= field_name %> = <%= field_name %>
+            self.serialize<%= escape_reserved_word(field.classify_name) %> = true


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

No, didSet observers arent triggered in initializers

alinebee

Some questions about what the hell is going on because I'm not too familiar with erb syntax. I like the change from propertySeen to serializeProperty - makes the intent much clearer.

alinebee · 2017-09-07T19:48:30Z

codegen/lib/graphql_swift_gen/templates/type.swift.erb

          <% 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<%= seperator %>


Spelling: separator

alinebee · 2017-09-07T19:54:18Z

codegen/lib/graphql_swift_gen/templates/type.swift.erb

@@ -214,36 +214,41 @@ extension <%= schema_name %> {
  <% when 'INPUT_OBJECT' %>
    open class <%= type.name %> {
      <% type.required_input_fields.each do |field| %>


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.

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.

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.

I replied saying the same thing as Dylan 😂

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.

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 }

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

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.

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.)

dylanahsmith · 2017-09-07T21:06:16Z

Some questions about what the hell is going on because I'm not too familiar with erb syntax.

Perhaps we should check the generated code into git as we do in the java gem Shopify/graphql_java_gen#17, since it can sometimes be easier to review the generated code.

BenEmdon · 2017-09-07T21:15:08Z

Perhaps we should check the generated code into git as we do in the java gem Shopify/graphql_java_gen#17, since it can sometimes be easier to review the generated code.

I totally agree. Do we currently have a way of doing that for the swift repo?

BenEmdon · 2017-09-08T16:59:04Z

Update:

I am going to talk to @dbart01 this afternoon and see how we align on the API

BenEmdon · 2017-09-12T20:21:56Z

Closing this in favor of #15

Added an unset method for each nullable field to prevent them from be…

d8c0c03

…ing serialized if explicitly called

BenEmdon requested review from dylanahsmith and wesleyjellis September 7, 2017 01:48

Remove all instances of unset

bbc43a0

Change {field name}Seen => serialize{field name} Expose serialize{field name} publicly

BenEmdon force-pushed the unset-nullable-field branch from 9e678b7 to bbc43a0 Compare September 7, 2017 17:31

BenEmdon changed the title ~~Add a way to unset an input property~~ Make explicit NULL easy – and break less API Sep 7, 2017

BenEmdon requested a review from raulriera September 7, 2017 18:36

BenEmdon mentioned this pull request Sep 7, 2017

Convert needlessly public methods and properties to internal. #3

Closed

dylanahsmith approved these changes Sep 7, 2017

View reviewed changes

alinebee reviewed Sep 7, 2017

View reviewed changes

sperator => separator

e39314a

BenEmdon requested a review from alinebee September 8, 2017 14:20

BenEmdon mentioned this pull request Sep 8, 2017

Explicit nulls on input types using InputValue wrapper #15

Merged

dylanahsmith mentioned this pull request Sep 11, 2017

Commit generated code for tests to provide more information to reviewers #17

Merged

BenEmdon closed this Sep 12, 2017

BenEmdon deleted the unset-nullable-field branch September 12, 2017 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make explicit NULL easy – and break less API #12

Make explicit NULL easy – and break less API #12

BenEmdon commented Sep 7, 2017 •

edited

Loading

alinebee commented Sep 7, 2017

dylanahsmith commented Sep 7, 2017

BenEmdon commented Sep 7, 2017

dylanahsmith Sep 7, 2017

BenEmdon Sep 7, 2017

alinebee left a comment

alinebee Sep 7, 2017

alinebee Sep 7, 2017

dylanahsmith Sep 7, 2017

BenEmdon Sep 7, 2017 •

edited

Loading

BenEmdon Sep 7, 2017

alinebee Sep 8, 2017

BenEmdon Sep 8, 2017 •

edited

Loading

alinebee Sep 8, 2017

BenEmdon Sep 8, 2017 •

edited

Loading

alinebee Sep 8, 2017

dylanahsmith commented Sep 7, 2017

BenEmdon commented Sep 7, 2017

BenEmdon commented Sep 8, 2017

BenEmdon commented Sep 12, 2017

		self.<%= field_name %> = <%= field_name %>
		self.serialize<%= escape_reserved_word(field.classify_name) %> = true

Make explicit NULL easy – and break less API #12

Make explicit NULL easy – and break less API #12

Conversation

BenEmdon commented Sep 7, 2017 • edited Loading

This PR

alinebee commented Sep 7, 2017

dylanahsmith commented Sep 7, 2017

BenEmdon commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinebee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenEmdon Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenEmdon Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenEmdon Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanahsmith commented Sep 7, 2017

BenEmdon commented Sep 7, 2017

BenEmdon commented Sep 8, 2017

Update:

BenEmdon commented Sep 12, 2017

BenEmdon commented Sep 7, 2017 •

edited

Loading

BenEmdon Sep 7, 2017 •

edited

Loading

BenEmdon Sep 8, 2017 •

edited

Loading

BenEmdon Sep 8, 2017 •

edited

Loading