Skip to content

Commit 49d213d

Browse files
lgebhardtlgebhardt
authored
Fix some testing issues including flappy tests, simplify quoting generated SQL strings (#1367)
* Refine when config.active_record.sqlite3.represent_boolean_as_integer is set * Add quoting for fields in built up sql * Remove tests for generated sql statements These are changing with new rails versions and it's creating a lot of false failures * Remove tests for generated sql statements using joins_left * Fix default sort for when empty array is provided Fixes issue with relationship requests not getting a default sort * Add helper methods for generating sql fields with aliases and quotes (cherry picked from commit 695bb7c)
1 parent ab344c4 commit 49d213d

File tree

3 files changed

+51
-79
lines changed

3 files changed

+51
-79
lines changed

‎lib/jsonapi/active_relation_resource.rb

Lines changed: 50 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -114,11 +114,11 @@ def find_fragments(filters, options = {})
114114
# This alias is going to be resolve down to the model's table name and will not actually be an alias
115115
resource_table_alias = resource_klass._table_name
116116

117-
pluck_fields = [Arel.sql("#{concat_table_field(resource_table_alias, resource_klass._primary_key)} AS #{resource_table_alias}_#{resource_klass._primary_key}")]
117+
pluck_fields = [sql_field_with_alias(resource_table_alias, resource_klass._primary_key)]
118118

119119
cache_field = attribute_to_model_field(:_cache_field) if options[:cache]
120120
if cache_field
121-
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, cache_field[:name])} AS #{resource_table_alias}_#{cache_field[:name]}")
121+
pluck_fields << sql_field_with_alias(resource_table_alias, cache_field[:name])
122122
end
123123

124124
linkage_fields = []
@@ -133,15 +133,15 @@ def find_fragments(filters, options = {})
133133

134134
linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
135135
primary_key = klass._primary_key
136-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
136+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
137137
end
138138
else
139139
klass = linkage_relationship.resource_klass
140140
linkage_fields << {relationship_name: name, resource_klass: klass}
141141

142142
linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
143143
primary_key = klass._primary_key
144-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
144+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
145145
end
146146
end
147147

@@ -150,7 +150,7 @@ def find_fragments(filters, options = {})
150150
attributes.try(:each) do |attribute|
151151
model_field = resource_klass.attribute_to_model_field(attribute)
152152
model_fields[attribute] = model_field
153-
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
153+
pluck_fields << sql_field_with_alias(resource_table_alias, model_field[:name])
154154
end
155155

156156
sort_fields = options.dig(:_relation_helper_options, :sort_fields)
@@ -409,13 +409,13 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
409409
resource_table_alias = join_manager.join_details_by_relationship(relationship)[:alias]
410410

411411
pluck_fields = [
412-
Arel.sql("#{_table_name}.#{_primary_key} AS source_id"),
413-
Arel.sql("#{concat_table_field(resource_table_alias, resource_klass._primary_key)} AS #{resource_table_alias}_#{resource_klass._primary_key}")
412+
Arel.sql("#{_table_name}.#{_primary_key} AS \"source_id\""),
413+
sql_field_with_alias(resource_table_alias, resource_klass._primary_key)
414414
]
415415

416416
cache_field = resource_klass.attribute_to_model_field(:_cache_field) if options[:cache]
417417
if cache_field
418-
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, cache_field[:name])} AS #{resource_table_alias}_#{cache_field[:name]}")
418+
pluck_fields << sql_field_with_alias(resource_table_alias, cache_field[:name])
419419
end
420420

421421
linkage_fields = []
@@ -430,15 +430,15 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
430430

431431
linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
432432
primary_key = klass._primary_key
433-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
433+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
434434
end
435435
else
436436
klass = linkage_relationship.resource_klass
437437
linkage_fields << {relationship_name: name, resource_klass: klass}
438438

439439
linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
440440
primary_key = klass._primary_key
441-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
441+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
442442
end
443443
end
444444

@@ -447,7 +447,7 @@ def find_related_monomorphic_fragments(source_rids, relationship, options, conne
447447
attributes.try(:each) do |attribute|
448448
model_field = resource_klass.attribute_to_model_field(attribute)
449449
model_fields[attribute] = model_field
450-
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, model_field[:name])} AS #{resource_table_alias}_#{model_field[:name]}")
450+
pluck_fields << sql_field_with_alias(resource_table_alias, model_field[:name])
451451
end
452452

453453
sort_fields = options.dig(:_relation_helper_options, :sort_fields)
@@ -543,9 +543,9 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
543543
related_type = concat_table_field(_table_name, relationship.polymorphic_type)
544544

