Skip to content

Commit 69a4432

Browse files
committed
Add default ORDER BY id to Sequel model queries
Adds a Sequel extension that automatically appends ORDER BY id to model queries, ensuring deterministic results for consistent API responses and reliable test behavior in parallel runs. Skips adding the default order when: - Query already has an explicit ORDER BY clause - Query has GROUP BY (aggregated results don't have individual ids) - Query is schema introspection (LIMIT 0) - Query has UNION/INTERSECT/EXCEPT (ORDER BY before UNION is a syntax error) - Query has DISTINCT ON (requires matching ORDER BY) - Query is a subquery (outer query handles ordering) - Model doesn't have id as primary key - id is not in the select list For JOIN queries (including many_to_many associations), it uses a qualified column (table.id) to avoid ambiguity.
1 parent b27da11 commit 69a4432

File tree

7 files changed

+199
-8
lines changed

7 files changed

+199
-8
lines changed

app/models/runtime/buildpack_lifecycle_data_model.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,7 @@ class BuildpackLifecycleDataModel < Sequel::Model(:buildpack_lifecycle_data)
2828
one_to_many :buildpack_lifecycle_buildpacks,
2929
class: '::VCAP::CloudController::BuildpackLifecycleBuildpackModel',
3030
key: :buildpack_lifecycle_data_guid,
31-
primary_key: :guid,
32-
order: :id
31+
primary_key: :guid
3332
plugin :nested_attributes
3433
nested_attributes :buildpack_lifecycle_buildpacks, destroy: true
3534
add_association_dependencies buildpack_lifecycle_buildpacks: :destroy

app/models/runtime/cnb_lifecycle_data_model.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,7 @@ class CNBLifecycleDataModel < Sequel::Model(:cnb_lifecycle_data)
2727
one_to_many :buildpack_lifecycle_buildpacks,
2828
class: '::VCAP::CloudController::BuildpackLifecycleBuildpackModel',
2929
key: :cnb_lifecycle_data_guid,
30-
primary_key: :guid,
31-
order: :id
30+
primary_key: :guid
3231
plugin :nested_attributes
3332
nested_attributes :buildpack_lifecycle_buildpacks, destroy: true
3433
add_association_dependencies buildpack_lifecycle_buildpacks: :destroy

app/models/runtime/process_model.rb

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,16 +112,14 @@ def diego
112112
left_primary_key: %i[app_guid type], left_key: %i[app_guid process_type],
113113
right_primary_key: :guid, right_key: :route_guid,
114114
distinct: true,
115-
order: Sequel.asc(:id),
116115
eager: :domain
117116

118117
many_to_many :sidecars,
119118
class: 'VCAP::CloudController::SidecarModel',
120119
join_table: SidecarProcessTypeModel.table_name,
121120
left_primary_key: %i[app_guid type], left_key: %i[app_guid type],
122121
right_primary_key: :guid, right_key: :sidecar_guid,
123-
distinct: true,
124-
order: Sequel.asc(:id)
122+
distinct: true
125123

126124
one_to_many :route_mappings, class: 'VCAP::CloudController::RouteMappingModel', primary_key: %i[app_guid type], key: %i[app_guid process_type]
127125

app/models/runtime/route.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ class InvalidOrganizationRelation < CloudController::Errors::InvalidRelation; en
2222
left_primary_key: :guid, left_key: :route_guid,
2323
right_primary_key: %i[app_guid type], right_key: %i[app_guid process_type],
2424
distinct: true,
25-
order: Sequel.asc(:id),
2625
conditions: { type: ProcessTypes::WEB }
2726

2827
many_to_many :shared_spaces,

lib/cloud_controller/db.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
require 'cloud_controller/execution_context'
66
require 'sequel/extensions/query_length_logging'
77
require 'sequel/extensions/request_query_metrics'
8+
require 'sequel/extensions/default_order_by_id'
89

910
module VCAP::CloudController
1011
class DB
@@ -45,6 +46,7 @@ def self.connect(opts, logger)
4546
add_connection_expiration_extension(db, opts)
4647
add_connection_validator_extension(db, opts)
4748
db.extension(:requires_unique_column_names_in_subquery)
49+
db.extension(:default_order_by_id)
4850
add_connection_metrics_extension(db)
4951
db
5052
end
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
# frozen_string_literal: true
2+
3+
# This Sequel extension adds a default ORDER BY id to model queries.
4+
#
5+
# It skips adding the default order when:
6+
# - Query already has an explicit ORDER BY clause
7+
# - Query has GROUP BY (aggregated results don't have individual ids)
8+
# - Query is schema introspection (LIMIT 0)
9+
# - Query has UNION/INTERSECT/EXCEPT (ORDER BY before UNION is a syntax error)
10+
# - Query has DISTINCT ON (requires matching ORDER BY)
11+
# - Query is a subquery (outer query handles ordering)
12+
# - Model doesn't have id as primary key
13+
# - id is not in the select list
14+
#
15+
# For JOIN queries (including many_to_many associations), it uses a qualified
16+
# column (table.id) to avoid ambiguity.
17+
#
18+
# This ensures deterministic query results, which is important for:
19+
# - Consistent API responses
20+
# - Reliable test behavior (especially in parallel test runs)
21+
#
22+
# Usage:
23+
# DB.extension(:default_order_by_id)
24+
#
25+
module Sequel
26+
module DefaultOrderById
27+
module DatasetMethods
28+
def select_sql
29+
id_column = find_id_column
30+
if id_column
31+
order(id_column).select_sql
32+
else
33+
super
34+
end
35+
end
36+
37+
private
38+
39+
def find_id_column
40+
return nil if should_skip_default_order?
41+
42+
find_id_in_select_list
43+
end
44+
45+
def should_skip_default_order?
46+
opts[:order] || # Already has explicit order
47+
opts[:group] || # Aggregated results don't have individual ids
48+
opts[:limit] == 0 || # Schema introspection, not a real query
49+
opts[:compounds] || # ORDER BY before UNION is a syntax error
50+
distinct_on? || # DISTINCT ON requires matching ORDER BY
51+
subquery? || # Outer query handles ordering
52+
!model_has_id_primary_key? # No id column to order by
53+
end
54+
55+
def distinct_on?
56+
opts[:distinct].is_a?(Array) && opts[:distinct].any?
57+
end
58+
59+
def subquery?
60+
opts[:from].is_a?(Array) && opts[:from].any? { |f| f.is_a?(Sequel::SQL::AliasedExpression) }
61+
end
62+
63+
def model_has_id_primary_key?
64+
return false unless respond_to?(:model) && model
65+
66+
model.primary_key == :id
67+
rescue StandardError
68+
false
69+
end
70+
71+
def find_id_in_select_list
72+
select_cols = opts[:select]
73+
74+
# SELECT * includes all columns including id
75+
if select_cols.nil? || select_cols.empty?
76+
return qualified_id_column if opts[:join]
77+
78+
return :id
79+
end
80+
81+
# Find id column in select list
82+
select_cols.each do |col|
83+
# ColumnAll (e.g., SELECT table.*) includes all columns including id
84+
if col.is_a?(Sequel::SQL::ColumnAll) && col.table == model.table_name
85+
return qualified_id_column if opts[:join]
86+
87+
return :id
88+
end
89+
90+
# Check for :id, :table__id, or aliased id
91+
id_col = extract_id_column(col)
92+
return id_col if id_col
93+
end
94+
95+
nil
96+
end
97+
98+
def qualified_id_column
99+
Sequel.qualify(model.table_name, :id)
100+
end
101+
102+
def extract_id_column(col)
103+
case col
104+
when Symbol
105+
col if col == :id || col.to_s.end_with?('__id')
106+
when Sequel::SQL::Identifier
107+
col if col.value == :id
108+
when Sequel::SQL::QualifiedIdentifier
109+
col if col.column == :id
110+
when Sequel::SQL::AliasedExpression
111+
col.alias if col.alias == :id # Use alias symbol (not the full expression with AS)
112+
end
113+
end
114+
end
115+
end
116+
117+
Dataset.register_extension(:default_order_by_id, DefaultOrderById::DatasetMethods)
118+
end
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
# frozen_string_literal: true
2+
3+
require 'spec_helper'
4+
require 'sequel/extensions/default_order_by_id'
5+
6+
RSpec.describe 'Sequel::DefaultOrderById' do
7+
# Use an existing model for testing
8+
let(:model_class) { VCAP::CloudController::Organization }
9+
10+
describe 'adding default order' do
11+
it 'adds ORDER BY id to model queries' do
12+
expect(model_class.dataset.sql).to match(/ORDER BY .id.$/)
13+
end
14+
15+
it 'preserves explicit order' do
16+
expect(model_class.dataset.order(:name).sql).to match(/ORDER BY .name.$/)
17+
end
18+
end
19+
20+
describe 'skipping default order' do
21+
it 'skips for queries with GROUP BY' do
22+
expect(model_class.dataset.group(:status).sql).not_to match(/ORDER BY/)
23+
end
24+
25+
it 'skips for UNION queries' do
26+
ds1 = model_class.dataset.where(name: 'a')
27+
ds2 = model_class.dataset.where(name: 'b')
28+
sql = ds1.union(ds2, all: true, from_self: false).sql
29+
# ORDER BY before UNION is a syntax error; subsequent datasets are parenthesized so it's harmless there
30+
expect(sql).to start_with('SELECT * FROM "organizations" WHERE ("name" = \'a\') UNION')
31+
end
32+
33+
it 'skips for DISTINCT ON queries' do
34+
sql = model_class.dataset.distinct(:guid).sql
35+
# DISTINCT ON requires ORDER BY to match the distinct columns
36+
expect(sql).not_to match(/ORDER BY/)
37+
end
38+
39+
it 'skips for subqueries (from_self)' do
40+
sql = model_class.dataset.where(name: 'a').from_self.sql
41+
# Outer query should not have ORDER BY - subquery handles it
42+
expect(sql).to end_with('AS "t1"')
43+
end
44+
45+
it 'skips for queries where id is not in select list' do
46+
expect(model_class.dataset.select(:guid, :name).sql).not_to match(/ORDER BY/)
47+
end
48+
end
49+
50+
describe 'handling JOIN queries' do
51+
it 'uses qualified column to avoid ambiguity' do
52+
sql = model_class.dataset.join(:spaces, organization_id: :id).sql
53+
expect(sql).to match(/ORDER BY .organizations.\..id.$/)
54+
end
55+
56+
it 'handles ColumnAll in select list (many_to_many pattern)' do
57+
# many_to_many associations use SELECT table.* which creates a ColumnAll
58+
sql = model_class.dataset.select(Sequel::SQL::ColumnAll.new(:organizations)).join(:spaces, organization_id: :id).sql
59+
expect(sql).to match(/ORDER BY .organizations.\..id.$/)
60+
end
61+
end
62+
63+
describe 'handling qualified id columns' do
64+
it 'uses qualified column when present in select' do
65+
sql = model_class.dataset.select(:organizations__id, :organizations__name).sql
66+
expect(sql).to match(/ORDER BY .organizations.\..id./)
67+
end
68+
end
69+
70+
describe 'handling aliased id columns' do
71+
it 'orders by alias when id is aliased' do
72+
sql = model_class.dataset.select(Sequel.as(:organizations__id, :id), :name).sql
73+
expect(sql).to match(/ORDER BY .id.$/)
74+
end
75+
end
76+
end

0 commit comments

Comments
 (0)