Skip to content

Commit eb79fd5

Browse files
committed
Add default ORDER BY id to Sequel model queries
Adds a Sequel extension that hooks into fetch methods (all, each, first) to add ORDER BY id just before execution, ensuring deterministic query results for consistent API responses and reliable test behavior. Skips ordering when incompatible (GROUP BY, compounds, DISTINCT ON, from_self) or unnecessary (explicit ORDER BY, no id column). Remove now-redundant explicit order: :id from lifecycle data model associations. Consolidate diego/lrp_constants require into cloud_controller/diego/constants. Introduce lightweight_db_spec_helper for Sequel extension specs that only need a bare DB connection without the full app bootstrap. Extract DbConnectionString to share connection logic between DbConfig and the new helper. Switch default_order_by_id and query_length_logging specs to use it.
1 parent 13b91dc commit eb79fd5

11 files changed

Lines changed: 326 additions & 29 deletions

File tree

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

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

lib/cloud_controller/diego/constants.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
require 'diego/lrp_constants'
2+
13
module VCAP::CloudController
24
module Diego
35
STAGING_DOMAIN = 'cf-app-staging'.freeze

lib/cloud_controller/diego/reporters/instances_reporter.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
require 'utils/workpool'
2+
require 'cloud_controller/diego/constants'
23
require 'cloud_controller/diego/reporters/reporter_mixins'
3-
require 'diego/lrp_constants'
44

55
module VCAP::CloudController
66
module Diego
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
# frozen_string_literal: true
2+
3+
# Sequel extension that adds default ORDER BY id to model queries.
4+
#
5+
# Hooks into fetch methods (all, each, first) to add ORDER BY id just before
6+
# execution. This ensures ordering is only added to the final query, not to
7+
# subqueries or compound query parts.
8+
#
9+
# Skips default ordering when:
10+
# - Query already has explicit ORDER BY
11+
# - Query is incompatible (GROUP BY, compounds, DISTINCT ON, from_self)
12+
# - Query is schema introspection (LIMIT 0)
13+
# - Model doesn't have id as primary key
14+
# - id is not in the select list
15+
#
16+
# For JOIN queries with SELECT *, uses qualified column (table.id) to avoid
17+
# ambiguity.
18+
#
19+
# Ensures deterministic query results for consistent API responses and
20+
# reliable test behavior.
21+
#
22+
# Usage:
23+
# DB.extension(:default_order_by_id)
24+
#
25+
module Sequel
26+
module DefaultOrderById
27+
module DatasetMethods
28+
def all(*, &)
29+
id_col = id_column_for_order
30+
return super unless id_col
31+
32+
order(id_col).all(*, &)
33+
end
34+
35+
def each(*, &)
36+
id_col = id_column_for_order
37+
return super unless id_col
38+
39+
order(id_col).each(*, &)
40+
end
41+
42+
def first(*, &)
43+
id_col = id_column_for_order
44+
return super unless id_col
45+
46+
order(id_col).first(*, &)
47+
end
48+
49+
private
50+
51+
def id_column_for_order
52+
return if already_ordered? || incompatible_with_order? || not_a_data_query? || !model_has_id_primary_key?
53+
54+
find_id_column
55+
end
56+
57+
def already_ordered?
58+
opts[:order]
59+
end
60+
61+
def incompatible_with_order?
62+
opts[:group] || # Aggregated results don't have individual ids
63+
opts[:compounds] || # Compound queries (e.g. UNION) have own ordering
64+
distinct_on? || # DISTINCT ON requires matching ORDER BY
65+
from_self? # Outer query handles ordering
66+
end
67+
68+
def distinct_on?
69+
opts[:distinct].is_a?(Array) && opts[:distinct].any?
70+
end
71+
72+
def from_self?
73+
opts[:from].is_a?(Array) && opts[:from].any? { |f| f.is_a?(Sequel::SQL::AliasedExpression) }
74+
end
75+
76+
def not_a_data_query?
77+
opts[:limit] == 0 # Schema introspection query
78+
end
79+
80+
def model_has_id_primary_key?
81+
return false unless respond_to?(:model) && model
82+
83+
model.primary_key == :id
84+
end
85+
86+
def find_id_column
87+
select_cols = opts[:select]
88+
89+
if select_cols.nil? || select_cols.empty?
90+
# SELECT * includes id
91+
if opts[:join]
92+
# Qualify to avoid ambiguity with joined tables
93+
return Sequel.qualify(model.table_name, :id)
94+
end
95+
96+
return :id
97+
end
98+
99+
select_cols.each do |col|
100+
# SELECT table.* includes id
101+
return :id if col.is_a?(Sequel::SQL::ColumnAll) && col.table == model.table_name
102+
103+
id_col = extract_id_column(col)
104+
return id_col if id_col
105+
end
106+
107+
nil
108+
end
109+
110+
def extract_id_column(col)
111+
return col if id_expression?(col)
112+
113+
return col.alias if col.is_a?(Sequel::SQL::AliasedExpression) && id_expression?(col.expression)
114+
115+
nil
116+
end
117+
118+
def id_expression?(expr)
119+
case expr
120+
when Symbol
121+
expr == :id || expr.to_s.end_with?('__id')
122+
when Sequel::SQL::Identifier
123+
expr.value == :id
124+
when Sequel::SQL::QualifiedIdentifier
125+
expr.column == :id
126+
else
127+
false
128+
end
129+
end
130+
end
131+
end
132+
133+
Dataset.register_extension(:default_order_by_id, DefaultOrderById::DatasetMethods)
134+
end

