Convenience fields of N depth (#1797)

Merged PR #1797.

api/resource.rb

@@ -219,6 +219,18 @@ def gettable_properties
       all_user_properties.reject(&:url_param_only)
     end
 
+    # Returns the list of top-level properties once any nested objects with flatten_object
+    # set to true have been collapsed
+    def root_properties
+      properties.flat_map do |p|
+        if p.flatten_object
+          p.root_properties
+        else
+          p
+        end
+      end
+    end
+
     # Return the product-level async object, or the resource-specific one
     # if one exists.
     def async

api/type.rb

@@ -474,6 +474,18 @@ def nested_properties
         properties
       end
 
+      # Returns the list of top-level properties once any nested objects with
+      # flatten_object set to true have been collapsed
+      def root_properties
+        properties.flat_map do |p|
+          if p.flatten_object
+            p.root_properties
+          else
+            p
+          end
+        end
+      end
+
       def exclude_if_not_in_version!(version)
         super
         @properties.each { |p| p.exclude_if_not_in_version!(version) }

overrides/terraform/property_override.rb

@@ -79,13 +79,22 @@ def self.attributes
           #
           # With great power comes great responsibility.
 
-          # Flatten the child field of a NestedObject into "convenience fields"
-          # that are addressed as if they were top level fields.
+          # Flattens a NestedObject by removing that field from the Terraform
+          # schema but will preserve it in the JSON sent/retreived from the API
+          #
+          # EX: a API schema where fields are nested (eg: `one.two.three`) and we
+          # desire the properties of the deepest nested object (eg: `three`) to
+          # become top level properties in the Terraform schema. By overidding
+          # the properties `one` and `one.two` and setting flatten_object then
+          # all the properties in `three` will be at the root of the TF schema.
           #
           # We need this for cases where a field inside a nested object has a
           # default, if we can't spend a breaking change to fix a misshapen
-          # field, or if the UX is _much_ better otherwise. Nesting flattened
-          # NestedObjects is inadvisable.
+          # field, or if the UX is _much_ better otherwise.
+          #
+          # WARN: only fully flattened properties are currently supported. In the
+          # example above you could not flatten `one.two` without also flattening
+          # all of it's parents such as `one`
           :flatten_object,
 
           # ===========

products/compute/terraform.yaml

@@ -747,6 +747,10 @@ overrides: !ruby/object:Overrides::ResourceOverrides
       routingConfig: !ruby/object:Overrides::Terraform::PropertyOverride
         flatten_object: true
       routingConfig.routingMode: !ruby/object:Overrides::Terraform::PropertyOverride
+        # flattened objects don't pass on the attributes of the parent to
+        # the child so redefining this on the child field is necessary.
+        update_verb: :PATCH
+        update_url:  projects/{{project}}/global/networks/{{name}}
         # while required inside routingConfig, routingMode is an optional convenience
         # field.
         required: false

templates/terraform/expand_property_method.erb

@@ -43,6 +43,7 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T
   }
   return m, nil
 }
+<%# End expanders for Maps %>
 
 <%# Generate expanders for KeyValuePairs %>
 <%   elsif property.is_a?(Api::Type::KeyValuePairs) -%>
@@ -59,11 +60,11 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T
 
 <%# Generate expanders for flattened objects %>
 <%   elsif property.flatten_object -%>
-func expand<%= prefix -%><%= titlelize_property(property) -%>(d TerraformResourceData, config *Config) (interface{}, error) {
+func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {
   transformed := make(map[string]interface{})
 <%     property.nested_properties.each do |prop| -%>
-  // Note that nesting flattened objects won't work because we don't handle them properly here.
-  transformed<%= titlelize_property(prop) -%>, err := expand<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config)
+  <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%>
+  transformed<%= titlelize_property(prop) -%>, err := expand<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config)
   if err != nil {
     return nil, err
 <%       if prop.send_empty_value -%>
@@ -75,8 +76,8 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(d TerraformResourc
 <%       end -%>
   }
 
-  return transformed, nil
 <%     end -%>
+  return transformed, nil
 }
 
 <%# Generate expanders for everything else %>
