Add nested_attributes flag for object: mappings (#1193)

* Add nested_attributes flag for object: mappings

Field mappings can now declare nested_attributes: true alongside an
object: target. When the flag is set, Bulkrax routes the imported data
through parsed_metadata['<name>_attributes'] as a numbered-key hash with
'_destroy: false' markers — the shape Reform's nested-attributes
machinery and similar populators consume directly.

Without the flag, callers had to add hardcoded translators (e.g.
ValkyrieObjectFactory#convert_based_near_to_attributes) to rename
parsed_metadata['<name>'] into the form their downstream form expected.
The flag generalizes that rename so each new nested-attribute consumer
declares its needs in the mapping rather than in factory code.

Also makes object_metadata on export tolerant of plain hashes (in
addition to the legacy stringified-hash literal). Resources backed by
JSONB or similar non-string persistence return Ruby hashes directly;
calling eval on them would fail. The legacy stringified path still
works for ActiveFedora-backed resources.

* Add round-trip spec for nested_attributes flag

Confirms that a single field-mapping declaration drives both directions:
imported numbered columns become _attributes-shaped data the form
populator consumes, and the persisted plain-hash array exports back to
the same numbered columns.

* Permit nested *_attributes keys in object factory

When a field-mapping declares nested_attributes: true, the parsed
metadata arrives at the object factory with a *_attributes virtual
key (e.g. redirects_attributes). The slice in #transform_attributes
was dropping these keys because permitted_attributes only listed
bare schema properties, so downstream form populators received
nothing and the data silently disappeared.

permitted_attributes now allows a *_attributes key through whenever
the corresponding bare property is itself permitted. Generic — any
nested-attributes property added in the future works without further
changes here.

* Expand object: properties in CSV template columns

When a property declared on a model (e.g. redirects) is the target of
one or more `object:` field-mappings, the CSV template now emits each
of those mappings' `from:` columns (e.g. redirect_path, redirect_canonical,
redirect_sequence) instead of the bare property name. Adopters get a
template that matches the column shape Bulkrax actually consumes for
nested-attribute imports.

* Tighten suffix validation to require known base

Headers ending in `_<digits>` previously bypassed validation regardless
of the base name, which masked typos like `creater_1` at validation
time even though the real importer would fail to map the column.

A suffixed header now passes validation only when its base name is
itself recognised — either present in valid_headers directly or
resolvable via the mapping manager to a known property — mirroring the
recognition rule already used for unsuffixed headers in
find_unrecognized_validation_headers.

Adds spec coverage for both the bare and numbered forms of an
`object:` mapping with `nested_attributes: true`, and a regression
spec for the typo-with-suffix case the new rule now catches.

* Keep object: columns in CSV template downloads

The CSV builder prunes columns whose data rows are all blank, so per-
child columns from `object:` mappings (e.g. redirect_path) were getting
dropped from the downloaded template even when they appeared in the
column-builder output. The row builder didn't recognise them as
belonging to a known property — `mapped_to_key('redirect_path')`
returned `path`, which isn't in the model's properties list.

ValueDeterminer now consults the mapping's `object:` value: if the
key isn't a property but its object name is, the cell is filled
("Required"/"Optional") based on the parent property. The columns
survive the empty-column pruning and reach the user.

* Memoize known_property_keys in check_headers

The known-property set was being rebuilt on every suffixed header
header_base_recognized? checked. Computing the set once in
check_headers and passing it down avoids the repeated allocations
during validation of CSVs with many numbered columns.

Behavior is unchanged.

Author: laritakr

app/factories/bulkrax/object_factory_interface.rb

@@ -468,7 +468,24 @@ def conditionally_destroy_existing_files
     # Regardless of what the Parser gives us, these are the properties we are
     # prepared to accept.
     def permitted_attributes
-      klass.properties.keys.map(&:to_sym) + base_permitted_attributes
+      bare = klass.properties.keys.map(&:to_sym) + base_permitted_attributes
+      bare + nested_attributes_keys(bare)
+    end
+
+    # Permits `*_attributes` virtual keys when the corresponding bare property
+    # is itself permitted. Field mappings declaring `nested_attributes: true`
+    # (see Bulkrax::HasMatchers#set_parsed_object_data) emit data under keys
+    # like `redirects_attributes`; without this, the slice in
+    # #transform_attributes would drop them and downstream form populators
+    # would receive nothing.
+    def nested_attributes_keys(bare_permitted)
+      bare_set = bare_permitted.map(&:to_sym).to_set
+      attributes.keys.filter_map do |key|
+        key_str = key.to_s
+        next unless key_str.end_with?('_attributes')
+        bare_name = key_str.sub(/_attributes\z/, '').to_sym
+        key.to_sym if bare_set.include?(bare_name)
+      end.uniq
     end
 
     # Return a copy of the given attributes, such that all values that are empty

app/factories/bulkrax/valkyrie_object_factory.rb

@@ -498,14 +498,15 @@ def perform_transaction_for(object:, attrs:)
     #
     # @return [Array<Symbols>]
     def permitted_attributes
-      @permitted_attributes ||= (
-        base_permitted_attributes + if klass.respond_to?(:schema)
-                                      admin_set_id = attributes[:admin_set_id] || attributes['admin_set_id']
-                                      Bulkrax::ValkyrieObjectFactory.schema_properties(klass: klass, admin_set_id: admin_set_id)
-                                    else
-                                      klass.properties.keys.map(&:to_sym)
-                                    end
-      ).uniq
+      @permitted_attributes ||= begin
+        bare = base_permitted_attributes + if klass.respond_to?(:schema)
+                                             admin_set_id = attributes[:admin_set_id] || attributes['admin_set_id']
+                                             Bulkrax::ValkyrieObjectFactory.schema_properties(klass: klass, admin_set_id: admin_set_id)
+                                           else
+                                             klass.properties.keys.map(&:to_sym)
+                                           end
+        (bare + nested_attributes_keys(bare)).uniq
+      end
     end
 
     def update_work(attrs)

app/models/bulkrax/csv_entry.rb

@@ -329,15 +329,14 @@ def prepare_export_data(datum)
     end
 
     def object_metadata(data)
-      # NOTE: What is `d` in this case:
-      #
-      #  "[{\"single_object_first_name\"=>\"Fake\", \"single_object_last_name\"=>\"Fakerson\", \"single_object_position\"=>\"Leader, Jester, Queen\", \"single_object_language\"=>\"english\"}]"
-      #
-      # The above is a stringified version of a Ruby string.  Using eval is a very bad idea as it
-      # will execute the value of `d` within the full Ruby interpreter context.
-      #
-      # TODO: Would it be possible to store this as a non-string?  Maybe the actual Ruby Array and Hash?
-      data = data.map { |d| eval(d) }.flatten # rubocop:disable Security/Eval
+      # Each `d` may be either a stringified Ruby hash literal (legacy
+      # ActiveFedora persistence) or a plain Hash (Valkyrie/Postgres
+      # JSONB). For the legacy stringified form we eval to recover the
+      # hash; for plain Hashes we pass through. Using eval is a very bad
+      # idea as it will execute the value of `d` within the full Ruby
+      # interpreter context — only do it when we know the input is a
+      # stringified hash.
+      data = data.map { |d| d.is_a?(Hash) ? d : eval(d) }.flatten # rubocop:disable Security/Eval
 
       data.each_with_index do |obj, index|
         next if obj.nil?

app/models/concerns/bulkrax/has_matchers.rb

@@ -38,7 +38,7 @@ def add_metadata(node_name, node_content, index = nil)
 
         if object_name
           Rails.logger.info("Bulkrax Column automatically matched object #{node_name}, #{node_content}")
-          parsed_metadata[object_name] ||= object_multiple ? [{}] : {}
+          init_object_container(object_name, object_multiple)
         end
 
         value = if matcher
@@ -60,6 +60,30 @@ def get_object_name(field)
       mapping&.[](field)&.[]('object')
     end
 
+    # When any field-mapping sibling under `object_name` carries
+    # `nested_attributes: true`, Bulkrax routes the imported data through
+    # `parsed_metadata["#{object_name}_attributes"]` as a numbered-key hash
+    # (with `_destroy: 'false'` per row) — the shape consumed by Reform's
+    # nested-attributes machinery and other `*_attributes`-style populators.
+    def nested_attributes_object?(object_name)
+      return false unless mapping.is_a?(Hash) && object_name.present?
+      mapping.any? { |_, cfg| cfg.is_a?(Hash) && cfg['object'] == object_name && cfg['nested_attributes'] }
+    end
+
+    def parsed_object_target_key(object_name)
+      nested_attributes_object?(object_name) ? "#{object_name}_attributes" : object_name
+    end
+
+    def init_object_container(object_name, object_multiple)
+      target_key = parsed_object_target_key(object_name)
+      default = if object_multiple
+                  nested_attributes_object?(object_name) ? {} : [{}]
+                else
+                  {}
+                end
+      parsed_metadata[target_key] ||= default
+    end
+
     def set_parsed_data(name, value)
       return parsed_metadata[name] = value unless multiple?(name)
 
@@ -69,22 +93,33 @@ def set_parsed_data(name, value)
     end
 
     def set_parsed_object_data(object_multiple, object_name, name, index, value)
-      if object_multiple
-        index ||= 0
-        parsed_metadata[object_name][index] ||= {}
-        parsed_metadata[object_name][index][name] ||= []
-        if value.is_a?(Array)
-          parsed_metadata[object_name][index][name] += value
-        else
-          parsed_metadata[object_name][index][name] = value
-        end
+      target_key = parsed_object_target_key(object_name)
+      target = object_target_for(target_key, object_name, object_multiple, index)
+      assign_object_value(target, name, value)
+    end
+
+    # Resolve the hash slot that `name` should be written into, initializing
+    # any intermediate containers. Returns the leaf hash so the caller can
+    # assign the value with a single statement.
+    def object_target_for(target_key, object_name, object_multiple, index)
+      return parsed_metadata[target_key] unless object_multiple
+
+      idx = index || 0
+      if nested_attributes_object?(object_name)
+        parsed_metadata[target_key][idx.to_s] ||= { '_destroy' => 'false' }
+        parsed_metadata[target_key][idx.to_s]
       else
-        parsed_metadata[object_name][name] ||= []
-        if value.is_a?(Array)
-          parsed_metadata[object_name][name] += value
-        else
-          parsed_metadata[object_name][name] = value
-        end
+        parsed_metadata[target_key][idx] ||= {}
+        parsed_metadata[target_key][idx]
+      end
+    end
+
+    def assign_object_value(target, name, value)
+      target[name] ||= []
+      if value.is_a?(Array)
+        target[name] += value
+      else
+        target[name] = value
       end
     end
 

app/parsers/concerns/bulkrax/csv_parser/csv_validation.rb

@@ -84,7 +84,15 @@ def check_headers(headers, raw_csv, mapping_manager, mappings, field_metadata, f
           all_models    = field_metadata.keys
           valid_headers = build_valid_validation_headers(mapping_manager, field_analyzer,
                                                          all_models, mappings, field_metadata)
-          suffixed      = headers.select { |h| h.match?(/_\d+\z/) }
+          # Only allow a suffixed header (e.g. `creator_1`, `redirect_path_2`)
+          # when its base name is itself recognised. The blanket allow that
+          # used to live here let through any *_<digits> column, which masked
+          # typos in numbered columns at validation time even though the real
+          # importer would fail to map them.
+          known_property_keys = (field_metadata || {}).values.flat_map { |m| Array(m[:properties]) }.to_set
+          suffixed = headers.select do |h|
+            h.match?(/_\d+\z/) && header_base_recognized?(h, valid_headers, mapping_manager, known_property_keys)
+          end
           valid_headers = (valid_headers + suffixed).uniq
 
           {
@@ -96,6 +104,20 @@ def check_headers(headers, raw_csv, mapping_manager, mappings, field_metadata, f
           }
         end
 
+        # Mirrors the recognition rule used by
+        # find_unrecognized_validation_headers: a header's base name is
+        # recognised if it appears in valid_headers directly or if its
+        # mapping_manager#mapped_to_key resolves to a known model property.
+        # known_property_keys is precomputed by check_headers so this can be
+        # called per-header without rebuilding the set each time.
+        def header_base_recognized?(header, valid_headers, mapping_manager, known_property_keys)
+          base = header.sub(/_\d+\z/, '')
+          return true if valid_headers.include?(base)
+
+          mapped_key = mapping_manager&.mapped_to_key(base)
+          mapped_key.present? && known_property_keys.include?(mapped_key)
+        end
+
         def extract_hierarchy_items(csv_data, all_ids, find_record, mappings)
           extract_validation_items(
             csv_data, all_ids, find_record,

app/services/bulkrax/csv_template/column_builder.rb

@@ -35,12 +35,22 @@ def property_columns
         properties = field_lists
                      .flat_map { |item| item.values.flat_map { |config| config["properties"] || [] } }
                      .uniq
-                     .map { |property| @service.mapping_manager.key_to_mapped_column(property) }
+                     .flat_map { |property| columns_for_property(property) }
                      .uniq
 
         (properties - required_columns).sort
       end
 
+      # When a property is the target of one or more `object:` field mappings,
+      # emit each of those mappings' `from:` columns (e.g. redirect_path,
+      # redirect_canonical, redirect_sequence) rather than the bare property
+      # name (redirects). Otherwise fall back to the standard 1:1 mapping.
+      def columns_for_property(property)
+        nested = @service.mapping_manager.object_columns_for(property)
+        return nested if nested.any?
+        [@service.mapping_manager.key_to_mapped_column(property)]
+      end
+
       def relationship_columns
         [
           @service.mapping_manager.find_by_flag("related_children_field_mapping", 'children'),

app/services/bulkrax/csv_template/mapping_manager.rb

@@ -26,6 +26,28 @@ def key_to_mapped_column(key)
         @mappings.dig(key, "from")&.first || key
       end
 
+      # Returns the `object:` value for a given mapping key, or nil. Mirrors
+      # the importer-side `Bulkrax::HasMatchers#get_object_name` for callers
+      # working with the template-side mapping manager.
+      def get_object_name(key)
+        @mappings.dig(key, "object")
+      end
+
+      # Returns the column names that target a given object name via the
+      # `object:` field-mapping pattern. The template generator uses this to
+      # emit the per-child columns (e.g. redirect_path, redirect_canonical,
+      # redirect_sequence) instead of the bare property name (redirects).
+      # Numbering is intentionally omitted — the template shows the column
+      # shape once; CSV rows can repeat the column with numeric suffixes
+      # (e.g. redirect_path_1, redirect_path_2) at import time.
+      def object_columns_for(object_name)
+        @mappings
+          .select { |_k, v| v.is_a?(Hash) && v["object"] == object_name }
+          .values
+          .flat_map { |v| Array(v["from"]) }
+          .uniq
+      end
+
       def find_by_flag(field_name, default)
         @mappings.find { |_k, v| v[field_name] == true }&.first || default
       end

app/services/bulkrax/csv_template/value_determiner.rb

@@ -12,9 +12,16 @@ def initialize(service)
       def determine_value(column, model_name, field_list)
         key = @service.mapping_manager.mapped_to_key(column)
         required_terms = field_list.dig(model_name, 'required_terms')
+        properties = field_list.dig(model_name, "properties") || []
+        object_name = @service.mapping_manager.get_object_name(key)
 
-        if field_list.dig(model_name, "properties")&.include?(key)
+        if properties.include?(key)
           mark_required_or_optional(key, required_terms)
+        elsif object_name && properties.include?(object_name)
+          # Column belongs to an `object:` mapping (e.g. `redirect_path` → object
+          # `redirects`). Treat the column as required/optional based on the
+          # parent property's required-terms list.
+          mark_required_or_optional(object_name, required_terms)
         elsif special_column?(column, key)
           special_value(column, key, model_name, required_terms)
         end