spec/lightweight_db_spec_helper.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
require 'lightweight_spec_helper'
2+
require 'sequel'
3+
require 'support/bootstrap/db_connection_string'
4+
5+
DB = Sequel.connect(DbConnectionString.new.to_s)

spec/support/bootstrap/db_config.rb

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
require 'cloud_controller/db'
22
require 'cloud_controller/database_parts_parser'
3+
require 'support/bootstrap/db_connection_string'
34

45
class DbConfig
56
def initialize(connection_string: ENV.fetch('DB_CONNECTION_STRING', nil), db_type: ENV.fetch('DB', nil))
6-
@connection_string = connection_string || default_connection_string(db_type || 'postgres')
7+
@connection_string = DbConnectionString.new(connection_string: connection_string, db_type: db_type).to_s
78
initialize_environment_for_cc_spawning
89
end
910

@@ -49,25 +50,4 @@ def self.reset_environment
4950
def initialize_environment_for_cc_spawning
5051
ENV['DB_CONNECTION_STRING'] = connection_string
5152
end
52-
53-
def default_connection_string(db_type)
54-
"#{default_connection_prefix(db_type)}/#{default_name}"
55-
end
56-
57-
def default_connection_prefix(db_type)
58-
default_connection_prefixes = {
59-
'mysql' => ENV['MYSQL_CONNECTION_PREFIX'] || 'mysql2://root:password@localhost:3306',
60-
'postgres' => ENV['POSTGRES_CONNECTION_PREFIX'] || 'postgres://postgres@localhost:5432'
61-
}
62-
63-
default_connection_prefixes[db_type]
64-
end
65-
66-
def default_name
67-
if ENV['TEST_ENV_NUMBER'].presence
68-
"cc_test_#{ENV.fetch('TEST_ENV_NUMBER')}"
69-
else
70-
'cc_test_1'
71-
end
72-
end
7353
end
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
class DbConnectionString
2+
def initialize(connection_string: ENV.fetch('DB_CONNECTION_STRING', nil), db_type: ENV.fetch('DB', nil))
3+
@connection_string = connection_string || default_connection_string(db_type || 'postgres')
4+
end
5+
6+
def to_s
7+
@connection_string
8+
end
9+
10+
private
11+
12+
def default_connection_string(db_type)
13+
"#{default_connection_prefix(db_type)}/#{default_name}"
14+
end
15+
16+
def default_connection_prefix(db_type)
17+
{
18+
'mysql' => ENV['MYSQL_CONNECTION_PREFIX'] || 'mysql2://root:password@localhost:3306',
19+
'postgres' => ENV['POSTGRES_CONNECTION_PREFIX'] || 'postgres://postgres@localhost:5432'
20+
}.fetch(db_type)
21+
end
22+
23+
def default_name
24+
if ENV['TEST_ENV_NUMBER'].to_s.empty?
25+
'cc_test_1'
26+
else
27+
"cc_test_#{ENV.fetch('TEST_ENV_NUMBER')}"
28+
end
29+
end
30+
end

0 commit comments

Comments
 (0)