@@ -158,6 +159,8 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T
 
 <% if property.nested_properties? -%>
 <%   property.nested_properties.each do |prop| -%>
+<%# Map is a map from {key -> object} in the API, but Terraform can't represent that
+so we treat the key as a property of the object in Terraform schema. %>
 <%     next if property.is_a?(Api::Type::Map) && prop.name == property.key_name -%>
 <%=      lines(build_expand_method(prefix + titlelize_property(property), prop), 1) -%>
 <%   end -%>

templates/terraform/flatten_property_method.erb

@@ -16,12 +16,6 @@
 <%= lines(compile_template(property.custom_flatten,
                            prefix: prefix,
                            property: property)) -%>
-<% elsif property.flatten_object -%>
-<%   if property.nested_properties? -%>
-  <%   property.nested_properties.each do |prop| -%>
-    <%=   lines(build_flatten_method(prefix + titlelize_property(property), prop), 1) -%>
-  <%   end -%>
-<%   end -%>
 <% else -%>
 <% if tf_types.include?(property.class) -%>
 func flatten<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d *schema.ResourceData) interface{} {
@@ -35,8 +29,17 @@ func flatten<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d
   }
   transformed := make(map[string]interface{})
   <% property.properties.each do |prop| -%>
+    <% if prop.flatten_object -%>
+    if <%= prop.api_name -%> := flatten<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(original["<%= prop.api_name -%>"], d); <%= prop.api_name -%> != nil {
+      obj := <%= prop.api_name -%>.([]interface{})[0]
+      for k, v := range obj.(map[string]interface{}) {
+        transformed[k] = v
+      }
+    }
+    <% else -%>
     transformed["<%= prop.name.underscore -%>"] =
     flatten<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(original["<%= prop.api_name -%>"], d)
+    <% end -%>
   <% end -%>
   return []interface{}{transformed}
 <% elsif property.is_a?(Api::Type::Array) && property.item_type.is_a?(Api::Type::NestedObject) -%>

templates/terraform/objectlib/base.go.erb

@@ -38,18 +38,10 @@ func Get<%= resource_name -%>ApiObject(d TerraformResourceData, config *Config)
     obj := make(map[string]interface{})
 <%  object.settable_properties.each do |prop| -%>
 <%    if prop.flatten_object -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config)
-    if err != nil {
-        return nil, err
-<%      unless prop.send_empty_value -%>
-    } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) {
-<%      else -%>
-    } else {
-<%      end -%>
-        obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
-    }
-<%    else -%>
+    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(nil, d, config)
+<%    else # if flatten -%>
     <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config)
+<%    end -%>
     if err != nil {
         return nil, err
 <%      unless prop.send_empty_value -%>
@@ -59,7 +51,6 @@ func Get<%= resource_name -%>ApiObject(d TerraformResourceData, config *Config)
 <%      end -%>
         obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
     }
-<%    end -%>
 <%  end -%>
 
 <%  if object.custom_code.encoder -%>

templates/terraform/resource.erb

@@ -110,29 +110,19 @@ func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{
 
     obj := make(map[string]interface{})
 <%  object.settable_properties.each do |prop| -%>
-<%    if prop.flatten_object -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config)
+    <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%>
+    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config)
     if err != nil {
         return err
-<%      unless prop.send_empty_value -%>
+<%    if prop.send_empty_value -%>
+    } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop) {
+<%    elsif prop.flatten_object -%>
     } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) {
-<%      else -%>
-    } else {
-<%      end -%>
-        obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
-    }
 <%    else -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config)
-    if err != nil {
-        return err
-<%      unless prop.send_empty_value -%>
     } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) && (ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop)) {
-<%      else -%>
-    } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop) {
-<%      end -%>
+<%    end -%>
         obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
     }
-<%    end -%>
 <%  end -%>
 
 <%  if object.custom_code.encoder -%>
