From ff9ad7a8f1936e8715078c0e8ab374e05db4ddc1 Mon Sep 17 00:00:00 2001 From: LaRita Robinson Date: Sun, 3 May 2026 17:19:07 -0400 Subject: [PATCH 1/4] Make BasedNearFieldBehavior compose via super A refactor to make BasedNearFieldBehavior more composable with other Field Behaviors, and to fix a bug in validates_with. Two related form-layer fixes: 1. BasedNearFieldBehavior used to handle its own deserialize step end-to-end. That made it impossible for a second Field Behavior on the same form to take part in deserialization. The module now lets Reform handle the standard rename pass first, removes its own key, and passes control along. Multiple Field Behaviors can now coexist on one form without conflict. 2. ResourceForm declared its redirects validator with `validates_with` and a list of attributes. Reform and ActiveModel together end up sharing one options hash across every form subclass at load time. ActiveModel empties that hash the first time through, so the second subclass to load crashed with "attributes cannot be blank." Wiring the same validator through a `validation { ... }` block instead gives each subclass its own copy of the options. --- .../hyrax/based_near_field_behavior.rb | 44 +++++++++++++++---- app/forms/hyrax/forms/resource_form.rb | 18 +++++++- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/app/forms/concerns/hyrax/based_near_field_behavior.rb b/app/forms/concerns/hyrax/based_near_field_behavior.rb index ac01711ada..d37f2d91c2 100644 --- a/app/forms/concerns/hyrax/based_near_field_behavior.rb +++ b/app/forms/concerns/hyrax/based_near_field_behavior.rb @@ -1,18 +1,46 @@ # frozen_string_literal: true module Hyrax + # Form-side handling for the `based_near` (location) controlled vocabulary + # field. The form expects a `ControlledVocabularies::Location` object as + # input and produces a hash like those used with `accepts_nested_attributes_for`. + # + # The `deserialize!` override below is the canonical Field Behavior pattern + # for nested-attribute properties: a module prepended onto every + # `ResourceForm` subclass that strips its own key from the rewritten params + # so Reform's `from_hash` doesn't write the raw `_attributes` payload to a + # same-named property. Calling `super` first lets the chain compose — every + # Field Behavior runs its own delete, then control falls through to Reform's + # base `deserialize!` (the `_attributes` rename pass). module BasedNearFieldBehavior - # Provides compatibility with the behavior of the based_near (location) controlled vocabulary form field. - # The form expects a ControlledVocabularies::Location object as input and produces a hash like those - # used with accepts_nested_attributes_for. def self.included(descendant) descendant.property :based_near_attributes, virtual: true, populator: :based_near_attributes_populator, prepopulator: :based_near_attributes_prepopulator end - # there is a race condition during validation that leaves the based_near field in an inconsistent state. - # we skip the unedited based_near for validation and only handle it during attribute population - def deserialize(params) - params = deserialize!(params) - deserializer.new(self).from_hash(params.except('based_near')) + # Skipping based_near in deserialize avoids a race condition where it + # would otherwise end up in an inconsistent state during validation; the + # field is handled exclusively by the populator on + # `based_near_attributes`. + # + # Override `deserialize!` (not `deserialize`) so the strip runs *after* + # Reform's `FormBuilderMethods#deserialize!` has renamed + # `based_near_attributes` to `based_near`, but *before* `from_hash` + # reads property values from the params. Stripping in `deserialize` + # would happen before the rename, and the rename would put the key + # back; `from_hash` would then write raw fragment hashes onto the + # form's `based_near` field, breaking the populator's contract. + # + # Mutate `params` in place — never replace it. Reform's `validate(params)` + # exposes the same hash via `form.input_params`, and downstream callers + # (`WorksControllerBehavior#update_valkyrie_work` reading + # `form.input_params["permissions"]`) read from that exact reference + # *after* the rename. + def deserialize!(params) + result = super + if result.respond_to?(:delete) + result.delete('based_near') + result.delete(:based_near) + end + result end private diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb index f77b5b89be..07d06bd621 100644 --- a/app/forms/hyrax/forms/resource_form.rb +++ b/app/forms/hyrax/forms/resource_form.rb @@ -43,7 +43,23 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength # @see https://github.com/samvera/valkyrie/wiki/Optimistic-Locking property :version, virtual: true, prepopulator: LockKeyPrepopulator - validates_with Hyrax::RedirectValidator, attributes: [:redirects] if Hyrax.config.redirects_enabled? + # Wire validators with `attributes:` through `validation { ... }` rather + # than the bare `validates_with`. Reform's `validates_with` shim closes + # over the args array, so the options hash (`{attributes: [:foo]}`) is + # shared across heritage replays. ActiveModel mutates that hash on each + # call (`options[:class] = self`, then `options.delete(:attributes)` + # inside `EachValidator#initialize`). The first replay leaves the hash + # without `:attributes`; the second replay raises + # `:attributes cannot be blank` and the subclass crashes at load time. + # Wrapping in `validation(name: :default, inherit: true) { ... }` rebuilds + # the literal options hash on every replay so each subclass gets its own + # clean copy. This pattern applies to *any* `validates_with` that takes + # an `attributes:` keyword. + if Hyrax.config.redirects_enabled? + validation(name: :default, inherit: true) do + validates_with Hyrax::RedirectValidator, attributes: [:redirects] + end + end ## # @api public From adc28d11508061204cd42fd44a76e7ab26f9e92b Mon Sep 17 00:00:00 2001 From: LaRita Robinson Date: Mon, 4 May 2026 17:04:21 -0400 Subject: [PATCH 2/4] Persist redirects as plain hashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stores redirects as plain hashes on the parent work or collection, mirroring how based_near stores plain URI strings. The previous shape (a set of nested resources) round-tripped badly through Postgres — sub-fields were stripped on save and unrelated parent fields leaked into each entry, leaving the work unloadable. Hyrax::Redirect is kept as a thin presenter the form view uses; other code reads the hash directly. Hyrax.config.redirects_active? becomes the single source of truth for "is the feature in use right now?", combining the config and Flipflop gates. The catch-all redirect route moves to the host application so it no longer intercepts engine routes like /concern/... Co-Authored-By: Claude Opus 4.7 (1M context) --- .dassie/config/routes.rb | 5 + .koppie/config/routes.rb | 5 + app/forms/hyrax/forms/resource_form.rb | 20 ---- .../hyrax/indexers/redirects_indexer.rb | 15 +-- app/models/concerns/hyrax/flexibility.rb | 2 +- app/models/hyrax/redirect.rb | 73 +++++++++--- .../redirects_validator.rb | 57 ++++++++-- app/services/hyrax/schema_loader.rb | 81 ++++--------- app/validators/hyrax/redirect_validator.rb | 56 ++++++--- config/metadata/redirects.yaml | 2 +- config/routes.rb | 5 - documentation/redirects.md | 40 +++++-- lib/generators/hyrax/install_generator.rb | 15 ++- lib/hyrax/configuration.rb | 9 ++ .../shared_specs/factories/hyrax_redirect.rb | 11 -- .../transactions/steps/sync_redirect_paths.rb | 5 +- .../hyrax/redirects_controller_spec.rb | 2 +- .../hyrax/indexers/redirects_indexer_spec.rb | 24 ++-- .../steps/sync_redirect_paths_spec.rb | 21 +--- spec/models/hyrax/redirect_spec.rb | 73 ++++++++++-- .../redirects_validator_spec.rb | 63 +++++++++-- spec/services/hyrax/schema_loader_spec.rb | 67 ++++------- .../hyrax/redirect_validator_spec.rb | 107 +++++++++++++----- 23 files changed, 477 insertions(+), 281 deletions(-) delete mode 100644 lib/hyrax/specs/shared_specs/factories/hyrax_redirect.rb diff --git a/.dassie/config/routes.rb b/.dassie/config/routes.rb index 5607b8d2f1..0d0b10ac0f 100644 --- a/.dassie/config/routes.rb +++ b/.dassie/config/routes.rb @@ -36,4 +36,9 @@ end end # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html + + # Catch-all redirect resolver — must be last. See documentation/redirects.md. + get '*alias_path', to: 'hyrax/redirects#show', + constraints: ->(_req) { Hyrax.config.redirects_active? }, + format: false end diff --git a/.koppie/config/routes.rb b/.koppie/config/routes.rb index 93cab6b8a4..a4fc824bd7 100644 --- a/.koppie/config/routes.rb +++ b/.koppie/config/routes.rb @@ -36,4 +36,9 @@ end end # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html + + # Catch-all redirect resolver — must be last. See documentation/redirects.md. + get '*alias_path', to: 'hyrax/redirects#show', + constraints: ->(_req) { Hyrax.config.redirects_active? }, + format: false end diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb index 07d06bd621..5e243707aa 100644 --- a/app/forms/hyrax/forms/resource_form.rb +++ b/app/forms/hyrax/forms/resource_form.rb @@ -205,14 +205,6 @@ def []=(attr, value) public_send("#{attr}=".to_sym, value) end - # Normalize redirect paths on assignment so the form, the persisted - # resource, the uniqueness ledger, and the resolver all agree on the - # canonical shape. See Hyrax::RedirectPathNormalizer. - def redirects=(values) - return super unless Hyrax.config.redirects_enabled? - super(Array(values).map { |entry| normalize_redirect_entry(entry) }) - end - ## # @deprecated use model.class instead # @@ -258,18 +250,6 @@ def _form_field_definitions self.class.definitions end end - - def normalize_redirect_entry(entry) - case entry - when Hash - entry = entry.transform_keys(&:to_sym) - entry.merge(path: Hyrax::RedirectPathNormalizer.call(entry[:path])) - when Hyrax::Redirect - Hyrax::Redirect.new(entry.attributes.merge(path: Hyrax::RedirectPathNormalizer.call(entry.path))) - else - entry - end - end end end end diff --git a/app/indexers/hyrax/indexers/redirects_indexer.rb b/app/indexers/hyrax/indexers/redirects_indexer.rb index ac0a6224ea..d4191a8552 100644 --- a/app/indexers/hyrax/indexers/redirects_indexer.rb +++ b/app/indexers/hyrax/indexers/redirects_indexer.rb @@ -7,22 +7,19 @@ module Indexers # resources that carry a `redirects` attribute. The redirect resolver # (`Hyrax::RedirectsController`) queries this field by path. # - # Include this mixin only in apps where `Hyrax.config.redirects_enabled?` - # is true (the inclusion site is the config gate). The mixin's body - # then needs only the Flipflop check — when the config is on, the - # `:redirects` feature is registered and `Flipflop.redirects?` is safe - # to call. - # # @example # class WorkIndexer < Hyrax::Indexers::PcdmObjectIndexer - # include Hyrax::Indexers::RedirectsIndexer if Hyrax.config.redirects_enabled? + # include Hyrax::Indexers::RedirectsIndexer # end module RedirectsIndexer def to_solr(*args) super.tap do |document| - next document unless Flipflop.redirects? + next document unless Hyrax.config.redirects_active? next document unless resource.respond_to?(:redirects) - document['redirects_path_ssim'] = Array(resource.redirects).map(&:path).compact + # Valkyrie's JSONValueMapper symbolizes hash keys on read; accept either. + document['redirects_path_ssim'] = Array(resource.redirects) + .map { |entry| Hyrax::RedirectPathNormalizer.call(entry['path'] || entry[:path]) } + .reject(&:blank?) end end end diff --git a/app/models/concerns/hyrax/flexibility.rb b/app/models/concerns/hyrax/flexibility.rb index a7a1a92cb5..579faea8a5 100644 --- a/app/models/concerns/hyrax/flexibility.rb +++ b/app/models/concerns/hyrax/flexibility.rb @@ -67,7 +67,7 @@ def new(attributes = default_attributes, safe = false, &block) # rubocop:disable load(attributes, safe) end rescue Dry::Types::CoercionError => e - raise Dry::Error, "[#{self}.new] #{e}", e.backtrace + raise RuntimeError, "[#{self}.new] #{e.class}: #{e.message}", e.backtrace end ## Read the schema from the database and load the correct schemas for the instance in to the class diff --git a/app/models/hyrax/redirect.rb b/app/models/hyrax/redirect.rb index c4fdef8379..2df8d33637 100644 --- a/app/models/hyrax/redirect.rb +++ b/app/models/hyrax/redirect.rb @@ -2,23 +2,66 @@ module Hyrax ## - # The Valkyrie model for a single URL redirect entry on a work or collection. + # Wraps a single redirect entry for use in form views, providing + # `.path`, `.canonical`, and `.sequence` readers over the underlying + # persisted hash. # - # A work or collection has zero or more `redirects`. Each entry holds a path - # (the lookup key for the redirect resolver), a `canonical` flag (at most one - # entry per record may be true), and an optional `sequence` for display - # ordering in the admin UI. + # Redirects are persisted as plain hashes on the parent work or + # collection. Form-render code calls `Hyrax::Redirect.wrap(entry)` to + # get a value object the view can call methods on. Other code (the + # validator, indexer, sync step) reads the persisted hash directly. # # @example - # work.redirects = [ - # Hyrax::Redirect.new(path: '/handle/12345/678', canonical: false), - # Hyrax::Redirect.new(path: '/robs-cat-study', canonical: true, sequence: 0) - # ] - # - # @see Hyrax::RedirectsController for the resolution path - class Redirect < Valkyrie::Resource - attribute :path, Valkyrie::Types::String - attribute :canonical, Valkyrie::Types::Bool.default(false) - attribute :sequence, Valkyrie::Types::Integer.optional + # Hyrax::Redirect.new(path: '/handle/12345/678', canonical: false) + # Hyrax::Redirect.wrap('path' => '/foo', 'canonical' => true) + class Redirect + attr_reader :path, :canonical, :sequence + + ## + # Accept nil values so the view can build an empty trailing row. + def initialize(path: nil, canonical: false, sequence: nil) + @path = path + @canonical = canonical + @sequence = sequence + end + + ## + # Build a presenter from a hash. If passed a presenter, returns it + # unchanged. Returns nil for nil input. Accepts string or symbol keys. + # + # @param input [Hyrax::Redirect, Hash, nil] + # @return [Hyrax::Redirect, nil] + def self.wrap(input) + return nil if input.nil? + return input if input.is_a?(Hyrax::Redirect) + raise ArgumentError, "cannot wrap #{input.class} as Hyrax::Redirect" unless input.respond_to?(:to_h) + + h = input.to_h.transform_keys(&:to_s) + new(path: h['path'], canonical: h.fetch('canonical', false), sequence: h['sequence']) + end + + ## + # @return [Hash{String => Object}] string-keyed hash matching the persisted shape. + def to_h + { 'path' => path, 'canonical' => canonical, 'sequence' => sequence } + end + + def as_json(*) + to_h + end + + ## + # Equality on attribute values, so `Array#uniq` works as expected. + def ==(other) + other.is_a?(Hyrax::Redirect) && + other.path == path && + other.canonical == canonical && + other.sequence == sequence + end + alias eql? == + + def hash + [path, canonical, sequence].hash + end end end diff --git a/app/services/hyrax/flexible_schema_validators/redirects_validator.rb b/app/services/hyrax/flexible_schema_validators/redirects_validator.rb index 45ec61f632..a608a83c0b 100644 --- a/app/services/hyrax/flexible_schema_validators/redirects_validator.rb +++ b/app/services/hyrax/flexible_schema_validators/redirects_validator.rb @@ -15,11 +15,10 @@ module FlexibleSchemaValidators # | on | off | present | warn (property is loaded but unused)| # | on | off | absent | silent | # | on | on | absent | error (property is required) | - # | on | on | present | check available_on.class and pass | - # | | | | or error if work/collection missing | + # | on | on | present | check available_on.class lists at | + # | | | | least one work or collection class | + # | | | | declared in this profile's classes | class RedirectsValidator - REQUIRED_CLASSES = %w[Hyrax::Work Hyrax::PcdmCollection].freeze - ## # @param profile [Hash] the flexible metadata profile # @param errors [Array] an array to append errors to @@ -64,11 +63,53 @@ def validate_when_enabled return end - available_on = Array(redirects_property.dig('available_on', 'class')) - missing = REQUIRED_CLASSES - available_on - return if missing.empty? + @errors << "m3 profile `redirects` property must declare `type: hash` (got `#{redirects_property['type'].inspect}`)" unless redirects_property['type'].to_s == 'hash' + + available_on = clean(Array(redirects_property.dig('available_on', 'class'))) + return if (available_on & profile_work_or_collection_classes).any? + + @errors << 'm3 profile `redirects` property must be available on at least one work or collection class declared in this profile' + end + + # Class names declared in this m3 profile's top-level `classes:` block, + # filtered to keep only those that represent works or collections. + # FileSets, AdminSets, and any non-work/non-collection class are + # excluded — redirects only apply to works and collections. + def profile_work_or_collection_classes + @profile_work_or_collection_classes ||= begin + declared = clean(Array(@profile&.dig('classes')&.keys)) + declared.select { |name| work_or_collection?(name) } + end + end + + def work_or_collection?(class_name) + registered_collection_names.include?(class_name) || + registered_work_names.include?(class_name) + end + + def registered_collection_names + @registered_collection_names ||= clean(Hyrax::ModelRegistry.collection_class_names) + end + + # Work class names from the registry, with collection / file_set / + # admin_set names explicitly excluded. The registry can include any + # class an adopter has registered as a curation concern, so the + # exclusion guards against e.g. a FileSet being registered alongside + # works and slipping through as a "work" type for redirects. + # Each registered name is also paired with its `Resource`-suffixed + # Valkyrie equivalent — `class_validator` accepts both forms. + def registered_work_names + @registered_work_names ||= begin + works = clean(Hyrax::ModelRegistry.work_class_names) + works -= clean(Hyrax::ModelRegistry.collection_class_names) + works -= clean(Hyrax::ModelRegistry.file_set_class_names) + works -= clean(Hyrax::ModelRegistry.admin_set_class_names) + works.flat_map { |name| [name, "#{name}Resource"] } + end + end - @errors << "m3 profile `redirects` property must be available on: #{missing.join(', ')}" + def clean(names) + names.map { |name| name.to_s.delete_prefix('::') } end def warn_dead_property(reason) diff --git a/app/services/hyrax/schema_loader.rb b/app/services/hyrax/schema_loader.rb index ce748831c1..b6696eedd3 100644 --- a/app/services/hyrax/schema_loader.rb +++ b/app/services/hyrax/schema_loader.rb @@ -106,34 +106,20 @@ def admin_only? # @return [Dry::Types::Type] def type member_type = type_for(config['type']) - nested_resource = member_type.is_a?(Class) && member_type < Valkyrie::Resource - - raise ArgumentError, "nested resource members require `multiple: true` (got #{member_type})" if nested_resource && !multiple? - - collection_type = if multiple? - # When the entries are nested Hyrax resources (e.g. Hyrax::Redirect), - # use Set so reading-and-writing the same value back works. - # Array of resources would crash on `record.foo = record.foo` - # because it tries to rebuild each entry from a hash. - if nested_resource - Valkyrie::Types::Set.constructor(&Coerce) - else - Valkyrie::Types::Array.constructor(&Coerce) - end - else - Identity - end - - collection_type.of(member_type) + wrapper_type = multiple? ? Valkyrie::Types::Array.constructor(&Coerce) : Identity + wrapper_type.of(member_type) end # Cleans up the input before the type system sees it: drops the # "no value provided" placeholder dry-types uses internally, then - # removes blanks. Without dropping the placeholder, it leaks into - # member coercion and breaks nested-resource attributes. + # removes blanks. Wraps a bare Hash in a one-element array; using + # `Array(hash)` would surprise-flatten it into [[:k, v], ...] pairs. + # Valkyrie's JSONValueMapper unwraps single-element arrays on read, + # so the type sees a hash here when there was originally one entry. Coerce = lambda do |value| return [] if value.equal?(Dry::Types::Undefined) - Array(value).reject { |v| v.equal?(Dry::Types::Undefined) }.select(&:present?) + wrapped = value.is_a?(::Hash) ? [value] : Array(value) + wrapped.reject { |v| v.equal?(Dry::Types::Undefined) }.select(&:present?) end # Determine whether this attribute allows multiple values. @@ -149,13 +135,13 @@ def multiple? ## # @api private # - # This class acts as a Valkyrie/Dry::Types collection with typed members, - # but instead of wrapping the given type with itself as the collection type - # (as in `Valkyrie::Types::Array.of(MyType)`), it returns the given type. + # No-op wrapper for single-value attributes. Lets `#type` build its + # output the same way regardless of whether the attribute holds one + # value or many: `wrapper.of(member_type)` either wraps it (Array) + # or hands it back unchanged (Identity). # # @example # Identity.of(Valkyrie::Types::String) # => Valkyrie::Types::String - # class Identity ## # @param [Dry::Types::Type] @@ -168,19 +154,21 @@ def self.of(type) private ## - # Resolves a `type:` value from a schema YAML to the actual class to use. + # Resolves a `type:` value from a schema YAML to a Dry::Types::Type. # - # Looks for the type in this order: - # 1. The shortcuts `id`, `uri`, and `date_time`. - # 2. A primitive type under `Valkyrie::Types::*` (e.g. `string` → `Valkyrie::Types::String`). - # 3. A `Valkyrie::Resource` class. Short names are looked up under `Hyrax::*` - # (so `type: redirect` finds `Hyrax::Redirect`); fully-qualified names like - # `MyApp::Citation` are looked up as-is. + # Recognized values: + # - `id`, `uri`, `date_time` — Valkyrie type shortcuts. + # - `hash` — for attributes whose entries carry multiple sub-fields + # (e.g. redirects, with path / canonical / sequence). Use this + # instead of nesting a Valkyrie::Resource. See + # `documentation/redirects.md` for a worked example. + # - Any primitive Valkyrie type, looked up under `Valkyrie::Types::*` + # by classified name (e.g. `string` → `Valkyrie::Types::String`). # # Raises `ArgumentError` if nothing matches. # # @param [String] - # @return [Dry::Types::Type, Class] + # @return [Dry::Types::Type] def type_for(type) case type when 'id' @@ -189,34 +177,13 @@ def type_for(type) Valkyrie::Types::URI when 'date_time' Valkyrie::Types::DateTime + when 'hash' + Dry::Types['hash'] else "Valkyrie::Types::#{type.classify}".safe_constantize || - nested_resource_type(type) || raise(ArgumentError, "Unrecognized type: #{type}") end end - - ## - # Looks up a `Valkyrie::Resource` class by name. Returns nil if the - # name doesn't resolve to one. - # - # A short name like `redirect` is checked under `Hyrax::*` first - # (`Hyrax::Redirect`), then at the top level (`Redirect`). A name - # with `::` in it is taken as-is (`MyApp::Citation`). Anything that - # resolves to something other than a `Valkyrie::Resource` class is - # rejected, so non-resource classes don't accidentally get used as - # nested-attribute types. - # - # @param [String] type - # @return [Class, nil] a Valkyrie::Resource subclass, or nil if no match - def nested_resource_type(type) - candidates = type.include?('::') ? [type] : ["Hyrax::#{type.classify}", type.classify] - candidates.each do |name| - klass = name.safe_constantize - return klass if klass.is_a?(Class) && klass < Valkyrie::Resource - end - nil - end end class UndefinedSchemaError < ArgumentError; end diff --git a/app/validators/hyrax/redirect_validator.rb b/app/validators/hyrax/redirect_validator.rb index 23e9a8c365..1f8ce29205 100644 --- a/app/validators/hyrax/redirect_validator.rb +++ b/app/validators/hyrax/redirect_validator.rb @@ -4,11 +4,6 @@ module Hyrax # Validates the `redirects` attribute on a work or collection. Runs only # when the redirects feature is fully enabled (config + Flipflop). # - # Path normalization happens upstream of this validator (in - # `Hyrax::Forms::ResourceForm#redirects=`), so the entries this - # validator sees already match the canonical form stored in - # `hyrax_redirect_paths` and queried by the resolver. - # # See documentation/redirects.md for the validation rules. class RedirectValidator < ActiveModel::EachValidator PATH_FORMAT = %r{\A/[^?\s#]+\z}.freeze @@ -23,7 +18,7 @@ class RedirectValidator < ActiveModel::EachValidator # 2. record_supports_redirects? — the structural gate. Catches the case # where both feature gates are on but the property wasn't added def validate(record) - return unless Hyrax.config.redirects_enabled? && Flipflop.redirects? + return unless Hyrax.config.redirects_active? return unless record_supports_redirects?(record) super end @@ -39,39 +34,70 @@ def validate_each(record, attribute, entries) private + # Entries are persisted hashes, but the form-render path may pass + # Hyrax::Redirect presenter instances. Both shapes are acceptable. + def path_for(entry) + return entry.path if entry.respond_to?(:path) + return entry['path'] || entry[:path] if entry.respond_to?(:[]) + nil + end + + def canonical_for(entry) + return entry.canonical if entry.respond_to?(:canonical) + return entry.key?('canonical') ? entry['canonical'] : entry[:canonical] if entry.respond_to?(:[]) + nil + end + def validate_each_entry(record, attribute, entries) entries.each do |entry| - path = entry.try(:path) + path = path_for(entry) if path.blank? record.errors.add(attribute, message_for(:blank)) elsif !PATH_FORMAT.match?(path) record.errors.add(attribute, message_for(:invalid_format, path: path)) - elsif Hyrax.config.reserved_redirect_prefixes.any? { |prefix| path == prefix || path.start_with?("#{prefix}/") } + elsif reserved_prefix?(path) record.errors.add(attribute, message_for(:reserved_prefix, path: path)) end end end + def reserved_prefix?(path) + normalized = normalized_path(path) + Hyrax.config.reserved_redirect_prefixes.any? do |prefix| + normalized == prefix || normalized.start_with?("#{prefix}/") + end + end + def validate_intra_record_uniqueness(record, attribute, entries) - paths = entries.map { |entry| entry.try(:path) }.compact - duplicates = paths.tally.select { |_, count| count > 1 }.keys - duplicates.each do |path| - record.errors.add(attribute, message_for(:intra_record_duplicate, path: path)) + grouped = entries.each_with_object({}) do |entry, acc| + path = path_for(entry) + next if path.blank? + canonical = normalized_path(path) + next if canonical.blank? + (acc[canonical] ||= []) << path + end + grouped.each_value do |paths| + next unless paths.size > 1 + record.errors.add(attribute, message_for(:intra_record_duplicate, path: paths.first)) end end def validate_global_uniqueness(record, attribute, entries) except_id = record.try(:id) entries.each do |entry| - path = entry.try(:path) + path = path_for(entry) next if path.blank? - next unless Hyrax::RedirectsLookup.taken?(path, except_id: except_id) + next unless Hyrax::RedirectsLookup.taken?(normalized_path(path), except_id: except_id) record.errors.add(attribute, message_for(:already_taken, path: path)) end end + def normalized_path(path) + Hyrax::RedirectPathNormalizer.call(path) + end + def validate_at_most_one_canonical(record, attribute, entries) - canonical_count = entries.count { |entry| entry.try(:canonical) } + canonical_count = entries.count { |entry| canonical_for(entry) } return if canonical_count <= 1 record.errors.add(attribute, message_for(:multiple_canonical)) end diff --git a/config/metadata/redirects.yaml b/config/metadata/redirects.yaml index 2b55cfb202..a62c19bcd6 100644 --- a/config/metadata/redirects.yaml +++ b/config/metadata/redirects.yaml @@ -1,7 +1,7 @@ --- attributes: redirects: - type: redirect + type: hash multiple: true form: primary: false diff --git a/config/routes.rb b/config/routes.rb index 9ee1ab4fa6..b8280798dd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -279,9 +279,4 @@ %w[zotero mendeley].each do |action| get action, controller: 'static', action: action, as: action end - - # Catch-all redirect resolver — must be last. See documentation/redirects.md. - get '*alias_path', to: 'redirects#show', - constraints: ->(_req) { Hyrax.config.redirects_enabled? && Flipflop.redirects? }, - format: false end diff --git a/documentation/redirects.md b/documentation/redirects.md index 21fe3894e7..3638361165 100644 --- a/documentation/redirects.md +++ b/documentation/redirects.md @@ -16,7 +16,7 @@ The redirects feature is gated by **two** independent switches: |---|---|---| | off | n/a (unregistered) | The schema is not loaded. The Flipflop feature is not registered. No `redirects` attribute on any resource. No indexer. No route. No controller. m3 profile does not require a `redirects` property. The feature is wholly absent. | | on | registered, off (default) | The schema is loaded. The `redirects` attribute exists on `Hyrax::Work` and `Hyrax::PcdmCollection`. The indexer is included on resource indexers but emits no Solr field. Routes/controllers/UI gates check Flipflop and stay silent. m3 profile may declare `redirects` (loaded but unused — a warning is emitted on profile validation). | -| on | on | All of the above, plus: the indexer emits `redirects_path_ssim`. The route/controller/UI engage. m3 profile validation **requires** the `redirects` property to be declared and available on `Hyrax::Work` and `Hyrax::PcdmCollection`. | +| on | on | All of the above, plus: the indexer emits `redirects_path_ssim`. The route/controller/UI engage. m3 profile validation **requires** the `redirects` property to be declared with `type: hash` and available on at least one work or collection class declared in the profile. | The two-layer split is deliberate: the application-level config controls *availability* (the schema is structural — toggling it after data is written would orphan persisted entries), and the Flipflop controls *use* at request time. @@ -72,16 +72,16 @@ properties: redirects: available_on: class: - - Hyrax::Work - - Hyrax::PcdmCollection + - GenericWork + - CollectionResource cardinality: minimum: 0 - type: redirect + type: hash multiple: true predicate: http://samvera.org/ns/hyku/redirects ``` -The `type: redirect` token resolves to `Hyrax::Redirect`, a `Valkyrie::Resource` with `path`, `canonical`, and `sequence` sub-attributes. `multiple: true` is required for nested-resource members; the schema loader raises `ArgumentError` if a nested-resource property is declared with `multiple: false`. +Each redirect entry is a plain hash with `path`, `canonical`, and `sequence` keys, persisted as JSONB on the parent resource. The `type: hash` token resolves to `Dry::Types['hash']`, which round-trips entries through Postgres without the sub-field stripping a nested `Valkyrie::Resource` would suffer. `available_on.class` must include at least one work or collection class declared in this profile's top-level `classes:` block; substitute the class names your profile declares (`Image`, `Etd`, `Oer`, etc.) as appropriate. Validation matrix on profile save (with the config on): @@ -90,7 +90,8 @@ Validation matrix on profile save (with the config on): | off | absent | silent (the feature is not in active use) | | off | present | warning (property is loaded but unused) | | on | absent | error (property is required) | -| on | present, missing `Hyrax::Work` or `Hyrax::PcdmCollection` in `available_on.class` | error | +| on | declared without `type: hash` | error | +| on | `available_on.class` lists no work or collection class declared in this profile | error | | on | present, complete | silent (valid) | When the config is **off**, an m3 profile that declares `redirects` produces a warning rather than an error. The property is dead — it won't be loaded — but Hyrax doesn't refuse to save the profile. @@ -102,7 +103,7 @@ In default (non-flexible) mode the schema lives in `config/metadata/redirects.ya ```yaml attributes: redirects: - type: redirect + type: hash multiple: true form: primary: false @@ -114,10 +115,10 @@ attributes: When the config is on, this schema is loaded and `Hyrax::Work` / `Hyrax::PcdmCollection` include it via `Hyrax::Schema(:redirects)`. The schema loader produces: ```ruby -attribute :redirects, Valkyrie::Types::Set.of(Hyrax::Redirect) +attribute :redirects, Valkyrie::Types::Array.of(Dry::Types['hash']) ``` -`Set` (rather than `Array`) is used so instance round-trips through assignment don't re-invoke the Dry::Struct constructor on existing entries — `Array.of(Resource)` raises `can't convert Object into Hash` on `work.redirects = work.redirects`. +Each entry is a plain hash with `path`, `canonical`, and `sequence` keys. Entries persist as JSONB on the parent resource, so sub-fields round-trip cleanly without an intermediate nested-resource schema mangling them. `Hyrax::Redirect` is retained as a thin Ruby presenter the form view consumes; non-form code (validator, indexer, sync step) reads the persisted hash directly. When the config is off, the loader filters `redirects.yaml` out of the schema set entirely. The file is on disk but invisible to `permissive_schema_for_valkrie_adapter`, `Hyrax::Schema(:redirects)`, and any other consumer of the simple schema loader. @@ -132,7 +133,7 @@ Every redirect path in Hyrax — whether typed into a form, looked up by the res The normalizer is idempotent — `normalize(normalize(x)) == normalize(x)` — and is wired in at four call sites so they all agree on the canonical form: -- `Hyrax::Forms::ResourceForm#redirects=` normalizes each entry's path on form assignment, before validation. The normalized form is what the validator sees and what the resource persists. +- `Hyrax::RedirectsFieldBehavior#redirects_attributes_populator` normalizes each entry's path on form submission, before validation. The normalized form is what the validator sees and what the resource persists. - `Hyrax::RedirectsController#show` (the resolver) normalizes the incoming request path before the Solr lookup, so `/foo/` and `/foo` both resolve. - `Hyrax::RedirectsLookup` normalizes its input on construction, so callers can pass any reasonable form. - `Hyrax::Transactions::Steps::SyncRedirectPaths` normalizes paths before writing to the ledger, as defense in depth. @@ -194,6 +195,23 @@ Neither step does anything when the config is off, so adopters who don't enable ## Resolver behavior +### Route placement + +The redirect resolver is wired up as a **catch-all route in the host application's `config/routes.rb`**, not in the Hyrax engine. It must be the **last route in the host application** so every other route — engine mounts, host-specific routes, and `curation_concerns_basic_routes` — gets first crack at matching the request. The install generator appends the route at the end of `config/routes.rb`: + +```ruby +# config/routes.rb (host app, end of file) +get '*alias_path', to: 'hyrax/redirects#show', + constraints: ->(_req) { Hyrax.config.redirects_active? }, + format: false +``` + +The constraint lambda is evaluated on every request: when `redirects_active?` is false (config off, or Flipflop off, or both), the catch-all is transparent and Rails returns its default 404 for any path that didn't match an earlier route. When `redirects_active?` is true, the request reaches `Hyrax::RedirectsController#show`. + +Adopters with existing installs (predating this feature) need to add the catch-all line manually. Adopters who run a custom catch-all (e.g. a 404 page handler) should put the redirect line *before* their handler, so registered redirects take precedence and unregistered paths fall through to the custom 404. + +### What happens at request time + When both gates are open, `Hyrax::RedirectsController#show` serves any path not claimed by an earlier route: - The incoming path is normalized via `Hyrax::RedirectPathNormalizer` so the lookup matches the canonical form stored in Solr (a request for `/foo/` resolves the same record as `/foo`). @@ -238,7 +256,7 @@ To disable the feature at runtime without changing the config, toggle the `:redi ### Caveats -- **`Hyrax::Redirect` (the resource class) is always defined.** The class file is loaded by Rails autoloading as soon as anything references the constant. It costs effectively nothing when unused. The "wholly absent" effect of disabling the config applies to the schema, the attribute on `Hyrax::Work` / `Hyrax::PcdmCollection`, the Flipflop, the indexer, and the m3 profile validator's enforcement — but not to the constant itself. +- **`Hyrax::Redirect` (the presenter) is always defined.** The class file is loaded by Rails autoloading as soon as anything references the constant. It costs effectively nothing when unused. The "wholly absent" effect of disabling the config applies to the schema, the attribute on `Hyrax::Work` / `Hyrax::PcdmCollection`, the Flipflop, the indexer, and the m3 profile validator's enforcement — but not to the constant itself. - **The `disabled_schemas` filter only affects `Hyrax::SimpleSchemaLoader`.** Adopters running `flexible: true` whose m3 profile contains a stale `redirects` property will still see the attribute defined on records loaded via `M3SchemaLoader` even if `Hyrax.config.redirects_enabled?` is false. The m3 profile validator emits a warning in that situation ("the property will be ignored"); follow the warning by removing the `redirects` property from the m3 profile or setting the config back on. ## For contributors diff --git a/lib/generators/hyrax/install_generator.rb b/lib/generators/hyrax/install_generator.rb index 59afee88a5..8b59f4d4b0 100644 --- a/lib/generators/hyrax/install_generator.rb +++ b/lib/generators/hyrax/install_generator.rb @@ -61,7 +61,7 @@ def insert_builder end # The engine routes have to come after the devise routes so that /users/sign_in will work - def inject_routes + def inject_routes # rubocop:disable Metrics/MethodLength # Remove root route that was added by blacklight generator gsub_file 'config/routes.rb', /root (:to =>|to:) "catalog#index"/, '' @@ -73,6 +73,19 @@ def inject_routes " root 'hyrax/homepage#index'\n"\ " curation_concerns_basic_routes\n"\ end + + # Catch-all redirect resolver. Must be the last route in the host + # application so that every other route — engine mounts, host-specific + # routes, curation concerns — gets first crack at matching the request. + # Gated by Hyrax.config.redirects_active? at request time so the route is + # transparent when the redirects feature is off. + # See documentation/redirects.md. + inject_into_file 'config/routes.rb', before: /^end\s*\Z/ do + "\n # Catch-all redirect resolver — must be last. See documentation/redirects.md.\n"\ + " get '*alias_path', to: 'hyrax/redirects#show',\n"\ + " constraints: ->(_req) { Hyrax.config.redirects_active? },\n"\ + " format: false\n" + end end # Add behaviors to the SolrDocument model diff --git a/lib/hyrax/configuration.rb b/lib/hyrax/configuration.rb index 9405d3bd55..510edafd47 100644 --- a/lib/hyrax/configuration.rb +++ b/lib/hyrax/configuration.rb @@ -350,6 +350,15 @@ def redirects_enabled? end alias redirects_enabled redirects_enabled? + # @return [Boolean] true when both feature gates are open. Single + # source of truth for "is the redirects feature actively in use + # right now?". Short-circuits on the config so the Flipflop call + # is safe (the :redirects feature is only registered when the + # config is on; calling Flipflop.redirects? otherwise raises). + def redirects_active? + redirects_enabled? && Flipflop.redirects? + end + # Path prefixes that may not be claimed as redirect aliases (Hyrax::RedirectValidator # rejects any redirect path equal to or starting with one of these). Defaults to the # routes Hyrax itself reserves; adopters can extend the list to cover host-app routes: diff --git a/lib/hyrax/specs/shared_specs/factories/hyrax_redirect.rb b/lib/hyrax/specs/shared_specs/factories/hyrax_redirect.rb deleted file mode 100644 index 54c58b16ec..0000000000 --- a/lib/hyrax/specs/shared_specs/factories/hyrax_redirect.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true -FactoryBot.define do - factory :hyrax_redirect, class: "Hyrax::Redirect" do - sequence(:path) { |n| "/legacy/#{n}" } - canonical { false } - - trait :canonical do - canonical { true } - end - end -end diff --git a/lib/hyrax/transactions/steps/sync_redirect_paths.rb b/lib/hyrax/transactions/steps/sync_redirect_paths.rb index d3c674e820..d274d9f8c6 100644 --- a/lib/hyrax/transactions/steps/sync_redirect_paths.rb +++ b/lib/hyrax/transactions/steps/sync_redirect_paths.rb @@ -31,13 +31,14 @@ def call(object) private def syncable?(object) - return false unless Hyrax.config.redirects_enabled? && Flipflop.redirects? + return false unless Hyrax.config.redirects_active? object.respond_to?(:redirects) && object.respond_to?(:id) && object.id.present? end def build_rows(object) + # Valkyrie's JSONValueMapper symbolizes hash keys on read; accept either. paths = Array(object.redirects) - .map { |entry| Hyrax::RedirectPathNormalizer.call(entry.try(:path)) } + .map { |entry| Hyrax::RedirectPathNormalizer.call(entry['path'] || entry[:path]) } .reject(&:blank?) .uniq now = Time.current diff --git a/spec/controllers/hyrax/redirects_controller_spec.rb b/spec/controllers/hyrax/redirects_controller_spec.rb index af2b0f8bde..0a12cd70d8 100644 --- a/spec/controllers/hyrax/redirects_controller_spec.rb +++ b/spec/controllers/hyrax/redirects_controller_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe Hyrax::RedirectsController, type: :controller do - routes { Hyrax::Engine.routes } + routes { Rails.application.routes } describe '#show' do let(:work_id) { 'abc-123-xyz' } diff --git a/spec/indexers/hyrax/indexers/redirects_indexer_spec.rb b/spec/indexers/hyrax/indexers/redirects_indexer_spec.rb index 63fa75419b..64fd18c9df 100644 --- a/spec/indexers/hyrax/indexers/redirects_indexer_spec.rb +++ b/spec/indexers/hyrax/indexers/redirects_indexer_spec.rb @@ -1,16 +1,12 @@ # frozen_string_literal: true RSpec.describe Hyrax::Indexers::RedirectsIndexer do - # Minimal Valkyrie::Resource that carries the same `redirects` attribute - # shape as Hyrax::Work / Hyrax::PcdmCollection do when the redirects - # schema is included. Defined here so the spec runs without depending on - # Hyrax.config.redirects_enabled? being toggled in the test env. let(:resource_class) do Class.new(Hyrax::Resource) do def self.name 'TestResourceWithRedirects' end - attribute :redirects, Valkyrie::Types::Set.of(Hyrax::Redirect) + attribute :redirects, Valkyrie::Types::Array.of(Dry::Types['hash']) end end @@ -22,14 +18,14 @@ def self.name let(:indexer) { host_indexer_class.new(resource: resource) } - context 'with the :redirects feature flag on' do - before { allow(Flipflop).to receive(:redirects?).and_return(true) } + context 'with the redirects feature active' do + before { allow(Hyrax.config).to receive(:redirects_active?).and_return(true) } context 'for a resource with redirects entries' do let(:resource) do resource_class.new(redirects: [ - Hyrax::Redirect.new(path: '/handle/12345/678'), - Hyrax::Redirect.new(path: '/islandora/object/ir:1138') + { 'path' => '/handle/12345/678' }, + { 'path' => '/islandora/object/ir:1138' } ]) end @@ -65,8 +61,8 @@ def self.name context 'when an entry is missing a path' do let(:resource) do resource_class.new(redirects: [ - Hyrax::Redirect.new(path: '/handle/12345/678'), - Hyrax::Redirect.new(path: nil) + { 'path' => '/handle/12345/678' }, + { 'path' => nil } ]) end @@ -77,11 +73,11 @@ def self.name end end - context 'with the :redirects feature flag off' do - before { allow(Flipflop).to receive(:redirects?).and_return(false) } + context 'with the redirects feature inactive' do + before { allow(Hyrax.config).to receive(:redirects_active?).and_return(false) } let(:resource) do - resource_class.new(redirects: [Hyrax::Redirect.new(path: '/handle/12345/678')]) + resource_class.new(redirects: [{ 'path' => '/handle/12345/678' }]) end it 'does not emit redirects_path_ssim' do diff --git a/spec/lib/hyrax/transactions/steps/sync_redirect_paths_spec.rb b/spec/lib/hyrax/transactions/steps/sync_redirect_paths_spec.rb index 6ffad847c2..41d0a78f73 100644 --- a/spec/lib/hyrax/transactions/steps/sync_redirect_paths_spec.rb +++ b/spec/lib/hyrax/transactions/steps/sync_redirect_paths_spec.rb @@ -4,16 +4,14 @@ subject(:step) { described_class.new } let(:resource_id) { 'res-1' } - let(:entry_class) { Struct.new(:path, keyword_init: true) } let(:resource_class) do Struct.new(:id, :redirects) end let(:resource) { resource_class.new(resource_id, redirects) } - let(:redirects) { [entry_class.new(path: '/handle/1'), entry_class.new(path: '/handle/2')] } + let(:redirects) { [{ 'path' => '/handle/1' }, { 'path' => '/handle/2' }] } before do - allow(Hyrax.config).to receive(:redirects_enabled?).and_return(true) - allow(Flipflop).to receive(:redirects?).and_return(true) + allow(Hyrax.config).to receive(:redirects_active?).and_return(true) Hyrax::RedirectPath.delete_all end @@ -38,17 +36,8 @@ end end - context 'when the config is off' do - before { allow(Hyrax.config).to receive(:redirects_enabled?).and_return(false) } - - it 'is a no-op (returns Success without touching the table)' do - expect(Hyrax::RedirectPath).not_to receive(:transaction) - expect(step.call(resource)).to be_success - end - end - - context 'when the Flipflop is off' do - before { allow(Flipflop).to receive(:redirects?).and_return(false) } + context 'when the redirects feature is inactive' do + before { allow(Hyrax.config).to receive(:redirects_active?).and_return(false) } it 'is a no-op (returns Success without touching the table)' do expect(Hyrax::RedirectPath).not_to receive(:transaction) @@ -87,7 +76,7 @@ let(:original_created_at) { 1.day.ago.change(usec: 0) } let(:redirects) do # Same paths as the existing rows, just listed in reverse order on the resource. - [entry_class.new(path: '/handle/2'), entry_class.new(path: '/handle/1')] + [{ 'path' => '/handle/2' }, { 'path' => '/handle/1' }] end before do diff --git a/spec/models/hyrax/redirect_spec.rb b/spec/models/hyrax/redirect_spec.rb index 5a4c9d9998..f4cc0eb0cd 100644 --- a/spec/models/hyrax/redirect_spec.rb +++ b/spec/models/hyrax/redirect_spec.rb @@ -1,15 +1,7 @@ # frozen_string_literal: true -require 'valkyrie/specs/shared_specs' - RSpec.describe Hyrax::Redirect do - subject(:redirect) { described_class.new } - - it_behaves_like 'a Valkyrie::Resource' do - let(:resource_klass) { described_class } - end - - describe 'attributes' do + describe '#initialize' do it 'accepts a path, canonical flag, and sequence' do r = described_class.new(path: '/handle/12345/678', canonical: true, sequence: 0) expect(r.path).to eq('/handle/12345/678') @@ -22,9 +14,68 @@ expect(r.canonical).to be false end - it 'allows sequence to be omitted' do - r = described_class.new(path: '/foo') + it 'allows path and sequence to be nil (e.g. for the trailing blank row in the form view)' do + r = described_class.new(path: nil) + expect(r.path).to be_nil expect(r.sequence).to be_nil end end + + describe '.wrap' do + it 'returns nil for nil input' do + expect(described_class.wrap(nil)).to be_nil + end + + it 'returns the same instance when input is already a presenter' do + r = described_class.new(path: '/foo') + expect(described_class.wrap(r)).to equal(r) + end + + it 'builds a presenter from a string-keyed hash (JSONB shape)' do + r = described_class.wrap('path' => '/foo', 'canonical' => true, 'sequence' => 2) + expect(r).to have_attributes(path: '/foo', canonical: true, sequence: 2) + end + + it 'builds a presenter from a symbol-keyed hash' do + r = described_class.wrap(path: '/foo', canonical: true, sequence: 2) + expect(r).to have_attributes(path: '/foo', canonical: true, sequence: 2) + end + + it 'defaults canonical to false when omitted' do + r = described_class.wrap('path' => '/foo') + expect(r.canonical).to be false + end + + it 'raises ArgumentError when input cannot be coerced to a hash' do + expect { described_class.wrap(42) }.to raise_error(ArgumentError, /Hyrax::Redirect/) + end + end + + describe '#to_h and #as_json' do + it 'returns a string-keyed hash matching the persisted shape' do + r = described_class.new(path: '/foo', canonical: true, sequence: 1) + expected = { 'path' => '/foo', 'canonical' => true, 'sequence' => 1 } + expect(r.to_h).to eq(expected) + expect(r.as_json).to eq(expected) + end + end + + describe '#==' do + it 'compares equal on attribute values' do + a = described_class.new(path: '/x', canonical: true, sequence: 0) + b = described_class.new(path: '/x', canonical: true, sequence: 0) + expect(a).to eq(b) + end + + it 'differs when any attribute differs' do + a = described_class.new(path: '/x') + b = described_class.new(path: '/y') + expect(a).not_to eq(b) + end + + it 'is not equal to a Hash with the same shape (presenters and hashes are distinct types)' do + r = described_class.new(path: '/x', canonical: false, sequence: nil) + expect(r).not_to eq('path' => '/x', 'canonical' => false, 'sequence' => nil) + end + end end diff --git a/spec/services/hyrax/flexible_schema_validators/redirects_validator_spec.rb b/spec/services/hyrax/flexible_schema_validators/redirects_validator_spec.rb index f2690f3dcc..02bd7a23ac 100644 --- a/spec/services/hyrax/flexible_schema_validators/redirects_validator_spec.rb +++ b/spec/services/hyrax/flexible_schema_validators/redirects_validator_spec.rb @@ -7,9 +7,14 @@ let(:profile_with_redirects) do { + 'classes' => { + 'GenericWork' => {}, + 'CollectionResource' => {} + }, 'properties' => { 'redirects' => { - 'available_on' => { 'class' => %w[Hyrax::Work Hyrax::PcdmCollection] } + 'type' => 'hash', + 'available_on' => { 'class' => %w[GenericWork CollectionResource] } } } } @@ -19,6 +24,18 @@ { 'properties' => { 'title' => {} } } end + before do + # In the engine spec environment, hyrax/file_set is registered as a + # curation concern (see spec/support/flexible_metadata_setup.rb), which + # propagates into work_class_names. Stub that scenario explicitly so + # the validator is forced to actively exclude file_set rather than + # relying on it being absent. + allow(Hyrax::ModelRegistry).to receive(:work_class_names).and_return(['GenericWork', 'Hyrax::FileSet']) + allow(Hyrax::ModelRegistry).to receive(:collection_class_names).and_return(['CollectionResource']) + allow(Hyrax::ModelRegistry).to receive(:file_set_class_names).and_return(['Hyrax::FileSet']) + allow(Hyrax::ModelRegistry).to receive(:admin_set_class_names).and_return(['Hyrax::AdministrativeSet']) + end + describe '#validate!' do context 'when Hyrax.config.redirects_enabled? is false' do before { allow(Hyrax.config).to receive(:redirects_enabled?).and_return(false) } @@ -80,7 +97,7 @@ context 'and the m3 profile has no `redirects` property' do let(:profile) { profile_without_redirects } - it 'errors that the property is required' do + it 'records an error that the property is required' do validator.validate! expect(errors).to include(/m3 profile must declare a `redirects` property/) end @@ -96,33 +113,59 @@ end end - context 'and the m3 profile has `redirects` but is missing Hyrax::PcdmCollection' do + context 'and the m3 profile has `redirects` available_on no work or collection class declared in the profile' do + let(:profile) do + { + 'classes' => { 'GenericWork' => {}, 'CollectionResource' => {} }, + 'properties' => { + 'redirects' => { + 'type' => 'hash', + 'available_on' => { 'class' => ['SomeOtherClass'] } + } + } + } + end + + it 'records an error that the property must be available on a declared work or collection class' do + validator.validate! + expect(errors).to include(/`redirects`.*available on.*work or collection class/) + end + end + + context 'and the m3 profile has `redirects` available_on a FileSet (registered as a curation concern)' do let(:profile) do { + 'classes' => { 'Hyrax::FileSet' => {} }, 'properties' => { - 'redirects' => { 'available_on' => { 'class' => ['Hyrax::Work'] } } + 'redirects' => { + 'type' => 'hash', + 'available_on' => { 'class' => ['Hyrax::FileSet'] } + } } } end - it 'errors that the missing class must be in available_on.class' do + it 'records an error that the property must be available on a declared work or collection class' do validator.validate! - expect(errors).to include(/`redirects`.*available on.*Hyrax::PcdmCollection/) + expect(errors).to include(/`redirects`.*available on.*work or collection class/) end end - context 'and the m3 profile has `redirects` but is missing Hyrax::Work' do + context 'and the m3 profile has `redirects` declared without `type: hash`' do let(:profile) do { + 'classes' => { 'GenericWork' => {}, 'CollectionResource' => {} }, 'properties' => { - 'redirects' => { 'available_on' => { 'class' => ['Hyrax::PcdmCollection'] } } + 'redirects' => { + 'available_on' => { 'class' => %w[GenericWork CollectionResource] } + } } } end - it 'errors that the missing class must be in available_on.class' do + it 'records an error that the property must declare type: hash' do validator.validate! - expect(errors).to include(/`redirects`.*available on.*Hyrax::Work/) + expect(errors).to include(/`redirects`.*declare `type: hash`/) end end end diff --git a/spec/services/hyrax/schema_loader_spec.rb b/spec/services/hyrax/schema_loader_spec.rb index 111bb3713b..16877390ad 100644 --- a/spec/services/hyrax/schema_loader_spec.rb +++ b/spec/services/hyrax/schema_loader_spec.rb @@ -94,58 +94,42 @@ end end - context 'when type is not recognized' do - let(:config) { { 'type' => 'custom' } } - - it 'raises an ArgumentError' do - expect { attribute_definition.type }.to raise_error(ArgumentError, 'Unrecognized type: custom') - end - end - - context 'when the type names a nested Hyrax resource class (short form)' do - # In a YAML schema, `type: redirect` resolves to `Hyrax::Redirect`, - # so adopters can declare nested-resource attributes without - # registering anything in the Valkyrie::Types namespace. - let(:config) { { 'type' => 'redirect', 'multiple' => true } } - - it 'wraps the nested resource class in a typed array constructor' do + context 'when type is hash' do + # The `hash` shortcut lets a YAML schema declare a nested-attribute + # property whose entries are plain hashes (e.g. `redirects` with + # `path`, `canonical`, `sequence` sub-fields). Persisted as JSONB + # without a nested Valkyrie::Resource schema in between, so round-trips + # don't strip sub-fields. See documentation/redirects.md. + let(:config) { { 'type' => 'hash', 'multiple' => true } } + + it 'returns an array-of-hash typed constructor that round-trips hash entries' do expect(attribute_definition.type).to be_a(Dry::Types::Constructor) - expect(attribute_definition.type.member).to eq(Hyrax::Redirect) + expect(attribute_definition.type.to_s).to include('Array') + # End-to-end: an array of hashes coerces to itself, preserving sub-keys. + input = [{ 'path' => '/foo', 'canonical' => true, 'sequence' => 0 }] + expect(attribute_definition.type.call(input)).to eq(input) end end - context 'when a nested Hyrax resource type is declared with multiple: false' do - # Single-value nested resources are not supported because the schema - # loader's #type would return the bare class rather than a Dry::Types::Type, - # producing a downstream shape inconsistency. The loader rejects this - # configuration explicitly. - let(:config) { { 'type' => 'redirect', 'multiple' => false } } + context 'when type is hash with multiple: false' do + let(:config) { { 'type' => 'hash', 'multiple' => false } } - it 'raises ArgumentError' do - expect { attribute_definition.type } - .to raise_error(ArgumentError, /nested resource members require `multiple: true`/) + it 'returns a bare Dry::Types["hash"]' do + expect(attribute_definition.type).to eq(Dry::Types['hash']) end end - context 'when the type name has multiple words' do - # The lookup uses String#classify (not capitalize), so multi-word - # types like `access_control` resolve to `Hyrax::AccessControl`. - it 'resolves a multi-word type via #classify' do - expect(attribute_definition.send(:type_for, 'access_control')).to eq(Hyrax::AccessControl) - end - end + context 'when type is not recognized' do + let(:config) { { 'type' => 'custom' } } - context 'when the type is a fully-qualified class name' do - # Adopters with their own namespace can write `type: MyApp::Citation` - # in YAML and the loader will resolve it directly. - it 'returns the named class' do - expect(attribute_definition.send(:type_for, 'Hyrax::Redirect')).to eq(Hyrax::Redirect) + it 'raises an ArgumentError' do + expect { attribute_definition.type }.to raise_error(ArgumentError, 'Unrecognized type: custom') end end - context 'when the type name matches a class that is not a Valkyrie::Resource' do - # ::StringIO is a real class but not a resource. Resolution must reject - # it rather than silently use it as a nested-attribute type. + context 'when the type name matches an unrelated Ruby class' do + # ::StringIO is a real class but not a Valkyrie::Types::*. Resolution + # must reject it rather than silently match it via #classify. let(:config) { { 'type' => 'string_io' } } it 'raises ArgumentError' do @@ -156,8 +140,7 @@ context 'when the type name singularizes via #classify (e.g. "data" -> "Datum")' do # String#classify singularizes, which can surprise. Confirm that an # unrecognized type still raises ArgumentError regardless of inflection - # quirks — neither Hyrax::Datum, ::Datum, Valkyrie::Types::Datum, nor - # any other resolution short-circuits the safety check. + # quirks. let(:config) { { 'type' => 'data' } } it 'raises ArgumentError' do diff --git a/spec/validators/hyrax/redirect_validator_spec.rb b/spec/validators/hyrax/redirect_validator_spec.rb index 4efdae825e..7c33819e1e 100644 --- a/spec/validators/hyrax/redirect_validator_spec.rb +++ b/spec/validators/hyrax/redirect_validator_spec.rb @@ -16,18 +16,19 @@ r.redirects = entries end end - let(:entry_class) { Struct.new(:path, :canonical, keyword_init: true) } + def entry(path:, canonical: false) + { 'path' => path, 'canonical' => canonical } + end let(:entries) { [] } before do - allow(Hyrax.config).to receive(:redirects_enabled?).and_return(true) - allow(Flipflop).to receive(:redirects?).and_return(true) + allow(Hyrax.config).to receive(:redirects_active?).and_return(true) allow(Hyrax::RedirectsLookup).to receive(:taken?).and_return(false) end describe '#validate_each' do - context 'when both gates are open' do - let(:entries) { [entry_class.new(path: '/handle/12345/678', canonical: true)] } + context 'when the redirects feature is active' do + let(:entries) { [entry(path: '/handle/12345/678', canonical: true)] } it 'is valid' do record.valid? @@ -35,19 +36,9 @@ end end - context 'when the config is off' do - before { allow(Hyrax.config).to receive(:redirects_enabled?).and_return(false) } - let(:entries) { [entry_class.new(path: 'no-leading-slash', canonical: false)] } - - it 'short-circuits without raising or recording errors' do - record.valid? - expect(record.errors[:redirects]).to be_empty - end - end - - context 'when the Flipflop is off' do - before { allow(Flipflop).to receive(:redirects?).and_return(false) } - let(:entries) { [entry_class.new(path: 'no-leading-slash', canonical: false)] } + context 'when the redirects feature is inactive' do + before { allow(Hyrax.config).to receive(:redirects_active?).and_return(false) } + let(:entries) { [entry(path: 'no-leading-slash', canonical: false)] } it 'short-circuits without recording errors' do record.valid? @@ -116,7 +107,7 @@ def t(key, **interp) end context 'with a blank path' do - let(:entries) { [entry_class.new(path: '', canonical: false)] } + let(:entries) { [entry(path: '', canonical: false)] } it 'records a blank-path error' do record.valid? @@ -125,7 +116,7 @@ def t(key, **interp) end context 'with a path missing a leading slash' do - let(:entries) { [entry_class.new(path: 'handle/12345/678', canonical: false)] } + let(:entries) { [entry(path: 'handle/12345/678', canonical: false)] } it 'records a format error' do record.valid? @@ -134,7 +125,7 @@ def t(key, **interp) end context 'with whitespace in the path' do - let(:entries) { [entry_class.new(path: '/has space', canonical: false)] } + let(:entries) { [entry(path: '/has space', canonical: false)] } it 'records a format error' do record.valid? @@ -143,7 +134,7 @@ def t(key, **interp) end context 'with a query string in the path' do - let(:entries) { [entry_class.new(path: '/foo?bar=baz', canonical: false)] } + let(:entries) { [entry(path: '/foo?bar=baz', canonical: false)] } it 'records a format error' do record.valid? @@ -152,7 +143,7 @@ def t(key, **interp) end context 'with a path under a reserved Hyrax prefix' do - let(:entries) { [entry_class.new(path: '/concern/generic_works/abc', canonical: false)] } + let(:entries) { [entry(path: '/concern/generic_works/abc', canonical: false)] } it 'records a reserved-prefix error' do record.valid? @@ -161,7 +152,7 @@ def t(key, **interp) end context 'with a path that exactly matches a reserved prefix' do - let(:entries) { [entry_class.new(path: '/dashboard', canonical: false)] } + let(:entries) { [entry(path: '/dashboard', canonical: false)] } it 'records a reserved-prefix error' do record.valid? @@ -172,8 +163,22 @@ def t(key, **interp) context 'with two entries sharing the same path on the same record' do let(:entries) do [ - entry_class.new(path: '/handle/1', canonical: false), - entry_class.new(path: '/handle/1', canonical: false) + entry(path: '/handle/1', canonical: false), + entry(path: '/handle/1', canonical: false) + ] + end + + it 'records an intra-record duplicate error' do + record.valid? + expect(record.errors[:redirects]).to include(t(:intra_record_duplicate, path: '/handle/1')) + end + end + + context 'with two entries that normalize to the same canonical path' do + let(:entries) do + [ + entry(path: '/handle/1', canonical: false), + entry(path: '/handle/1/', canonical: false) ] end @@ -183,8 +188,17 @@ def t(key, **interp) end end + context 'with a reserved prefix typed with a trailing slash' do + let(:entries) { [entry(path: '/dashboard/', canonical: false)] } + + it 'records a reserved-prefix error' do + record.valid? + expect(record.errors[:redirects]).to include(t(:reserved_prefix, path: '/dashboard/')) + end + end + context 'when the path is taken on another record' do - let(:entries) { [entry_class.new(path: '/handle/12345/678', canonical: false)] } + let(:entries) { [entry(path: '/handle/12345/678', canonical: false)] } before do allow(Hyrax::RedirectsLookup) @@ -199,11 +213,27 @@ def t(key, **interp) end end + context 'when a non-canonical form of the path is taken on another record' do + let(:entries) { [entry(path: '/handle/12345/678/', canonical: false)] } + + before do + allow(Hyrax::RedirectsLookup) + .to receive(:taken?) + .with('/handle/12345/678', except_id: 'self-id') + .and_return(true) + end + + it 'records a global-uniqueness error against the canonical path' do + record.valid? + expect(record.errors[:redirects]).to include(t(:already_taken, path: '/handle/12345/678/')) + end + end + context 'when more than one entry is marked canonical' do let(:entries) do [ - entry_class.new(path: '/a', canonical: true), - entry_class.new(path: '/b', canonical: true) + entry(path: '/a', canonical: true), + entry(path: '/b', canonical: true) ] end @@ -216,8 +246,8 @@ def t(key, **interp) context 'when zero entries are marked canonical' do let(:entries) do [ - entry_class.new(path: '/a', canonical: false), - entry_class.new(path: '/b', canonical: false) + entry(path: '/a', canonical: false), + entry(path: '/b', canonical: false) ] end @@ -227,4 +257,19 @@ def t(key, **interp) end end end + + describe '#canonical_for' do + it 'returns the stored false flag rather than nil' do + expect(validator.send(:canonical_for, 'canonical' => false)).to be(false) + expect(validator.send(:canonical_for, canonical: false)).to be(false) + end + + it 'returns true when the flag is set' do + expect(validator.send(:canonical_for, 'canonical' => true)).to be(true) + end + + it 'returns nil when the flag is absent' do + expect(validator.send(:canonical_for, 'path' => '/x')).to be_nil + end + end end From 9cacd92a0d4f5acae0861eab90e80b90b25c588d Mon Sep 17 00:00:00 2001 From: LaRita Robinson Date: Mon, 4 May 2026 16:39:38 -0400 Subject: [PATCH 3/4] Add the Aliases tab and validator wiring Adds the Aliases tab to work and collection edit forms when the redirects feature is active. Wires the validator into the form's validation chain so collisions and invalid paths are caught before save. Bridges the form layer to the persisted hash shape via a populator and prepopulator on the resource form. --- .../hyrax/redirects_field_behavior.rb | 80 +++++++++++ app/forms/hyrax/forms/resource_form.rb | 9 +- app/helpers/hyrax/collections_helper.rb | 2 + app/helpers/hyrax/redirects_tab_helper.rb | 29 ++++ app/helpers/hyrax/work_form_helper.rb | 14 +- app/views/hyrax/base/_form_redirects.html.erb | 52 +++++++ .../dashboard/collections/_form.html.erb | 17 +++ config/locales/hyrax.en.yml | 16 +++ .../hyrax/redirects_field_behavior_spec.rb | 129 ++++++++++++++++++ .../hyrax/redirects_tab_helper_spec.rb | 80 +++++++++++ 10 files changed, 421 insertions(+), 7 deletions(-) create mode 100644 app/forms/concerns/hyrax/redirects_field_behavior.rb create mode 100644 app/helpers/hyrax/redirects_tab_helper.rb create mode 100644 app/views/hyrax/base/_form_redirects.html.erb create mode 100644 spec/forms/concerns/hyrax/redirects_field_behavior_spec.rb create mode 100644 spec/helpers/hyrax/redirects_tab_helper_spec.rb diff --git a/app/forms/concerns/hyrax/redirects_field_behavior.rb b/app/forms/concerns/hyrax/redirects_field_behavior.rb new file mode 100644 index 0000000000..bac11ef490 --- /dev/null +++ b/app/forms/concerns/hyrax/redirects_field_behavior.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module Hyrax + # Form-side handling for the `redirects` nested-attribute field. + # + # Submitted form payloads arrive under `redirects_attributes` and are + # turned into plain hashes (the persisted shape — see + # `config/metadata/redirects.yaml`, `type: hash`) by the populator. + # On render, the prepopulator wraps each persisted hash in a + # `Hyrax::Redirect` value object so the view can call `.path` / + # `.canonical` / `.sequence`. + # + # The `deserialize!` override removes the renamed `redirects` key + # before Reform's `from_hash` runs, so the form's `redirects` + # property is written exclusively by the populator. See + # `Hyrax::BasedNearFieldBehavior` for the parallel pattern. + # + # ## Feature gating + # + # `self.included` runs at class load time and uses the structural + # gate `Hyrax.config.redirects_enabled?`. The runtime Flipflop + # check isn't meaningful here because the form class is being + # defined, not handling a request. + # + # All runtime-side methods (`deserialize!`, populator, prepopulator) + # delegate to `Hyrax.config.redirects_active?`, the two-gate + # combinator (`redirects_enabled? && Flipflop.redirects?`). Calling + # `Flipflop.redirects?` directly is unsafe when the config is off + # because the feature isn't registered in that case. + module RedirectsFieldBehavior + def self.included(descendant) + return unless Hyrax.config.redirects_enabled? + descendant.property :redirects_attributes, + virtual: true, + populator: :redirects_attributes_populator, + prepopulator: :redirects_attributes_prepopulator + end + + # Reform's FormBuilderMethods rewrites `redirects_attributes` → + # `redirects` before `from_hash` runs. Strip the renamed key so + # the populator on `redirects_attributes` is the single entry + # point for form-driven writes. + def deserialize!(params) + result = super + if Hyrax.config.redirects_active? && result.respond_to?(:delete) + result.delete('redirects') + result.delete(:redirects) + end + result + end + + private + + # Builds plain hashes (the persisted shape) from the submitted + # `redirects_attributes` payload. Drops rows marked for destruction + # or with a blank path. Normalizes paths to the canonical form + # stored in the uniqueness ledger. + def redirects_attributes_populator(fragment:, **_options) + return unless respond_to?(:redirects) + return unless Hyrax.config.redirects_active? + entries = Array(fragment&.values) + .reject { |row| row['_destroy'].to_s == 'true' || row['path'].to_s.strip.empty? } + .each_with_index.map do |row, i| + { 'path' => Hyrax::RedirectPathNormalizer.call(row['path']), + 'canonical' => row['canonical'].to_s == 'true', + 'sequence' => row['sequence'].presence&.to_i || i } + end + self.redirects = entries + end + + # Wraps each persisted hash in a `Hyrax::Redirect` value object for + # the form view. Mirrors how `BasedNearFieldBehavior` hydrates URI + # strings into `ControlledVocabularies::Location` instances. + def redirects_attributes_prepopulator + return unless respond_to?(:redirects) + return unless Hyrax.config.redirects_active? + self.redirects = Array(redirects).map { |entry| Hyrax::Redirect.wrap(entry) } + end + end +end diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb index 5e243707aa..1c7b63e50d 100644 --- a/app/forms/hyrax/forms/resource_form.rb +++ b/app/forms/hyrax/forms/resource_form.rb @@ -27,6 +27,7 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength end include BasedNearFieldBehavior + include RedirectsFieldBehavior class_attribute :model_class property :human_readable_type, writable: false @@ -112,9 +113,13 @@ def initialize(deprecated_resource = nil, resource: nil) class << self def inherited(subclass) - # this is a noop if based near is not defined on a given model - # we need these to be before and included properties + # Field Behaviors must be prepended onto every subclass so their + # `deserialize!` overrides land above Reform's base method on the + # ancestor chain. Each behavior gates itself internally and is a + # no-op when its feature is off or its property isn't on the + # subclass's model. subclass.prepend(BasedNearFieldBehavior) + subclass.prepend(RedirectsFieldBehavior) super end diff --git a/app/helpers/hyrax/collections_helper.rb b/app/helpers/hyrax/collections_helper.rb index 3fef59899d..09374db983 100644 --- a/app/helpers/hyrax/collections_helper.rb +++ b/app/helpers/hyrax/collections_helper.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Hyrax module CollectionsHelper # rubocop:disable Metrics/ModuleLength + include Hyrax::RedirectsTabHelper + ## # @since 3.1.0 # @return [Array] diff --git a/app/helpers/hyrax/redirects_tab_helper.rb b/app/helpers/hyrax/redirects_tab_helper.rb new file mode 100644 index 0000000000..28eee88544 --- /dev/null +++ b/app/helpers/hyrax/redirects_tab_helper.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Hyrax + ## + # Decides whether the Aliases tab should appear on a work or + # collection edit form. Included into `WorkFormHelper` and + # `CollectionsHelper` so both forms share one rule. + # + # The tab appears only when the redirects feature is fully active + # (`Hyrax.config.redirects_active?`), the form is a ResourceForm + # (the only form pipeline with the redirects populator, validator, + # and prepopulator wired in), AND the form's underlying resource + # carries the `redirects` attribute. The structural check guards + # against adopter work or collection classes that don't include + # the redirects schema; without it the tab would render and crash + # on `f.object.redirects`. + module RedirectsTabHelper + def redirects_tab?(form) + return false unless Hyrax.config.redirects_active? + return false unless redirects_supported_form?(form) + target = form.respond_to?(:model) ? form.model : form + target.respond_to?(:redirects) + end + + def redirects_supported_form?(form) + form.is_a?(Hyrax::Forms::ResourceForm) + end + end +end diff --git a/app/helpers/hyrax/work_form_helper.rb b/app/helpers/hyrax/work_form_helper.rb index 4ac993465a..d6f4b1faac 100644 --- a/app/helpers/hyrax/work_form_helper.rb +++ b/app/helpers/hyrax/work_form_helper.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Hyrax module WorkFormHelper + include Hyrax::RedirectsTabHelper + ## # @todo this implementation hits database backends (solr) and is invoked # from views. refactor to avoid @@ -30,11 +32,13 @@ def admin_set_options # @param form [Hyrax::Forms::WorkForm, Hyrax::Forms::ResourceForm] # @return [Array] the list of names of tabs to be rendered in the form def form_tabs_for(form:) - if form.instance_of? Hyrax::Forms::BatchUploadForm - %w[files metadata relationships] - else - %w[metadata files relationships] - end + tabs = if form.instance_of? Hyrax::Forms::BatchUploadForm + %w[files metadata relationships] + else + %w[metadata files relationships] + end + tabs << 'redirects' if redirects_tab?(form) + tabs end ## diff --git a/app/views/hyrax/base/_form_redirects.html.erb b/app/views/hyrax/base/_form_redirects.html.erb new file mode 100644 index 0000000000..0faa9e57b3 --- /dev/null +++ b/app/views/hyrax/base/_form_redirects.html.erb @@ -0,0 +1,52 @@ +<%# Aliases tab — see documentation/redirects.md %> +<% return unless redirects_tab?(f.object) %> +

