From 1e321bb5c900c885af20c20f56a0913f2aae4ce0 Mon Sep 17 00:00:00 2001 From: Nathan Flood Date: Mon, 10 Aug 2026 15:32:44 +0000 Subject: [PATCH] fix: serialize range fields as bracket literals, symmetric with writes The 7 range fields on PlantType/VarietyType (nAccumulationRange, biomassProductionRange, optimalTemperatureRange, optimalRainfallRange, seasonalityDaysRange, optimalAltitudeRange, phRange) were rendering through graphql-ruby's default String coercion, i.e. Ruby's Range#to_s ("5.5..7.0", "600...1201", "0...Infinity" against prod). Writes accept Postgres-style bracket literals ("[min,max]"); reads emitted Ruby Range syntax. The admin SPA's range parser only understands bracket literals, so every existing value rendered as a blank input. Add RangeLiteral (app/services/range_literal.rb), a pure serializer that mirrors the write-side format. Integer ranges (int4range) round-trip through Postgres canonicalized to an inclusive-lower/exclusive-upper Ruby Range, so a finite exclusive upper bound is decremented back to the inclusive value an editor actually typed. Numeric ranges (numrange) preserve whatever inclusivity was written, formatted via BigDecimal#to_s('F') to avoid engineering notation, with a trailing ".0" stripped. Unbounded bounds (nil or infinite) render as an empty side, always closed with "]". Wire it into both types via Types::Concerns::RangeLiteralFields, which define_method's the 7 resolvers from the single field list already declared in Mutations::Concerns::RangeLiteralValidation::RANGE_FIELDS. Also fix RecordDraftInfoType#author/#last_editor: Principal#display_name is nil for principals created by resolve_actor's upsert (it never passes one), so the field rendered "Unknown" for the common case. Falls back to email, which editors (the only audience -- draft is update?-gated) can already see via ownedBy/createdBy. Full RSpec suite green (2324 examples), including mobile contracts (spec/contracts -- zero range assertions there, confirmed by the run), and rubocop clean. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RnYLUscETuPwsQzU9nCSxu --- .../types/concerns/range_literal_fields.rb | 29 +++++ app/graphql/types/plant_type.rb | 1 + app/graphql/types/record_draft_info_type.rb | 16 ++- app/graphql/types/variety_type.rb | 1 + app/services/range_literal.rb | 90 ++++++++++++++ schema.graphql | 4 +- .../range_literal_round_trip_spec.rb | 113 ++++++++++++++++++ spec/services/range_literal_spec.rb | 79 ++++++++++++ spec/types/record_draft_info_type_spec.rb | 58 +++++++++ 9 files changed, 385 insertions(+), 6 deletions(-) create mode 100644 app/graphql/types/concerns/range_literal_fields.rb create mode 100644 app/services/range_literal.rb create mode 100644 spec/mutations/range_literal_round_trip_spec.rb create mode 100644 spec/services/range_literal_spec.rb create mode 100644 spec/types/record_draft_info_type_spec.rb diff --git a/app/graphql/types/concerns/range_literal_fields.rb b/app/graphql/types/concerns/range_literal_fields.rb new file mode 100644 index 0000000..7c3b906 --- /dev/null +++ b/app/graphql/types/concerns/range_literal_fields.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Types + module Concerns + # Resolver methods for the 7 numeric-range fields shared by PlantType and + # VarietyType. The `field :x_range, String, ...` declarations already + # exist on each including type; this concern only supplies the resolver + # methods, so every range field renders through RangeLiteral.serialize -- + # the same canonical bracket-literal format + # (Mutations::Concerns::RangeLiteralValidation) that create/update + # mutations accept -- instead of graphql-ruby's default String + # coercion, which falls back to Ruby's Range#to_s ("5.5..7.0", + # "0...Infinity"). + # + # Sources the field list from RangeLiteralValidation::RANGE_FIELDS so + # there is exactly one list of "the 7 range fields" in the codebase. + module RangeLiteralFields + extend ActiveSupport::Concern + + included do + Mutations::Concerns::RangeLiteralValidation::RANGE_FIELDS.each do |field| + define_method(field) do + RangeLiteral.serialize(object.public_send(field)) + end + end + end + end + end +end diff --git a/app/graphql/types/plant_type.rb b/app/graphql/types/plant_type.rb index 2ff6a6d..449335e 100644 --- a/app/graphql/types/plant_type.rb +++ b/app/graphql/types/plant_type.rb @@ -8,6 +8,7 @@ class PlantType < Types::BaseObject # rubocop:disable Metrics/ClassLength include Types::Concerns::CapabilityFields include Types::Concerns::DraftFields + include Types::Concerns::RangeLiteralFields description 'A plant is a crop species available through the Plant API.' diff --git a/app/graphql/types/record_draft_info_type.rb b/app/graphql/types/record_draft_info_type.rb index 39f3db6..f0e54a2 100644 --- a/app/graphql/types/record_draft_info_type.rb +++ b/app/graphql/types/record_draft_info_type.rb @@ -5,9 +5,11 @@ module Types class RecordDraftInfoType < Types::BaseObject field :updated_at, GraphQL::Types::ISO8601DateTime, null: false field :author, String, null: true, - description: 'Display name of the principal who started this draft.' + description: 'Display name of the principal who started this draft, falling back to ' \ + 'their email when no display name is on file.' field :last_editor, String, null: true, - description: 'Display name of the principal who last edited it.' + description: 'Display name of the principal who last edited it, falling back to ' \ + 'their email when no display name is on file.' field :changed_fields, [String], null: false, description: 'Attribute names this draft changes.' field :is_stale, Boolean, null: false, @@ -16,12 +18,18 @@ class RecordDraftInfoType < Types::BaseObject 'every resolution -- avoid requesting it across large lists; batching it ' \ 'is a tracked follow-up.' + # display_name is not always populated: resolve_actor (application_controller.rb) + # never passes it when it upserts a Principal from a JWT claim, so it is nil for + # most real users today and this field would otherwise render "Unknown" client + # side. Fall back to email, which this type's callers (editors, gated by + # DraftFields#draft requiring update?) already see elsewhere on the record via + # ownedBy/createdBy, so this is no new exposure. def author - object.author&.display_name + object.author&.display_name.presence || object.author&.email end def last_editor - object.last_editor&.display_name + object.last_editor&.display_name.presence || object.last_editor&.email end def is_stale diff --git a/app/graphql/types/variety_type.rb b/app/graphql/types/variety_type.rb index 2ab47e6..feba800 100644 --- a/app/graphql/types/variety_type.rb +++ b/app/graphql/types/variety_type.rb @@ -8,6 +8,7 @@ class VarietyType < Types::BaseObject # rubocop:disable Metrics/ClassLength include Types::Concerns::CapabilityFields include Types::Concerns::DraftFields + include Types::Concerns::RangeLiteralFields description 'A variety represents a more precisely defined subgroup of plants with a common set of characteristics.' diff --git a/app/services/range_literal.rb b/app/services/range_literal.rb new file mode 100644 index 0000000..7b57dd3 --- /dev/null +++ b/app/services/range_literal.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +# Serializes an ActiveRecord Range attribute (backed by a Postgres int4range +# or numrange column) back into the bracket-literal string format the write +# side (Mutations::Concerns::RangeLiteralValidation::RANGE_LITERAL) accepts, +# e.g. "[10,20]", "[10,]", "[,100]". Symmetric with writes: what an editor +# typed on create/update is what they see again on read. +# +# Before this existed, the 7 range fields on PlantType/VarietyType rendered +# through graphql-ruby's default String coercion, i.e. Ruby's Range#to_s -- +# "5.5..7.0", "600...1201", "0...Infinity" in production. Writes accepted +# bracket literals; reads emitted Ruby Range syntax. The admin SPA's range +# parser (and the documented write format) only understands bracket +# literals, so every existing value rendered as a blank input. +# +# This module is stateless -- all methods are module functions. +module RangeLiteral + module_function + + # range - a Ruby Range as ActiveRecord's PostgreSQL::OID::Range casts it + # (nil endpoints from an unbounded side are represented as + # Float::INFINITY / -Float::INFINITY, never literal nil, but nil is + # tolerated defensively). Returns nil for a nil range, otherwise a bracket + # literal string. + def serialize(range) + return nil if range.nil? + + lower = range.begin + upper = range.end + exclude_end = range.exclude_end? + upper_unbounded = unbounded?(upper) + + # Postgres canonicalizes discrete ranges (int4range) to an inclusive + # lower / exclusive upper bound -- an editor who wrote the inclusive + # literal "[500,2000]" gets back the Ruby Range 500...2001 once the row + # round-trips through the database. Numeric ranges (numrange) are + # continuous, so Postgres stores exactly the inclusivity it was given + # and this branch never fires for them. + if integer_range?(lower, upper) && !upper_unbounded && exclude_end + upper -= 1 + exclude_end = false + end + + # An unbounded upper bound always renders as an empty side with a + # closing "]" ("[10,]"), matching the literal the write side expects for + # "no upper limit" -- never "[10,)", which would read as a real + # (if oddly punctuated) exclusive bound rather than "unset". + upper_bracket = upper_unbounded || !exclude_end ? ']' : ')' + + "[#{format_bound(lower)},#{format_bound(upper)}#{upper_bracket}" + end + + # A Range's lower bound is always inclusive here: Ruby's Range class has + # no way to represent an open lower bound (assigning a Postgres literal + # with an open lower bound, e.g. "(5,10]", raises ArgumentError at cast + # time), so the opening bracket is always "[". + def unbounded?(bound) + bound.nil? || (bound.respond_to?(:infinite?) && bound.infinite?) + end + private_class_method :unbounded? + + # int4range bounds cast to Integer; numrange bounds cast to BigDecimal. + # Either bound being an Integer is enough to identify a discrete range, + # since an unbounded side casts to Float::INFINITY regardless of subtype. + def integer_range?(lower, upper) + lower.is_a?(Integer) || upper.is_a?(Integer) + end + private_class_method :integer_range? + + def format_bound(bound) + return '' if unbounded?(bound) + return bound.to_s if bound.is_a?(Integer) + return format_big_decimal(bound) if bound.is_a?(BigDecimal) + + bound.to_s + end + private_class_method :format_bound + + # BigDecimal#to_s defaults to scientific notation ("0.55e1" for 5.5), which + # is not a valid range-literal bound. #to_s('F') gives fixed-point + # notation instead. Formatting choice: a trailing ".0" is stripped, so a + # whole-number bound like 7.0 renders as "7" (matching what an editor + # would type for a whole number), while a fractional bound like 5.5 is + # left untouched. The round-trip spec asserts this exact choice. + def format_big_decimal(bound) + str = bound.to_s('F') + str.end_with?('.0') ? str.delete_suffix('.0') : str + end + private_class_method :format_big_decimal +end diff --git a/schema.graphql b/schema.graphql index 4c4e692..30fc035 100644 --- a/schema.graphql +++ b/schema.graphql @@ -7375,7 +7375,7 @@ type Query { type RecordDraftInfo { """ - Display name of the principal who started this draft. + Display name of the principal who started this draft, falling back to their email when no display name is on file. """ author: String @@ -7392,7 +7392,7 @@ type RecordDraftInfo { isStale: Boolean! """ - Display name of the principal who last edited it. + Display name of the principal who last edited it, falling back to their email when no display name is on file. """ lastEditor: String updatedAt: ISO8601DateTime! diff --git a/spec/mutations/range_literal_round_trip_spec.rb b/spec/mutations/range_literal_round_trip_spec.rb new file mode 100644 index 0000000..dfa85e7 --- /dev/null +++ b/spec/mutations/range_literal_round_trip_spec.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Reads of the 7 range fields now go through RangeLiteral.serialize +# (app/graphql/types/concerns/range_literal_fields.rb), so what a mutation +# accepts is what a subsequent query returns -- previously reads fell back to +# graphql-ruby's default String coercion, i.e. Ruby's Range#to_s +# ("5.5..7.0", "0...Infinity"), which the admin SPA's range parser cannot +# read back into its inputs. +RSpec.describe 'range literal read/write symmetry', type: :graphql_mutation do + before :each do + Mobility.locale = nil + end + + let(:current_user) { build(:user, :readwrite) } + let!(:plant) { create(:plant, owned_by: current_user.email, created_by: current_user.email) } + let(:plant_gid) { PlantApiSchema.id_from_object(plant, Plant, {}) } + + def update(input) + query = <<~GRAPHQL + mutation($input: UpdatePlantInput!) { + updatePlant(input: $input) { + errors { field message code } + plant { uuid } + } + } + GRAPHQL + PlantApiSchema.execute(query, context: { current_user: current_user }, + variables: { input: { plantId: plant_gid }.merge(input) }) + end + + def read(perspective: 'PUBLISHED') + query = <<~GRAPHQL + query($id: ID!, $perspective: Perspective) { + plant(id: $id, perspective: $perspective) { + nAccumulationRange + phRange + } + } + GRAPHQL + PlantApiSchema.execute(query, context: { current_user: current_user }, + variables: { id: plant_gid, perspective: perspective }) + .dig('data', 'plant') + end + + it 'round-trips an unbounded-upper integer literal' do + result = update(nAccumulationRange: '[10,]') + expect(result.dig('data', 'updatePlant', 'errors')).to eq([]) + + expect(read['nAccumulationRange']).to eq '[10,]' + end + + it 'round-trips a finite inclusive integer literal, decrementing back from ' \ + "Postgres's canonicalized exclusive-upper storage" do + result = update(nAccumulationRange: '[500,2000]') + expect(result.dig('data', 'updatePlant', 'errors')).to eq([]) + + # Confirms the canonicalization this spec exists to guard against: what + # is actually stored is the exclusive-upper form, not what was typed. + expect(plant.reload.n_accumulation_range).to eq(500...2001) + expect(read['nAccumulationRange']).to eq '[500,2000]' + end + + it 'round-trips a numrange literal per the documented BigDecimal formatting choice ' \ + '(trailing .0 stripped)' do + result = update(phRange: '[5.5,7.0]') + expect(result.dig('data', 'updatePlant', 'errors')).to eq([]) + + expect(read['phRange']).to eq '[5.5,7]' + end + + describe 'DRAFT perspective' do + it 'returns the bracket literal for a staged (never-persisted-to-Postgres) range value' do + result = update(nAccumulationRange: '[500,2000]', saveAsDraft: true) + expect(result.dig('data', 'updatePlant', 'errors')).to eq([]) + + # The live row is untouched; only the draft carries the staged value. + expect(plant.reload.n_accumulation_range).not_to eq(500...2001) + + expect(read(perspective: 'DRAFT')['nAccumulationRange']).to eq '[500,2000]' + end + end + + describe 'VarietyType' do + let!(:variety) { create(:variety, owned_by: current_user.email, created_by: current_user.email) } + let(:variety_gid) { PlantApiSchema.id_from_object(variety, Variety, {}) } + + it 'serializes range fields the same way as PlantType' do + mutation = <<~GRAPHQL + mutation($input: UpdateVarietyInput!) { + updateVariety(input: $input) { + errors { field message code } + variety { uuid } + } + } + GRAPHQL + result = PlantApiSchema.execute(mutation, context: { current_user: current_user }, + variables: { input: { varietyId: variety_gid, + optimalAltitudeRange: '[500,2000]' } }) + expect(result.dig('data', 'updateVariety', 'errors')).to eq([]) + + query = <<~GRAPHQL + query($id: ID!) { + variety(id: $id) { optimalAltitudeRange } + } + GRAPHQL + result = PlantApiSchema.execute(query, context: { current_user: current_user }, + variables: { id: variety_gid }) + expect(result.dig('data', 'variety', 'optimalAltitudeRange')).to eq '[500,2000]' + end + end +end diff --git a/spec/services/range_literal_spec.rb b/spec/services/range_literal_spec.rb new file mode 100644 index 0000000..5fbe5cb --- /dev/null +++ b/spec/services/range_literal_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe RangeLiteral, type: :service do + describe '.serialize' do + it 'returns nil for a nil range' do + expect(described_class.serialize(nil)).to be_nil + end + + context 'integer ranges (int4range)' do + it 'decrements a finite exclusive upper bound to the inclusive equivalent' do + # Postgres canonicalizes int4range to inclusive-lower/exclusive-upper, + # so an editor's "[500,2000]" round-trips through the database as the + # Ruby Range 500...2001. + range = Range.new(500, 2001, true) + expect(described_class.serialize(range)).to eq '[500,2000]' + end + + it 'renders an unbounded upper as an empty side closed with ]' do + range = Range.new(10, Float::INFINITY, true) + expect(described_class.serialize(range)).to eq '[10,]' + end + + it 'renders an unbounded lower as an empty side' do + range = Range.new(-Float::INFINITY, 101, true) + expect(described_class.serialize(range)).to eq '[,100]' + end + + it 'renders a fully unbounded range as [,]' do + range = Range.new(-Float::INFINITY, Float::INFINITY, true) + expect(described_class.serialize(range)).to eq '[,]' + end + + it 'handles negative bounds' do + range = Range.new(-10, 5, true) + expect(described_class.serialize(range)).to eq '[-10,4]' + end + end + + context 'numeric ranges (numrange)' do + it 'formats inclusive BigDecimal bounds without scientific notation' do + range = Range.new(BigDecimal('5.5'), BigDecimal('7.0'), false) + expect(described_class.serialize(range)).to eq '[5.5,7]' + end + + it 'strips a trailing .0 from a whole-number BigDecimal bound' do + range = Range.new(BigDecimal('0.0'), BigDecimal('14.0'), false) + expect(described_class.serialize(range)).to eq '[0,14]' + end + + it 'does not touch a fractional bound that does not end in .0' do + range = Range.new(BigDecimal('1.25'), BigDecimal('3.75'), false) + expect(described_class.serialize(range)).to eq '[1.25,3.75]' + end + + it 'preserves an exclusive upper bound (rare, only via raw data) as a paren form' do + range = Range.new(BigDecimal('1.0'), BigDecimal('2.0'), true) + expect(described_class.serialize(range)).to eq '[1,2)' + end + + it 'renders a fully unbounded numeric range as [,]' do + range = Range.new(-Float::INFINITY, Float::INFINITY, true) + expect(described_class.serialize(range)).to eq '[,]' + end + + it 'never emits BigDecimal engineering notation for the bounds' do + # BigDecimal#inspect (and, on some bigdecimal versions, plain #to_s) + # renders engineering notation, e.g. BigDecimal('5.5').inspect == + # "0.55e1". That is not a valid range-literal bound, so the + # serializer must always go through #to_s('F'). Guard against a + # future edit accidentally swapping back to plain #to_s/#inspect. + expect(BigDecimal('5.5').inspect).to include 'e' + range = Range.new(BigDecimal('5.5'), BigDecimal('7.0'), false) + expect(described_class.serialize(range)).not_to include 'e' + end + end + end +end diff --git a/spec/types/record_draft_info_type_spec.rb b/spec/types/record_draft_info_type_spec.rb new file mode 100644 index 0000000..699c083 --- /dev/null +++ b/spec/types/record_draft_info_type_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# RecordDraftInfoType#author/#last_editor fall back to the principal's email +# when display_name is nil. Most real principals today have no display_name: +# Principal.resolve! (called from application_controller.rb#resolve_actor) is +# never passed one, so the un-fallback'd field rendered "Unknown" for the +# common case. +RSpec.describe 'RecordDraftInfoType author/lastEditor display_name fallback', type: :graphql_query do + let(:user) { build(:user, :superadmin) } + let(:plant) { create(:plant, scientific_name: 'Live name') } + let(:query) do + <<~GRAPHQL + query($id: ID!) { + plant(id: $id) { + draft { author lastEditor } + } + } + GRAPHQL + end + + def run + vars = { id: PlantApiSchema.id_from_object(plant, Plant, {}) } + PlantApiSchema.execute(query, context: { current_user: user }, variables: vars) + .dig('data', 'plant', 'draft') + end + + it 'uses display_name when present' do + principal = create(:principal, display_name: 'Jo Author', email: 'jo@example.com') + create(:record_draft, draftable: plant, data: { 'scientific_name' => 'Draft name' }, + author_principal_id: principal.id, last_editor_principal_id: principal.id) + + draft = run + expect(draft['author']).to eq 'Jo Author' + expect(draft['lastEditor']).to eq 'Jo Author' + end + + it 'falls back to email when display_name is nil (the resolve_actor shape)' do + principal = create(:principal, display_name: nil, email: 'no-name@example.com') + create(:record_draft, draftable: plant, data: { 'scientific_name' => 'Draft name' }, + author_principal_id: principal.id, last_editor_principal_id: principal.id) + + draft = run + expect(draft['author']).to eq 'no-name@example.com' + expect(draft['lastEditor']).to eq 'no-name@example.com' + end + + it 'falls back to email when display_name is an empty string' do + principal = create(:principal, display_name: '', email: 'blank-name@example.com') + create(:record_draft, draftable: plant, data: { 'scientific_name' => 'Draft name' }, + author_principal_id: principal.id, last_editor_principal_id: principal.id) + + draft = run + expect(draft['author']).to eq 'blank-name@example.com' + expect(draft['lastEditor']).to eq 'blank-name@example.com' + end +end