545545
pluck_fields = [
546-
Arel.sql("#{primary_key} AS #{_table_name}_#{_primary_key}"),
547-
Arel.sql("#{related_key} AS #{_table_name}_#{relationship.foreign_key}"),
548-
Arel.sql("#{related_type} AS #{_table_name}_#{relationship.polymorphic_type}")
546+
Arel.sql("#{primary_key} AS #{alias_table_field(_table_name, _primary_key)}"),
547+
Arel.sql("#{related_key} AS #{alias_table_field(_table_name, relationship.foreign_key)}"),
548+
Arel.sql("#{related_type} AS #{alias_table_field(_table_name, relationship.polymorphic_type)}")
549549
]
550550

551551
# Get the additional fields from each relation. There's a limitation that the fields must exist in each relation
@@ -570,7 +570,7 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
570570

571571
cache_offset = relation_index
572572
if cache_field
573-
pluck_fields << Arel.sql("#{concat_table_field(table_alias, cache_field[:name])} AS cache_#{type}_#{cache_field[:name]}")
573+
pluck_fields << sql_field_with_alias(table_alias, cache_field[:name])
574574
relation_index+= 1
575575
end
576576

@@ -579,7 +579,7 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
579579
attributes.try(:each) do |attribute|
580580
model_field = related_klass.attribute_to_model_field(attribute)
581581
model_fields[attribute] = model_field
582-
pluck_fields << Arel.sql("#{concat_table_field(table_alias, model_field[:name])} AS #{table_alias}_#{model_field[:name]}")
582+
pluck_fields << sql_field_with_alias(table_alias, model_field[:name])
583583
relation_index+= 1
584584
end
585585

@@ -616,15 +616,15 @@ def find_related_polymorphic_fragments(source_rids, relationship, options, conne
616616

617617
linkage_table_alias = join_manager.join_details_by_polymorphic_relationship(linkage_relationship, resource_type)[:alias]
618618
primary_key = klass._primary_key
619-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
619+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
620620
end
621621
else
622622
klass = linkage_relationship.resource_klass
623623
linkage_fields << {relationship: linkage_relationship, resource_klass: klass}
624624

625625
linkage_table_alias = join_manager.join_details_by_relationship(linkage_relationship)[:alias]
626626
primary_key = klass._primary_key
627-
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS #{linkage_table_alias}_#{primary_key}")
627+
pluck_fields << sql_field_with_alias(linkage_table_alias, primary_key)
628628
end
629629
end
630630

@@ -790,22 +790,51 @@ def concat_table_field(table, field, quoted = false)
790790
if table.blank? || field.to_s.include?('.')
791791
# :nocov:
792792
if quoted
793-
"\"#{field.to_s}\""
793+
quote(field)
794794
else
795795
field.to_s
796796
end
797797
# :nocov:
798798
else
799799
if quoted
800+
"#{quote(table)}.#{quote(field)}"
801+
else
800802
# :nocov:
801-
"\"#{table.to_s}\".\"#{field.to_s}\""
803+
"#{table.to_s}.#{field.to_s}"
802804
# :nocov:
805+
end
806+
end
807+
end
808+
809+
def sql_field_with_alias(table, field, quoted = true)
810+
Arel.sql("#{concat_table_field(table, field, quoted)} AS #{alias_table_field(table, field, quoted)}")
811+
end
812+
813+
def alias_table_field(table, field, quoted = false)
814+
if table.blank? || field.to_s.include?('.')
815+
# :nocov:
816+
if quoted
817+
quote(field)
803818
else
804-
"#{table.to_s}.#{field.to_s}"
819+
field.to_s
820+
end
821+
# :nocov:
822+
else
823+
if quoted
824+
# :nocov:
825+
quote("#{table.to_s}_#{field.to_s}")
826+
# :nocov:
827+
else
828+
"#{table.to_s}_#{field.to_s}"
805829
end
806830
end
807831
end
808832

833+
def quote(field)
834+
"\"#{field.to_s}\""
835+
end
836+
837+
809838
def apply_filters(records, filters, options = {})
810839
if filters
811840
filters.each do |filter, value|

‎lib/jsonapi/basic_resource.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1058,7 +1058,7 @@ def default_sort
10581058
end
10591059

10601060
def construct_order_options(sort_params)
1061-
sort_params ||= default_sort
1061+
sort_params = default_sort if sort_params.blank?
10621062

10631063
return {} unless sort_params
10641064

‎test/unit/active_relation_resource_finder/join_manager_test.rb

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,6 @@ def test_add_nested_scoped_joins
110110
records = Api::V10::PostResource.records({})
111111
records = join_manager.join(records, {})
112112

113-
if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
114-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
115-
else
116-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
117-
end
118-
119-
assert_equal sql, records.to_sql
120-
121113
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
122114
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
123115
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
@@ -135,37 +127,11 @@ def test_add_nested_scoped_joins
135127
records = Api::V10::PostResource.records({})
136128
records = join_manager.join(records, {})
137129

138-
# Note sql is in different order, but aliases should still be right
139-
if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
140-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
141-
else
142-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
143-
end
144-
145-
assert_equal sql, records.to_sql
146-
147130
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
148131
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
149132
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
150133
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:tags)))
151134
assert_hash_equals({alias: 'people', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:author)))
152-
153-
# Easier to read SQL to show joins are the same, but in different order
154-
# Pass 1
155-
# SELECT "posts".* FROM "posts"
156-
# LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id"
157-
# LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"
158-
# LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id"
159-
# LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id"
160-
# LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = 1 AND "author"."special" = 1
161-
#
162-
# Pass 2
163-
# SELECT "posts".* FROM "posts"
164-
# LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id"
165-
# LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id"
166-
# LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id"
167-
# LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id"
168-
# LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = 1 AND "author"."special" = 1
169135
end
170136

