Skip to content

Commit 57e6b6a

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). Detects from_self by checking for subquery datasets in the FROM clause, avoiding false positives on plain table aliases. Annotate auto-ordered queries with an SQL comment via the sql_comments extension for traceability in query logs. 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 57e6b6a

12 files changed

Lines changed: 346 additions & 30 deletions

File tree

.rubocop_todo.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ RSpec/BeforeAfterAll:
173173
- 'spec/integration/app_log_emitter_spec.rb'
174174
- 'spec/integration/cors_spec.rb'
175175
- 'spec/isolated_specs/inline_runner_spec.rb'
176+
- 'spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb'
176177
- 'spec/unit/lib/vcap/rest_api/event_query_spec.rb'
177178
- 'spec/unit/lib/vcap/rest_api/query_spec.rb'
178179

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: 4 additions & 1 deletion
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
@@ -32,7 +33,7 @@ def self.connect(opts, logger)
3233
db.logger = logger
3334
db.sql_log_level = opts[:log_level]
3435
db.extension :caller_logging
35-
db.caller_logging_ignore = /sequel_paginator|query_length_logging|sequel_pg|paginated_collection_renderer|request_query_metrics/
36+
db.caller_logging_ignore = /sequel_paginator|query_length_logging|sequel_pg|paginated_collection_renderer|request_query_metrics|default_order_by_id/
3637
db.caller_logging_formatter = lambda do |caller|
3738
"(#{caller.sub(%r{^.*/cloud_controller_ng/}, '')})"
3839
end
@@ -45,6 +46,8 @@ 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(:sql_comments)
50+
db.extension(:default_order_by_id)
4851
add_connection_metrics_extension(db)
4952
db
5053
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: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
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(:sql_comments)
24+
# DB.extension(:default_order_by_id)
25+
#
26+
module Sequel
27+
module DefaultOrderById
28+
module DatasetMethods
29+
def all(*, &)
30+
ds = default_order_by_id
31+
return super unless ds
32+
33+
ds.all(*, &)
34+
end
35+
36+
def each(*, &)
37+
ds = default_order_by_id
38+
return super unless ds
39+
40+
ds.each(*, &)
41+
end
42+
43+
def first(*, &)
44+
ds = default_order_by_id
45+
return super unless ds
46+
47+
ds.first(*, &)
48+
end
49+
50+
private
51+
52+
def default_order_by_id
53+
id_col = id_column_for_order
54+
return unless id_col
55+
56+
order(id_col).comment('default_order_by_id')
57+
end
58+
59+
def id_column_for_order
60+
return if already_ordered? || incompatible_with_order? || not_a_data_query? || !model_has_id_primary_key?
61+
62+
find_id_column
63+
end
64+
65+
def already_ordered?
66+
opts[:order]
67+
end
68+
69+
def incompatible_with_order?
70+
opts[:group] || # Aggregated results don't have individual ids
71+
opts[:compounds] || # Compound queries (e.g. UNION) have own ordering
72+
distinct_on? || # DISTINCT ON requires matching ORDER BY
73+
from_self? # Outer query handles ordering
74+
end
75+
76+
def distinct_on?
77+
opts[:distinct].is_a?(Array) && opts[:distinct].any?
78+
end
79+
80+
def from_self?
81+
opts[:from].is_a?(Array) && opts[:from].any? { |f| f.is_a?(Sequel::SQL::AliasedExpression) && f.expression.is_a?(Sequel::Dataset) }
82+
end
83+
84+
def not_a_data_query?
85+
opts[:limit] == 0 # Schema introspection query
86+
end
87+
88+
def model_has_id_primary_key?
89+
return false unless respond_to?(:model) && model
90+
91+
model.primary_key == :id
92+
end
93+
94+
def find_id_column
95+
select_cols = opts[:select]
96+
97+
if select_cols.nil? || select_cols.empty?
98+
# SELECT * includes id
99+
if opts[:join]
100+
# Qualify to avoid ambiguity with joined tables
101+
return Sequel.qualify(model.table_name, :id)
102+
end
103+
104+
return :id
105+
end
106+
107+
select_cols.each do |col|
108+
# SELECT table.* includes id
109+
return :id if col.is_a?(Sequel::SQL::ColumnAll) && col.table == model.table_name
110+
111+
id_col = extract_id_column(col)
112+
return id_col if id_col
113+
end
114+
115+
nil
116+
end
117+
118+
def extract_id_column(col)
119+
return col if id_expression?(col)
120+
121+
return col.alias if col.is_a?(Sequel::SQL::AliasedExpression) && id_expression?(col.expression)
122+
123+
nil
124+
end
125+
126+
def id_expression?(expr)
127+
case expr
128+
when Symbol
129+
expr == :id || expr.to_s.end_with?('__id')
130+
when Sequel::SQL::Identifier
131+
expr.value == :id
132+
when Sequel::SQL::QualifiedIdentifier
133+
expr.column == :id
134+
else
135+
false
136+
end
137+
end
138+
end
139+
end
140+
141+
Dataset.register_extension(:default_order_by_id, DefaultOrderById::DatasetMethods)
142+
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_relative '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: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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)
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+
postgres = ENV.fetch('POSTGRES_CONNECTION_PREFIX', 'postgres://postgres@localhost:5432')
18+
{
19+
'mysql' => ENV.fetch('MYSQL_CONNECTION_PREFIX', 'mysql2://root:password@localhost:3306'),
20+
'postgres' => postgres
21+
}.fetch(db_type, postgres)
22+
end
23+
24+
def default_name
25+
test_env_number = ENV.fetch('TEST_ENV_NUMBER', '')
26+
test_env_number.empty? ? 'cc_test_1' : "cc_test_#{test_env_number}"
27+
end
28+
end

0 commit comments

Comments
 (0)