From b2fc07ac71c5f364015eaf647fcf248bdc064e25 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 14 Aug 2026 16:04:54 +0200 Subject: [PATCH 1/6] add group_bys column to persisted_queries --- ...0153_add_group_bys_to_persisted_queries.rb | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 db/migrate/20260814140153_add_group_bys_to_persisted_queries.rb diff --git a/db/migrate/20260814140153_add_group_bys_to_persisted_queries.rb b/db/migrate/20260814140153_add_group_bys_to_persisted_queries.rb new file mode 100644 index 000000000000..bed340ef76e8 --- /dev/null +++ b/db/migrate/20260814140153_add_group_bys_to_persisted_queries.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class AddGroupBysToPersistedQueries < ActiveRecord::Migration[8.1] + def change + add_column :persisted_queries, :group_bys, :jsonb, default: [] + end +end From 44ff989c55960a6f1c31dd4d95a2bc6629a83dc5 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 14 Aug 2026 16:18:00 +0200 Subject: [PATCH 2/6] Allow registering group_bys for persisted queries and ensure that not adding any does not break --- app/models/persisted_query.rb | 1 + app/models/queries/register.rb | 52 +++++++++--------- app/models/queries/serialization/group_bys.rb | 55 +++++++++++++++++++ 3 files changed, 81 insertions(+), 27 deletions(-) create mode 100644 app/models/queries/serialization/group_bys.rb diff --git a/app/models/persisted_query.rb b/app/models/persisted_query.rb index 5254df8898b5..26458d566043 100644 --- a/app/models/persisted_query.rb +++ b/app/models/persisted_query.rb @@ -53,6 +53,7 @@ def self.inherited(subclass) subclass.serialize :filters, coder: Queries::Serialization::Filters.new(subclass) subclass.serialize :orders, coder: Queries::Serialization::Orders.new(subclass) subclass.serialize :selects, coder: Queries::Serialization::Selects.new(subclass) + subclass.serialize :group_bys, coder: Queries::Serialization::GroupBys.new(subclass) end def self.register_query(&) diff --git a/app/models/queries/register.rb b/app/models/queries/register.rb index a93290fbea13..fb5e96198bee 100644 --- a/app/models/queries/register.rb +++ b/app/models/queries/register.rb @@ -31,52 +31,50 @@ module Queries::Register class << self def filter(query, filter) - @filters ||= Hash.new do |hash, filter_key| - hash[filter_key] = [] - end - - @filters[query] << filter + filters[query] << filter end # Exclude filter from filters collection representer. def exclude(filter) - @excluded_filters ||= [] - @excluded_filters << filter + excluded_filters << filter end def order(query, order) - @orders ||= Hash.new do |hash, order_key| - hash[order_key] = [] - end - - @orders[query] << order + orders[query] << order end def group_by(query, group_by) - @group_bys ||= Hash.new do |hash, group_key| - hash[group_key] = [] - end - - @group_bys[query] << group_by + group_bys[query] << group_by end def select(query, select) - @selects ||= Hash.new do |hash, select_key| - hash[select_key] = [] - end - - @selects[query] << select + selects[query] << select end def register(query, &) Registration.new(query).instance_exec(&) end - attr_accessor :filters, - :excluded_filters, - :orders, - :selects, - :group_bys + # A query class registering none of a given kind is normal - most notably + # group_bys, which only a handful of queries declare - so these must return + # an empty registry rather than nil. + def filters = @filters ||= registry + def orders = @orders ||= registry + def selects = @selects ||= registry + def group_bys = @group_bys ||= registry + def excluded_filters = @excluded_filters ||= [] + + attr_writer :filters, + :excluded_filters, + :orders, + :selects, + :group_bys + + private + + def registry + Hash.new { |hash, key| hash[key] = [] } + end end class Registration diff --git a/app/models/queries/serialization/group_bys.rb b/app/models/queries/serialization/group_bys.rb new file mode 100644 index 000000000000..261e94f41831 --- /dev/null +++ b/app/models/queries/serialization/group_bys.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +# -- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +# ++ + +class Queries::Serialization::GroupBys + include Queries::GroupBys::AvailableGroupBys + + def load(serialized_group_bys) + return [] if serialized_group_bys.nil? + + serialized_group_bys.map do |group_by| + group_by_for(group_by.to_sym) + end + end + + def dump(group_bys) + group_bys.map { |group_by| group_by.attribute.to_s } + end + + def group_by_register + ::Queries::Register.group_bys[klass] + end + + def initialize(klass) + @klass = klass + end + + attr_reader :klass +end From 9d0ed96d07b786c26f53bcb22b079c1839fdc5f8 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 14 Aug 2026 16:32:58 +0200 Subject: [PATCH 3/6] Allow the BaseQuery to handle multiple group_by entries --- app/models/queries/base_query.rb | 55 +++++++++++++++++-------- app/models/queries/unpersisted_query.rb | 6 +-- 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/app/models/queries/base_query.rb b/app/models/queries/base_query.rb index e5e6ca481c06..4344cc29e7d9 100644 --- a/app/models/queries/base_query.rb +++ b/app/models/queries/base_query.rb @@ -41,7 +41,7 @@ module Queries::BaseQuery validate :filters_valid, :sortation_valid - validate :group_by_valid, if: -> { respond_to?(:group_by) } + validate :group_bys_valid, if: -> { respond_to?(:group_bys) } end class_methods do @@ -72,15 +72,20 @@ def results end def groups - return nil if group_by.nil? + return nil if group_bys.empty? return empty_scope unless valid? apply_group_by(apply_filters(default_scope)) - .select(group_by.name, Arel.sql("COUNT(*)")) + .select(*group_by_names, Arel.sql("COUNT(*)")) end + # Keys are the plain value when grouping by a single attribute, and an array + # of values - one per group by - when grouping by several. def group_values - groups_hash = groups.pluck(group_by.name, Arel.sql("COUNT(*)")).to_h + groups_hash = groups.pluck(*group_by_names, Arel.sql("COUNT(*)")).to_h do |*values, count| + [group_bys.one? ? values.first : values, count] + end + instantiate_group_keys groups_hash end @@ -123,8 +128,8 @@ def order(hash) self end - def group(attribute) - self.group_by = group_by_for(attribute) + def group(*attributes) + self.group_bys = attributes.map { |attribute| group_by_for(attribute) } self end @@ -163,10 +168,12 @@ def sortation_valid end end - def group_by_valid - return if group_by.nil? || group_by.valid? + def group_bys_valid + group_bys.each do |group_by| + next if group_by.valid? - add_error(:group_by, group_by.name, group_by) + add_error(:group_by, group_by.name, group_by) + end end def add_error(local_attribute, attribute_name, object) @@ -207,30 +214,42 @@ def apply_orders(query_scope) end def apply_group_by(query_scope) - return query_scope if group_by.nil? + return query_scope if group_bys.empty? - group_by.apply_to(query_scope) - .order(group_by.name) + group_bys + .inject(query_scope) { |scope, group_by| group_by.apply_to(scope) } + .order(*group_by_names) + end + + def group_by_names + group_bys.map(&:name) end def build_orders - return orders if !respond_to?(:group_by) || group_by.nil? || has_group_by_order? + return orders unless respond_to?(:group_bys) + + group_by_orders + orders + end - [group_by_order] + orders + def group_by_orders + group_bys + .reject { |group_by| ordered_by?(group_by) } + .map { |group_by| group_by_order(group_by) } end - def has_group_by_order? - !!group_by && orders.detect { |order| order.class.key == group_by.order_key } + def ordered_by?(group_by) + orders.any? { |order| order.class.key == group_by.order_key } end - def group_by_order + def group_by_order(group_by) order_for(group_by.order_key).tap do |order| order.direction = :asc end end def instantiate_group_keys(groups) - return groups unless group_by&.association_class + group_by = group_bys.first + return groups unless group_bys.one? && group_by.association_class ar_keys = group_by.association_class.where(id: groups.keys.compact) diff --git a/app/models/queries/unpersisted_query.rb b/app/models/queries/unpersisted_query.rb index 0b768162f808..3ff1d299c540 100644 --- a/app/models/queries/unpersisted_query.rb +++ b/app/models/queries/unpersisted_query.rb @@ -34,18 +34,18 @@ module Queries::UnpersistedQuery included do attr_accessor :filters, :orders - attr_reader :group_by + attr_reader :group_bys def initialize(*args) @filters = [] @orders = [] - @group_by = nil + @group_bys = [] @user = args.first[:user] if args&.first end protected attr_accessor :user - attr_writer :group_by + attr_writer :group_bys end end From 2cfb6c4c47296edbdf9c5ae6212641cc224d0332 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 17 Aug 2026 10:26:20 +0200 Subject: [PATCH 4/6] Use group_bys in serialization --- app/models/queries/serialization/hash.rb | 4 ++-- lib/api/v3/utilities/endpoints/index.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/queries/serialization/hash.rb b/app/models/queries/serialization/hash.rb index 5fedad522eae..b636bcfa6575 100644 --- a/app/models/queries/serialization/hash.rb +++ b/app/models/queries/serialization/hash.rb @@ -39,7 +39,7 @@ def from_hash(hash) # rubocop:disable Metrics/AbcSize query.name = hash[:name] if hash[:name].present? query.add_filters hash[:filters] if hash[:filters].present? query.add_orders hash[:orders] if hash[:orders].present? - query.group hash[:group_by] if hash[:group_by].present? + query.group(*hash[:group_bys]) if hash[:group_bys].present? query.select(*hash[:selects]) if hash[:selects].present? end end @@ -49,7 +49,7 @@ def to_hash { filters: filters.map { |f| { name: f.name, operator: f.operator, values: f.values } }, orders: orders.map { |o| [o.attribute, o.direction] }, - group_by: respond_to?(:group_by) ? group_by : nil, + group_bys: respond_to?(:group_bys) ? group_bys.map(&:attribute) : [], selects: selects.map(&:attribute), user:, name: diff --git a/lib/api/v3/utilities/endpoints/index.rb b/lib/api/v3/utilities/endpoints/index.rb index 8eb3d9875a49..86e08d63b334 100644 --- a/lib/api/v3/utilities/endpoints/index.rb +++ b/lib/api/v3/utilities/endpoints/index.rb @@ -117,7 +117,7 @@ def calculate_resulting_params(query, provided_params) end def calculate_groups(query) - return if !query.respond_to?(:group_by) || !query.group_by + return if !query.respond_to?(:group_bys) || query.group_bys.empty? query.group_values.map do |group, count| ::API::Decorators::AggregationGroup.new(group, count, query:, current_user: User.current) From 5dc5a8d1a85441676a48dd100ddd79ee6e5b4b2d Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 17 Aug 2026 10:42:06 +0200 Subject: [PATCH 5/6] Add proper tests for the group by roundtrip --- spec/models/persisted_query_spec.rb | 28 +++ .../queries/base_query_group_bys_spec.rb | 165 ++++++++++++++++++ spec/models/queries/register_spec.rb | 81 +++++++++ .../queries/serialization/group_bys_spec.rb | 87 +++++++++ .../models/queries/serialization/hash_spec.rb | 68 ++++++++ 5 files changed, 429 insertions(+) create mode 100644 spec/models/queries/base_query_group_bys_spec.rb create mode 100644 spec/models/queries/register_spec.rb create mode 100644 spec/models/queries/serialization/group_bys_spec.rb create mode 100644 spec/models/queries/serialization/hash_spec.rb diff --git a/spec/models/persisted_query_spec.rb b/spec/models/persisted_query_spec.rb index 484497ac6f06..a3be819c1f23 100644 --- a/spec/models/persisted_query_spec.rb +++ b/spec/models/persisted_query_spec.rb @@ -54,4 +54,32 @@ expect(persisted_query.errors[:base]).to include("Cannot delete record because dependent views exist") end end + + describe "group_bys" do + it "defaults to none" do + expect(persisted_query.group_bys).to eq([]) + end + + it "is stored as an empty array" do + persisted_query.save! + + raw = described_class.connection.select_value( + "SELECT group_bys FROM persisted_queries WHERE id = #{persisted_query.id}" + ) + + expect(JSON.parse(raw)).to eq([]) + end + + # The base class gets no serializers - the `inherited` hook only installs + # them on subclasses. + it "installs a group by serializer on subclasses" do + expect(UserQuery.type_for_attribute(:group_bys).coder) + .to be_a(Queries::Serialization::GroupBys) + end + + # TODO: round trip actual group bys through the column once a PersistedQuery + # subclass registers some. UserQuery registers none, so anything it could be + # grouped by is a NotExistingGroupBy, which only tests the degraded path. + # CostReportQuery will be the first real one. + end end diff --git a/spec/models/queries/base_query_group_bys_spec.rb b/spec/models/queries/base_query_group_bys_spec.rb new file mode 100644 index 000000000000..fbac017483b0 --- /dev/null +++ b/spec/models/queries/base_query_group_bys_spec.rb @@ -0,0 +1,165 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Queries::BaseQuery, "group bys" do + current_user { create(:admin) } + + # NotificationQuery is a query class that registers group bys. + let(:instance) { Queries::Notifications::NotificationQuery.new(user: User.current) } + + describe "#group_bys" do + it "is empty by default" do + expect(instance.group_bys).to eq([]) + end + end + + describe "#group" do + it "sets a single group by" do + instance.group(:reason) + + expect(instance.group_bys.map(&:name)).to eq([:reason]) + end + + it "sets several group bys, keeping their order" do + instance.group(:reason, :project) + + expect(instance.group_bys.map(&:name)).to eq(%i[reason project_id]) + end + + it "replaces previously set group bys rather than appending" do + instance.group(:reason).group(:project) + + expect(instance.group_bys.map(&:name)).to eq([:project_id]) + end + + it "returns the query to allow chaining" do + expect(instance.group(:reason)).to eq(instance) + end + end + + describe "validations" do + it "is valid without any group by" do + expect(instance).to be_valid + end + + it "is valid with registered group bys" do + instance.group(:reason, :project) + + expect(instance).to be_valid + end + + it "is invalid with an unregistered group by" do + instance.group(:does_not_exist) + + expect(instance).not_to be_valid + expect(instance.errors[:group_by]).to be_present + end + + it "reports every invalid group by" do + instance.group(:nope, :also_nope) + instance.valid? + + expect(instance.errors[:group_by].size).to eq(2) + end + end + + describe "#groups" do + it "is nil without a group by" do + expect(instance.groups).to be_nil + end + + it "selects the group by and a count for a single group by" do + instance.group(:reason) + + expect(instance.groups.to_sql).to include('"notifications"."reason", COUNT(*)') + end + + it "selects every group by and a count for several group bys" do + instance.group(:reason, :project) + + expect(instance.groups.to_sql).to include('"notifications"."reason", "project_id", COUNT(*)') + end + + it "applies the joins of every group by" do + instance.group(:project) + + expect(instance.groups.to_sql).to include("JOIN work_packages") + end + + it "is an empty scope when invalid" do + instance.group(:does_not_exist) + + expect(instance.groups.to_sql).to include("1=0").or include("1 = 0") + end + end + + describe "#group_values" do + shared_let(:project) { create(:project) } + shared_let(:work_package) { create(:work_package, project:) } + + before do + create_list(:notification, 2, recipient: User.current, resource: work_package, reason: :mentioned) + create(:notification, recipient: User.current, resource: work_package, reason: :assigned) + end + + it "returns a count per value for a single group by" do + instance.group(:reason) + + expect(instance.group_values).to eq("mentioned" => 2, "assigned" => 1) + end + + it "returns a count per combination of values for several group bys" do + instance.group(:reason, :project) + + expect(instance.group_values).to eq(["mentioned", project.id] => 2, + ["assigned", project.id] => 1) + end + end + + # A query class that has no group bys at all - ProjectQuery does not even + # respond to #group_bys - must still validate and order. + describe "a query without group by support" do + let(:instance) { ProjectQuery.new(name: "Projects") } + + it "does not respond to group_bys" do + expect(instance).not_to respond_to(:group_bys) + end + + it "is valid" do + expect(instance).to be_valid + end + + it "still builds results" do + expect(instance.results.to_sql).to be_present + end + end +end diff --git a/spec/models/queries/register_spec.rb b/spec/models/queries/register_spec.rb new file mode 100644 index 000000000000..b93badeb5568 --- /dev/null +++ b/spec/models/queries/register_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Queries::Register do + describe "the registries" do + # A query class registering none of a given kind is normal - most notably + # group bys, which only a handful of queries declare - so the registries must + # never hand out nil. + %i[filters orders selects group_bys].each do |kind| + it "#{kind} is a hash, even before anything is registered" do + expect(described_class.public_send(kind)).to be_a(Hash) + end + + it "#{kind} returns an empty array for a query class that registered none" do + expect(described_class.public_send(kind)[Class.new]).to eq([]) + end + end + + it "excluded_filters is an array" do + expect(described_class.excluded_filters).to be_an(Array) + end + end + + describe "registering" do + let(:query_class) { Class.new } + let(:filter_class) { Class.new } + let(:order_class) { Class.new } + let(:select_class) { Class.new } + let(:group_by_class) { Class.new } + + it "collects the registered classes per query class" do + # Captured in locals because inside the block self is the Registration, + # where `filter` and friends are the DSL methods. + a_filter = filter_class + an_order = order_class + a_select = select_class + a_group_by = group_by_class + + described_class.register(query_class) do + filter a_filter + order an_order + select a_select + group_by a_group_by + end + + expect(described_class.filters[query_class]).to eq([filter_class]) + expect(described_class.orders[query_class]).to eq([order_class]) + expect(described_class.selects[query_class]).to eq([select_class]) + expect(described_class.group_bys[query_class]).to eq([group_by_class]) + end + end +end diff --git a/spec/models/queries/serialization/group_bys_spec.rb b/spec/models/queries/serialization/group_bys_spec.rb new file mode 100644 index 000000000000..b2fbe7cc1cc8 --- /dev/null +++ b/spec/models/queries/serialization/group_bys_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Queries::Serialization::GroupBys do + # NotificationQuery is a query class that registers group bys. + let(:query_class) { Queries::Notifications::NotificationQuery } + let(:instance) { described_class.new(query_class) } + + describe "#load" do + it "instantiates the registered group by for each attribute" do + expect(instance.load(%w[reason project]).map(&:class)) + .to eq([Queries::Notifications::GroupBys::GroupByReason, + Queries::Notifications::GroupBys::GroupByProject]) + end + + it "keeps the order of the serialized attributes" do + expect(instance.load(%w[project reason]).map(&:attribute)).to eq(%i[project reason]) + end + + it "returns an empty array for nil" do + expect(instance.load(nil)).to eq([]) + end + + it "returns an empty array for an empty array" do + expect(instance.load([])).to eq([]) + end + + it "degrades an unknown attribute instead of raising" do + group_bys = instance.load(%w[does_not_exist]) + + expect(group_bys.map(&:class)).to eq([Queries::GroupBys::NotExistingGroupBy]) + expect(group_bys.first).not_to be_valid + end + + it "resolves against the registry of the class it was built for" do + # UserQuery registers no group bys at all, so nothing is resolvable there. + expect(described_class.new(UserQuery).load(%w[reason]).map(&:class)) + .to eq([Queries::GroupBys::NotExistingGroupBy]) + end + end + + describe "#dump" do + # The attribute is dumped rather than #name, which may be the underlying SQL + # column - GroupByProject is keyed :project but names the project_id column - + # so that the dumped value can be loaded again. + it "reduces the group bys to their attributes" do + expect(instance.dump(instance.load(%w[reason project]))).to eq(%w[reason project]) + end + + it "returns an empty array for no group bys" do + expect(instance.dump([])).to eq([]) + end + end + + it "round trips" do + expect(instance.dump(instance.load(%w[reason project]))).to eq(%w[reason project]) + end +end diff --git a/spec/models/queries/serialization/hash_spec.rb b/spec/models/queries/serialization/hash_spec.rb new file mode 100644 index 000000000000..8da2a579ff73 --- /dev/null +++ b/spec/models/queries/serialization/hash_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Queries::Serialization::Hash do + current_user { create(:admin) } + + describe "a query without group by support" do + # This is the pair actually used in production, to hand a ProjectQuery to the + # export job. + let(:instance) do + ProjectQuery.new(name: "Projects").tap do |query| + query.where("active", "=", ["t"]) + query.order("name" => "asc") + query.select(:name) + end + end + + it "reports no group bys rather than nil" do + expect(instance.to_hash[:group_bys]).to eq([]) + end + + it "round trips the filters, orders and selects" do + restored = ProjectQuery.from_hash(instance.to_hash) + + expect(restored.name).to eq("Projects") + expect(restored.filters.map { |f| [f.name, f.operator, f.values] }) + .to eq(instance.filters.map { |f| [f.name, f.operator, f.values] }) + expect(restored.orders.map { |o| [o.attribute, o.direction] }) + .to eq(instance.orders.map { |o| [o.attribute, o.direction] }) + expect(restored.selects.map(&:attribute)).to eq(instance.selects.map(&:attribute)) + end + end + + # TODO: cover the group by round trip once a query that serializes to a hash + # actually registers group bys. Only ProjectQuery and PersistedQuery include + # this module today, and neither has any - CostReportQuery will be the first. + # The attributes rather than the group by objects are dumped, so that from_hash + # can resolve them again. +end From 24a0cdf4b84e0b78794f3a04692d8c4ea1668985 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 18 Aug 2026 11:57:23 +0200 Subject: [PATCH 6/6] Put errors on the group_bys key --- app/models/queries/base_query.rb | 2 +- config/locales/en.yml | 1 + spec/models/queries/base_query_group_bys_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/queries/base_query.rb b/app/models/queries/base_query.rb index 4344cc29e7d9..f66fc3be7a2d 100644 --- a/app/models/queries/base_query.rb +++ b/app/models/queries/base_query.rb @@ -172,7 +172,7 @@ def group_bys_valid group_bys.each do |group_by| next if group_by.valid? - add_error(:group_by, group_by.name, group_by) + add_error(:group_bys, group_by.name, group_by) end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 5a16ebc71dcc..5ebf8371fb6a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -219,6 +219,7 @@ en: position: "Position" persisted_query: filters: "Filters" + group_bys: "Group results by" name: "Name" orders: "Orders" selects: "Selects" diff --git a/spec/models/queries/base_query_group_bys_spec.rb b/spec/models/queries/base_query_group_bys_spec.rb index fbac017483b0..f777c4ae08a5 100644 --- a/spec/models/queries/base_query_group_bys_spec.rb +++ b/spec/models/queries/base_query_group_bys_spec.rb @@ -81,14 +81,14 @@ instance.group(:does_not_exist) expect(instance).not_to be_valid - expect(instance.errors[:group_by]).to be_present + expect(instance.errors[:group_bys]).to be_present end it "reports every invalid group by" do instance.group(:nope, :also_nope) instance.valid? - expect(instance.errors[:group_by].size).to eq(2) + expect(instance.errors[:group_bys].size).to eq(2) end end