<%= t('hyrax.works.form.redirects.heading') %>

+ +

<%= sanitize t('hyrax.works.form.redirects.help_html'), tags: %w[code] %>

+ + + + + + + + + + + <%# Wrap each row through Hyrax::Redirect.wrap so the view can rely on + the presenter API (.path, .canonical, .sequence). On initial render + the prepopulator already wrapped them; on a re-render after a + failed save, f.object.redirects holds the populator's plain hashes, + and we wrap them here. %> + <% rows = Array(f.object.redirects).map { |e| Hyrax::Redirect.wrap(e) } + [Hyrax::Redirect.new(path: nil)] %> + <% rows.each_with_index do |entry, index| %> + <% input_id = "#{f.object_name}_redirects_attributes_#{index}_path" %> + <% label_text = entry.path.present? ? t('hyrax.works.form.redirects.path_label') : t('hyrax.works.form.redirects.new_path_label') %> + + + + + <% end %> + +
<%= t('hyrax.works.form.redirects.caption') %>
<%= t('hyrax.works.form.redirects.header.path') %><%= t('hyrax.works.form.redirects.header.actions') %>
+ <%= label_tag input_id, label_text, class: 'sr-only' %> +
+
+ <%= request.base_url %> +
+ <%= text_field_tag "#{f.object_name}[redirects_attributes][#{index}][path]", + entry.path, + id: input_id, + class: 'form-control', + placeholder: t('hyrax.works.form.redirects.placeholder.path') %> +
+ 		+ <% if entry.path.present? %> + + <% end %> +
diff --git a/app/views/hyrax/dashboard/collections/_form.html.erb b/app/views/hyrax/dashboard/collections/_form.html.erb index 5e8c87441d..a19ee698b0 100644 --- a/app/views/hyrax/dashboard/collections/_form.html.erb +++ b/app/views/hyrax/dashboard/collections/_form.html.erb @@ -28,6 +28,13 @@ <% end %> + <% if redirects_tab?(@form) %> + + <% end %> <% end %> @@ -109,6 +116,16 @@ <% end %> + + <% if redirects_tab?(f.object) %> +
+
+
+ <%= render 'hyrax/base/form_redirects', f: f %> +
+
+
+ <% end %> <% end %>
diff --git a/config/locales/hyrax.en.yml b/config/locales/hyrax.en.yml index 3b183fe5e4..f001dc51ff 100644 --- a/config/locales/hyrax.en.yml +++ b/config/locales/hyrax.en.yml @@ -925,6 +925,7 @@ en: branding: Branding description: Description discovery: Discovery + redirects: Aliases relationships: Relationships sharing: Sharing form_branding: @@ -1809,9 +1810,24 @@ en: in_collections: Collections in_other_works: This Work in Other Works in_this_work: Other Works in this Work + redirects: + caption: List of URL aliases for this resource + errors_heading: Please correct the following alias errors + header: + actions: Actions + path: Path + heading: URL Aliases + help_html: 'Register legacy URL paths that should redirect to this record. Each path will resolve from this site''s root. Reserved Hyrax routes (e.g. /concern, /dashboard) cannot be used.' + new_path_label: New alias path + path_label: Alias path + placeholder: + path: Enter a legacy path... + remove_label: Remove + remove_with_path_label: 'Remove %{path}' tab: files: Files metadata: Descriptions + redirects: Aliases relationships: Relationships share: Sharing visibility_until: until diff --git a/spec/forms/concerns/hyrax/redirects_field_behavior_spec.rb b/spec/forms/concerns/hyrax/redirects_field_behavior_spec.rb new file mode 100644 index 0000000000..0bb0020c05 --- /dev/null +++ b/spec/forms/concerns/hyrax/redirects_field_behavior_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::RedirectsFieldBehavior do + let(:form_class) do + Class.new do + attr_accessor :redirects + + def self.property(*); end + + def from_hash(params) + params + end + + def deserialize!(params) + from_hash(params) + end + + include Hyrax::RedirectsFieldBehavior + prepend Hyrax::RedirectsFieldBehavior + end + end + + let(:form) { form_class.new } + + before do + allow(Hyrax.config).to receive(:redirects_enabled?).and_return(true) + allow(Hyrax.config).to receive(:redirects_active?).and_return(true) + end + + describe '.included' do + let(:property_target) do + Class.new do + def self.property(*); end + end + end + + it 'is a no-op when the config gate is closed' do + allow(Hyrax.config).to receive(:redirects_enabled?).and_return(false) + expect(property_target).not_to receive(:property) + property_target.include(described_class) + end + + it 'registers the redirects_attributes virtual property when enabled' do + expect(property_target).to receive(:property).with( + :redirects_attributes, + virtual: true, + populator: :redirects_attributes_populator, + prepopulator: :redirects_attributes_prepopulator + ) + property_target.include(described_class) + end + end + + describe '#deserialize!' do + it 'strips the renamed redirects key so the populator owns the write' do + result = form.deserialize!('redirects' => 'overwrite-me', :redirects => 'also', 'title' => 'kept') + expect(result).to eq('title' => 'kept') + end + + it 'is a no-op on the renamed key when the feature is inactive' do + allow(Hyrax.config).to receive(:redirects_active?).and_return(false) + result = form.deserialize!('redirects' => 'kept', 'title' => 'kept') + expect(result).to eq('redirects' => 'kept', 'title' => 'kept') + end + end + + describe '#redirects_attributes_populator' do + it 'normalizes paths and produces the persisted hash shape' do + fragment = { + '0' => { 'path' => ' /foo/ ', 'canonical' => 'true' }, + '1' => { 'path' => '/bar', 'canonical' => 'false', 'sequence' => '5' } + } + form.send(:redirects_attributes_populator, fragment: fragment) + + expect(form.redirects).to eq([ + { 'path' => '/foo', 'canonical' => true, 'sequence' => 0 }, + { 'path' => '/bar', 'canonical' => false, 'sequence' => 5 } + ]) + end + + it 'drops rows marked for destruction' do + fragment = { + '0' => { 'path' => '/keep', 'canonical' => 'false' }, + '1' => { 'path' => '/drop', 'canonical' => 'false', '_destroy' => 'true' } + } + form.send(:redirects_attributes_populator, fragment: fragment) + + expect(form.redirects.map { |e| e['path'] }).to eq(['/keep']) + end + + it 'drops rows with a blank path' do + fragment = { + '0' => { 'path' => '/keep', 'canonical' => 'false' }, + '1' => { 'path' => ' ', 'canonical' => 'false' } + } + form.send(:redirects_attributes_populator, fragment: fragment) + + expect(form.redirects.map { |e| e['path'] }).to eq(['/keep']) + end + + it 'is a no-op when the feature is inactive' do + allow(Hyrax.config).to receive(:redirects_active?).and_return(false) + form.redirects = 'untouched' + form.send(:redirects_attributes_populator, fragment: { '0' => { 'path' => '/foo' } }) + + expect(form.redirects).to eq('untouched') + end + end + + describe '#redirects_attributes_prepopulator' do + it 'wraps each persisted hash entry in a Hyrax::Redirect presenter' do + form.redirects = [{ 'path' => '/foo', 'canonical' => true, 'sequence' => 0 }] + form.send(:redirects_attributes_prepopulator) + + expect(form.redirects.first).to be_a(Hyrax::Redirect) + expect(form.redirects.first.path).to eq('/foo') + expect(form.redirects.first.canonical).to be(true) + end + + it 'is a no-op when the feature is inactive' do + allow(Hyrax.config).to receive(:redirects_active?).and_return(false) + original = [{ 'path' => '/foo' }] + form.redirects = original + form.send(:redirects_attributes_prepopulator) + + expect(form.redirects).to be(original) + end + end +end diff --git a/spec/helpers/hyrax/redirects_tab_helper_spec.rb b/spec/helpers/hyrax/redirects_tab_helper_spec.rb new file mode 100644 index 0000000000..0c1ecae6f1 --- /dev/null +++ b/spec/helpers/hyrax/redirects_tab_helper_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::RedirectsTabHelper, type: :helper do + let(:helper_class) do + Class.new do + include Hyrax::RedirectsTabHelper + end + end + subject(:helper) { helper_class.new } + + let(:resource_with_redirects) { Struct.new(:redirects).new([]) } + let(:resource_without_redirects) { Object.new } + + describe '#redirects_tab?' do + before do + allow(Hyrax.config).to receive(:redirects_active?).and_return(true) + # The form-class gate is exercised by its own context below; stub it true + # for the other contexts so they isolate the redirects_active? and + # respond_to?(:redirects) checks. + allow(helper).to receive(:redirects_supported_form?).and_return(true) + end + + context 'when the redirects feature is inactive' do + before { allow(Hyrax.config).to receive(:redirects_active?).and_return(false) } + + it 'returns false' do + expect(helper.redirects_tab?(resource_with_redirects)).to be(false) + end + end + + context 'when the form is not a ResourceForm descendant' do + before { allow(helper).to receive(:redirects_supported_form?).and_return(false) } + + it 'returns false even when the resource carries redirects' do + expect(helper.redirects_tab?(resource_with_redirects)).to be(false) + end + end + + context 'when the resource carries the redirects attribute' do + it 'returns true' do + expect(helper.redirects_tab?(resource_with_redirects)).to be(true) + end + end + + context 'when the resource does not carry the redirects attribute' do + it 'returns false' do + expect(helper.redirects_tab?(resource_without_redirects)).to be(false) + end + end + + context 'when given a form-like wrapper that exposes #model' do + let(:form) do + Struct.new(:model).new(resource_with_redirects) + end + + it 'unwraps the form and inspects the underlying model' do + expect(helper.redirects_tab?(form)).to be(true) + end + + context 'when the wrapped model lacks the redirects attribute' do + let(:form) { Struct.new(:model).new(resource_without_redirects) } + + it 'returns false' do + expect(helper.redirects_tab?(form)).to be(false) + end + end + end + end + + describe '#redirects_supported_form?' do + it 'returns true for a Hyrax::Forms::ResourceForm instance' do + form = Hyrax::Forms::ResourceForm.new(Hyrax::Resource.new) + expect(helper.redirects_supported_form?(form)).to be(true) + end + + it 'returns false for an arbitrary object' do + expect(helper.redirects_supported_form?(Object.new)).to be(false) + end + end +end From b26e83ec4953c0aea096839b6199a3dde12fe41c Mon Sep 17 00:00:00 2001 From: LaRita Robinson Date: Mon, 4 May 2026 17:41:03 -0400 Subject: [PATCH 4/4] Document the Field Behavior pattern Adds documentation/forms/field_behaviors.md covering the Field Behavior pattern Hyrax uses for nested-attribute form properties. Explains the contract every behavior must satisfy (registration, deserialize! strip, populator, prepopulator, prepend wiring), the decision points when adding a new one, and uses BasedNearFieldBehavior and RedirectsFieldBehavior as worked examples. Cross-links from documentation/redirects.md and updates the Hyrax::Redirect description there (now a presenter, not a Valkyrie::Resource). Co-Authored-By: Claude Opus 4.7 (1M context) --- documentation/forms/field_behaviors.md | 271 +++++++++++++++++++++++++ documentation/redirects.md | 4 +- 2 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 documentation/forms/field_behaviors.md diff --git a/documentation/forms/field_behaviors.md b/documentation/forms/field_behaviors.md new file mode 100644 index 0000000000..956003155b --- /dev/null +++ b/documentation/forms/field_behaviors.md @@ -0,0 +1,271 @@ +# Field Behaviors + +A **Field Behavior** is a Ruby module mixed into `Hyrax::Forms::ResourceForm` (and its subclasses) that wires up a single property whose persisted shape and submitted-form shape differ. It owns the populator, prepopulator, and `deserialize!` strip for that property. + +Use a Field Behavior when you have a property that: + +- Is rendered through `accepts_nested_attributes_for` semantics (`_attributes` payload from the form). +- Persists in a shape the view can't render directly (a hash of sub-fields, a URI string the view wants as a `ControlledVocabulary` instance, a triple of values, etc.). +- Needs the same wiring on every form subclass. + +Two reference exemplars ship with Hyrax: + +- `Hyrax::BasedNearFieldBehavior` — single-string-per-entry (URI), hydrated to a `ControlledVocabularies::Location` for the view. +- `Hyrax::RedirectsFieldBehavior` — multi-field-per-entry (path / canonical / sequence), hydrated to a `Hyrax::Redirect` presenter for the view. + +This document covers the contract a Field Behavior must satisfy, the decision points for a new behavior, and the worked examples. + +## Why this pattern exists + +Reform's `FormBuilderMethods#deserialize!` rewrites the submitted `_attributes` key to `` before `from_hash` runs. If the form *also* has a property named `` (e.g. from an `include Hyrax::Schema(:foo)`), Reform's `from_hash` writes the raw fragment hash onto the property — bypassing the populator entirely. + +`deserialize!` is the right hook to fix this: by the time it runs, the rename has happened, but `from_hash` hasn't. A Field Behavior strips the renamed key from `params`, leaving the `_attributes` payload as the only entry point. The populator then owns the write. + +## The contract + +A Field Behavior is a module that: + +1. **Registers a virtual `_attributes` property** in `self.included`, with a populator and prepopulator. + + ```ruby + def self.included(descendant) + descendant.property :_attributes, + virtual: true, + populator: :_attributes_populator, + prepopulator: :_attributes_prepopulator + end + ``` + +2. **Composes via `super` in `deserialize!`**, then deletes its own renamed key. + + ```ruby + def deserialize!(params) + result = super + if result.respond_to?(:delete) + result.delete('') + result.delete(:) + end + result + end + ``` + + Calling `super` first lets every behavior in the chain run its own delete before control reaches Reform's base `deserialize!`. **Never** call `from_hash` yourself — that breaks composition for any behavior added later. + + Mutate `params` in place. Reform exposes the same hash via `form.input_params`; replacing it would orphan downstream readers. + +3. **Provides a populator** that reads the `_attributes` fragment, normalizes to the persisted shape, and assigns to the property. + + ```ruby + def _attributes_populator(fragment:, **_options) + return unless respond_to?(:) + # turn fragment into the persisted shape, then: + self. = entries + end + ``` + + Drop empty rows and rows marked `_destroy` here. Normalize values here too — the persisted shape should be canonical so non-form callers (importers, console, validators) don't have to re-normalize. + +4. **Provides a prepopulator** that hydrates the persisted value into a render-friendly object. + + ```ruby + def _attributes_prepopulator + return unless respond_to?(:) + self. = Array().map { |entry| MyPresenter.wrap(entry) } + end + ``` + +5. **Is `prepend`ed onto every subclass** in `ResourceForm.inherited`: + + ```ruby + class << self + def inherited(subclass) + subclass.prepend(MyFieldBehavior) + super + end + end + ``` + + `prepend` (not `include`) places the module's `deserialize!` *above* the subclass's own method on the ancestor chain, so it actually overrides. Each behavior gates itself internally and is a no-op when its property isn't on the subclass's model, so the unconditional prepend is safe. + +## Feature-gated behaviors + +If your behavior is tied to a feature flag (Flipflop, env config, etc.), gate **inside** the methods, not around the include / prepend: + +```ruby +def self.included(descendant) + return unless Hyrax.config.my_feature_enabled? + descendant.property ... +end + +def deserialize!(params) + result = super + if Hyrax.config.my_feature_active? && result.respond_to?(:delete) + result.delete('') + result.delete(:) + end + result +end + +def _attributes_populator(fragment:, **_options) + return unless respond_to?(:) + return unless Hyrax.config.my_feature_active? + ... +end +``` + +The `self.included` gate uses the structural config (does this Hyrax build know about the feature at all?). The runtime methods use the combined gate (config + Flipflop) so the behavior turns off when an admin flips the feature off without restarting. + +## Decision points + +When adding a Field Behavior, work through these: + +### 1. What does each entry look like? + +- **Single value per entry** (URI string, scalar): see `BasedNearFieldBehavior`. +- **Multiple sub-fields per entry** (path + canonical + sequence; or label + value + lang): see `RedirectsFieldBehavior`. + +### 2. Persisted as what? + +- **Plain string / scalar** — fine for single-value entries. +- **Plain hash** with string keys — for multi-field entries. Add the `hash` shortcut to your YAML schema (`type: hash, multiple: true`). Use this *instead* of nesting a Valkyrie::Resource subclass; nested resources round-trip badly through Postgres JSONB (sub-fields strip, parent fields leak). + +### 3. Does the view need a presenter? + +- **Yes** if the view calls `.something` accessors on each entry. Build a small Ruby class with `path` / `value` / etc. readers and a `wrap(input)` class method that accepts both raw and already-wrapped input. The prepopulator wraps every entry; the view can rely on the presenter API. +- **No** if the view reads keys directly. Skip the presenter and let the view call `entry['key']`. + +### 4. What happens to invalid input? + +The populator drops empty rows and `_destroy` rows. Format validation belongs in an `ActiveModel::EachValidator` invoked by the form's `validation` block — *not* in the populator. The populator's job is shape conversion; the validator's job is correctness. + +### 5. Is the property optional on some forms? + +Always guard with `respond_to?(:)` at the top of populator and prepopulator. Adopter forms whose models don't include the schema get a no-op rather than a crash. + +## Exemplar: `BasedNearFieldBehavior` + +```ruby +module Hyrax + module BasedNearFieldBehavior + def self.included(descendant) + descendant.property :based_near_attributes, + virtual: true, + populator: :based_near_attributes_populator, + prepopulator: :based_near_attributes_prepopulator + end + + def deserialize!(params) + result = super + if result.respond_to?(:delete) + result.delete('based_near') + result.delete(:based_near) + end + result + end + + private + + def based_near_attributes_populator(fragment:, **_options) + return unless respond_to?(:based_near) + adds, deletes = [], [] + fragment.each do |_, h| + uri = RDF::URI.parse(h["id"]).to_s + h["_destroy"] == "true" ? deletes << uri : adds << uri + end + self.based_near = ((model.based_near + adds) - deletes).uniq + end + + def based_near_attributes_prepopulator + return unless respond_to?(:based_near) + self.based_near = based_near&.map { |loc| Hyrax::ControlledVocabularies::Location.new(RDF::URI.parse(loc)) } + self.based_near ||= [] + self.based_near << Hyrax::ControlledVocabularies::Location.new if self.based_near.blank? + end + end +end +``` + +- **Persisted shape:** array of URI strings. +- **View-side shape:** array of `Hyrax::ControlledVocabularies::Location` instances. +- **Diff from a plain `_attributes` setup:** `deserialize!` strips `based_near` after the rename so `from_hash` doesn't overwrite the property with raw fragment hashes; the populator merges adds/removes onto the existing `model.based_near` so partial form submissions are non-destructive. + +## Exemplar: `RedirectsFieldBehavior` + +```ruby +module Hyrax + module RedirectsFieldBehavior + def self.included(descendant) + return unless Hyrax.config.redirects_enabled? + descendant.property :redirects_attributes, + virtual: true, + populator: :redirects_attributes_populator, + prepopulator: :redirects_attributes_prepopulator + end + + def deserialize!(params) + result = super + if Hyrax.config.redirects_active? && result.respond_to?(:delete) + result.delete('redirects') + result.delete(:redirects) + end + result + end + + private + + def redirects_attributes_populator(fragment:, **_options) + return unless respond_to?(:redirects) + return unless Hyrax.config.redirects_active? + entries = Array(fragment&.values) + .reject { |row| row['_destroy'].to_s == 'true' || row['path'].to_s.strip.empty? } + .each_with_index.map do |row, i| + { 'path' => Hyrax::RedirectPathNormalizer.call(row['path']), + 'canonical' => row['canonical'].to_s == 'true', + 'sequence' => row['sequence'].presence&.to_i || i } + end + self.redirects = entries + end + + def redirects_attributes_prepopulator + return unless respond_to?(:redirects) + return unless Hyrax.config.redirects_active? + self.redirects = Array(redirects).map { |entry| Hyrax::Redirect.wrap(entry) } + end + end +end +``` + +- **Persisted shape:** array of plain hashes (`'path'`, `'canonical'`, `'sequence'`). Declared with `type: hash, multiple: true` in the YAML schema. +- **View-side shape:** array of `Hyrax::Redirect` presenters, exposing `.path` / `.canonical` / `.sequence`. +- **Diff from BasedNear:** entries carry multiple sub-fields, so the persisted shape is a hash rather than a string. The populator normalizes paths up front (canonical form lives in storage). The behavior is feature-gated — every callback consults `Hyrax.config.redirects_active?`. + +## Wiring on `ResourceForm` + +```ruby +module Hyrax + module Forms + class ResourceForm < Reform::Form + include BasedNearFieldBehavior + include RedirectsFieldBehavior + + class << self + def inherited(subclass) + subclass.prepend(BasedNearFieldBehavior) + subclass.prepend(RedirectsFieldBehavior) + super + end + end + end + end +end +``` + +Both behaviors compose. A subclass's ancestor chain ends up with both behaviors' `deserialize!` methods above Reform's base method; each runs its `super` then strips its own renamed key. + +## Common pitfalls + +- **Calling `from_hash` from inside `deserialize!`** — terminal. Breaks composition for any other behavior on the same form. Always call `super` and let Reform's base method do the rename + `from_hash`. +- **`include`-ing the behavior on subclasses** — the `included` callback runs but the `deserialize!` override doesn't take effect. Use `prepend` on subclasses so the override lands above the inherited method. +- **Conditional `prepend`** (`subclass.prepend(MyBehavior) if condition`) — the condition is evaluated at class-load time and never re-evaluated. Use unconditional `prepend` and gate inside the runtime methods. +- **Forgetting `respond_to?` guards** — adopter forms whose models don't include the schema raise `NoMethodError` instead of being a clean no-op. +- **Normalizing on read** — the persisted shape should be canonical so every consumer (validator, indexer, sync, importer) can compare without re-normalizing. Normalize in the populator (the form's write site) and add normalization to non-form write paths separately. diff --git a/documentation/redirects.md b/documentation/redirects.md index 3638361165..dd93144892 100644 --- a/documentation/redirects.md +++ b/documentation/redirects.md @@ -278,7 +278,9 @@ Alternatively, gate the inclusion of the calling code itself on `Hyrax.config.re ## See also - `documentation/flexible_metadata.md` — m3 profile fundamentals and how the redirects feature interacts with flexible metadata. -- `Hyrax::Redirect` (`app/models/hyrax/redirect.rb`) — the Valkyrie::Resource representing a single redirect entry. +- `documentation/forms/field_behaviors.md` — the Field Behavior pattern used by `Hyrax::RedirectsFieldBehavior` to wire the form's nested-attribute property. +- `Hyrax::Redirect` (`app/models/hyrax/redirect.rb`) — thin Ruby presenter for a single redirect entry; used on the form's render path. +- `Hyrax::RedirectsFieldBehavior` (`app/forms/concerns/hyrax/redirects_field_behavior.rb`) — form-side populator/prepopulator and the `deserialize!` strip for the `redirects` property. - `Hyrax::Indexers::RedirectsIndexer` (`app/indexers/hyrax/indexers/redirects_indexer.rb`) — the indexer mixin. - `Hyrax::RedirectsController` (`app/controllers/hyrax/redirects_controller.rb`) — the redirect resolver. - `Hyrax::FlexibleSchemaValidators::RedirectsValidator` (`app/services/hyrax/flexible_schema_validators/redirects_validator.rb`) — the m3 profile validator.