171137
def test_add_nested_joins_with_fields
@@ -179,14 +145,6 @@ def test_add_nested_joins_with_fields
179145
records = Api::V10::PostResource.records({})
180146
records = join_manager.join(records, {})
181147

182-
if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
183-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
184-
else
185-
sql = 'SELECT "posts".* FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "posts"."author_id" LEFT OUTER JOIN "people" "authors_comments" ON "authors_comments"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
186-
end
187-
188-
assert_equal sql, records.to_sql
189-
190148
assert_hash_equals({alias: 'posts', join_type: :root}, join_manager.source_join_details)
191149
assert_hash_equals({alias: 'comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
192150
assert_hash_equals({alias: 'authors_comments', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
@@ -202,16 +160,6 @@ def test_add_joins_with_sub_relationship
202160
records = Api::V10::PostResource.records({})
203161
records = join_manager.join(records, {})
204162

205-
if (Rails::VERSION::MAJOR == 6 && Rails::VERSION::MINOR >= 1) || Rails::VERSION::MAJOR > 6
206-
sql = 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" author ON author."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" LEFT OUTER JOIN "comments" "comments_people" ON "comments_people"."author_id" = "people"."id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
207-
assert_hash_equals({alias: 'author', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
208-
else
209-
sql = 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" LEFT OUTER JOIN "people" ON "people"."id" = "comments"."author_id" LEFT OUTER JOIN "comments_tags" ON "comments_tags"."comment_id" = "comments"."id" LEFT OUTER JOIN "tags" ON "tags"."id" = "comments_tags"."tag_id" LEFT OUTER JOIN "comments" "comments_people" ON "comments_people"."author_id" = "people"."id" WHERE "comments"."approved" = ' + db_true + ' AND "author"."special" = ' + db_true
210-
assert_hash_equals({alias: 'people', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:author)))
211-
end
212-
213-
assert_equal sql, records.to_sql
214-
215163
assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.source_join_details)
216164
assert_hash_equals({alias: 'comments', join_type: :inner}, join_manager.join_details_by_relationship(Api::V10::PostResource._relationship(:comments)))
217165
assert_hash_equals({alias: 'tags', join_type: :left}, join_manager.join_details_by_relationship(Api::V10::CommentResource._relationship(:tags)))
@@ -280,11 +228,6 @@ def test_polymorphic_join_belongs_to_filter_on_resource
280228
records = PictureResource.records({})
281229
records = join_manager.join(records, {})
282230

283-
#TODO: Fix this with a better test
284-
sql_v1 = 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\' LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "file_properties" ON "file_properties"."fileable_id" = "pictures"."id" AND "file_properties"."fileable_type" = \'Picture\''
285-
sql_v2 = 'SELECT "pictures".* FROM "pictures" LEFT OUTER JOIN "documents" ON "documents"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Document\' LEFT OUTER JOIN "products" ON "products"."id" = "pictures"."imageable_id" AND "pictures"."imageable_type" = \'Product\' LEFT OUTER JOIN "file_properties" ON "file_properties"."fileable_type" = \'Picture\' AND "file_properties"."fileable_id" = "pictures"."id"'
286-
assert records.to_sql == sql_v1 || records.to_sql == sql_v2, 'did not generate an expected sql statement'
287-
288231
assert_hash_equals({alias: 'pictures', join_type: :root}, join_manager.source_join_details)
289232
assert_hash_equals({alias: 'products', join_type: :left}, join_manager.join_details_by_polymorphic_relationship(PictureResource._relationship(:imageable), 'products'))
290233
assert_hash_equals({alias: 'documents', join_type: :left}, join_manager.join_details_by_polymorphic_relationship(PictureResource._relationship(:imageable), 'documents'))

0 commit comments

Comments
 (0)