Allow setting optional fields on input objects to explicit null #28
Conversation
|
Seems straightforward enough, but there's room to make it nicer:
|
| @@ -266,6 +267,7 @@ public class <%= schema_name %> { | |||
| <% type.optional_input_fields.each do |field| %> | |||
| private <%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>; | |||
| <% end %> | |||
| private HashSet<String> fieldsSeen = new HashSet<>(); | |||
There was a problem hiding this comment.
This is kind of a weird mix of using generated instance variables and a HashSet. If we are going to generate instance variables, then we should also generate a boolean to indicate that the field has been set. If we wanted a sparse object instead, it would make more sense to just use a HashMap, which could use the key's presence to indicate whether or not the field has been set.
Already done, I add the extra code only for |
| @@ -265,6 +265,7 @@ public class <%= schema_name %> { | |||
| <% end %> | |||
| <% type.optional_input_fields.each do |field| %> | |||
| private <%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>; | |||
| private boolean <%= escape_reserved_word(field.name) %>Seen = false; | |||
There was a problem hiding this comment.
No java reserved words end in Seen, so you don't need escape_reserved_word.
Also, I think you should use field.camelize_name to follow the java naming convention if the schema uses underscore separated words. E.g. if the field in the schema is two_words then the variable would be two_wordsSeen.
| public <%= java_input_type(field.type) %> get<%= field.classify_name %>() { | ||
| return <%= escape_reserved_word(field.camelize_name) %>; | ||
| } | ||
|
|
||
| public <%= type.name %> set<%= field.classify_name %>(<%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>) { | ||
| public <%= type.name %> set<%= field.classify_name %>(<%= java_annotations(field).gsub(/(.)$/, '\1 ') %><%= java_input_type(field.type) %> <%= escape_reserved_word(field.camelize_name) %>) { |
There was a problem hiding this comment.
The .gsub(/(.)$/, '\1 ') replacement is really weird, since it just adds a trailing whitespace at the end of each line in the string.
irb(main)> "one\ntwo\n".gsub(/(.)$/, '\1 ')
=> "one \ntwo \n"
but it looks like what you actually want is to replace the newlines. You could use .tr("\n", " ") instead to do that.
However, why not just add a keyword argument tojava_annotations to control the separator so you don't need to workaround its limitation with string replacements.
There was a problem hiding this comment.
Yes, I think the correct solution is to let java_annotations format differently for arguments vs methods or fields. I'm gonna add some tests for java_annotations while I'm at it
codegen/lib/graphql_java_gen.rb
Outdated
| end.compact.join("\n") | ||
| end.compact | ||
| if in_argument | ||
| annotations.join(" ") + " " |
There was a problem hiding this comment.
this is generating an extra space when there are no annotations
There was a problem hiding this comment.
The change I was thinking of was to just change
- end.compact.join("\n")
+ end.compact.join(separator)where separator is an argument passed into java_annotations. However, it might be even simpler to just have this method return an array and do the .join("\n") or .join(' ') on the return value in the template.
There was a problem hiding this comment.
There's still the problem that we need a trailing space at the end of annotations in argument that I feel would make the templates uglier to deal with it there. I'll just add an early return if there are no annotations
| this.<%= escape_reserved_word(field.camelize_name) %> = <%= escape_reserved_word(field.camelize_name) %>; | ||
| this.<%= escape_reserved_word(field.name) %>Seen = true; |
There was a problem hiding this comment.
This needs to use field.camelize_name without escape_reserved_word for consistency with the instance variable name.
| @@ -306,11 +310,15 @@ 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 (<%= escape_reserved_word(field.camelize_name) %> != null) { | |||
| if (this.<%= escape_reserved_word(field.name) %>Seen) { | |||
There was a problem hiding this comment.
Again, use field.camelize_name without escape_reserved_word to avoid errors if this does get escaped
This change allows you to manually set an optional field to null on an input object.
Change is needed to so that we can delete an a child object in an update mutation. Specifically, CollectionInput needs to support setting CollectionImage to null so that it gets deleted.
Closes #27