@@ -262,16 +252,15 @@ func resource<%= resource_name -%>Read(d *schema.ResourceData, meta interface{})
 
 <%  object.gettable_properties.reject{|p| p.ignore_read }.each do |prop| -%>
 <%    if prop.flatten_object -%>
-    if v, ok := res["<%= prop.api_name -%>"].(map[string]interface{}); res["<%= prop.api_name -%>"] != nil && ok {
-<%      prop.properties.each do |p| -%>
-      if err := d.Set("<%= p.name.underscore -%>", flatten<%= resource_name -%><%= titlelize_property(prop) -%><%= titlelize_property(p) -%>(v["<%= p.api_name -%>"], d)); err != nil {
-        return fmt.Errorf("Error reading <%= object.name -%>: %s", err)
-    }
-<%      end -%>
-    } else {
-<%      prop.properties.each do |p| -%>
-      d.Set("<%= p.name.underscore -%>", nil)
-<%      end -%>
+// Terraform must set the top level schema field, but since this object contains collapsed properties
+// it's difficult to know what the top level should be. Instead we just loop over the map returned from flatten.
+    if flattenedProp := flatten<%= resource_name -%><%= titlelize_property(prop) -%>(res["<%= prop.api_name -%>"], d); flattenedProp != nil {
+        casted := flattenedProp.([]interface{})[0]
+        if casted != nil {
+            for k, v := range casted.(map[string]interface{}) {
+                d.Set(k, v)
+            }
+        }
     }
 <%    else -%>
     if err := d.Set("<%= prop.name.underscore -%>", flatten<%= resource_name -%><%= titlelize_property(prop) -%>(res["<%= prop.api_name -%>"], d)); err != nil {
@@ -295,25 +284,13 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{
 <%  if object.input -%>
     d.Partial(true)
 
-<%  properties_by_custom_update(properties).each do |key, props| -%>
-<%  top_level_props = props.reject(&:flatten_object) + props.select(&:flatten_object).flat_map(&:properties) -%>
-    if <%= top_level_props.map { |prop| "d.HasChange(\"#{prop.name.underscore}\")" }.join ' || ' -%> {
+<%  properties_by_custom_update(object.root_properties).each do |key, props| -%>
+if <%= props.map { |prop| "d.HasChange(\"#{prop.name.underscore}\")" }.join ' || ' -%> {
         obj := make(map[string]interface{})
-<%    props.each do |prop| -%>
-<%      if prop.flatten_object -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config)
-    if err != nil {
-        return err
-<%        unless prop.send_empty_value -%>
-    } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) {
-<%        else -%>
-    } else {
-<%        end -%>
-        obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
-    }
-<%      else -%>
-<%        if update_body_properties.include? prop -%>
-        <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config)
+<%      props.each do |prop| -%>
+<%      if update_body_properties.include? prop -%>
+        <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%>
+        <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config)
         if err != nil {
             return err
 <%#         There is some nuance in when we choose to send a value to an update function.
@@ -344,7 +321,6 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{
         <%= prop.api_name -%>Prop := d.Get("<%= prop.name.underscore -%>")
         obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
 <%        end # if update_body_properties.include? prop -%>
-<%      end # prop.flatten_object -%>
 <%    end # props.each -%>
 
 <%      if object.mutex -%>
@@ -410,19 +386,9 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{
 <%  else # if object.input -%>
     obj := make(map[string]interface{})
 <%  update_body_properties.each do |prop| -%>
-<%    if prop.flatten_object -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config)
-    if err != nil {
-        return err
-<%      unless prop.send_empty_value -%>
-    } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) {
-<%      else -%>
-    } else {
-<%      end -%>
-        obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
-    }
-<%    else -%>
-    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config)
+    <%# flattened objects won't have something stored in state so instead nil is passed to the next expander. -%>
+    <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%>
+    <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config)
     if err != nil {
         return err
 <%      unless prop.send_empty_value -%>
@@ -432,7 +398,6 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{
 <%      end -%>
         obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop
     }
-<%    end -%>
 <%  end -%>
 
 <%# We need to decide what encoder to use here - if there's an update encoder, use that! -%>