Fix & Refactor BackendAPI scopes: orphans & not_used_by #1450

Martouta · 2019-11-22T14:11:01Z

1st commit

This comment is not true anymore. Now it is tested in the place where one would expect:

test/unit/fields/fields_test.rb
test/unit/account/fields_test.rb
test/unit/extra_fields_test.rb
Each testing individually what it should.

It is unrelated to the rest of the PR but it is just removing an outdated pretty irrelevant comment and removing it was committed originally just to check if circleci was working properly back then 😄

The rest

Makes lib/tasks/backend_api.rake more efficient by fetching the account in the same query and selecting only the necessary fields.

Closes THREESCALE-3966
Closes THREESCALE-3554

Fixes this random failure.

The reason is in BackendApiConfigs at that moment it fails there is stored something like this:

backend_api_configs
--------------------------------------------------------
ID ; backend_api_id ; service_id
1  ; NULL           ; NULL
2  ; NULL           ; NULL

What this line is actually doing in that case is:

SELECT `backend_apis`.* FROM `backend_apis` WHERE (`backend_apis`.`id` NOT IN (NULL, NULL))

I don't know what provokes the table to have records with null values occasionally in the CircleCI tests, but that shouldn't be possible.
I hope the reason is just that the test is missing disable_transactional_fixtures!

anyway, using NOT IN operator with null values is asking for problems.
And doing not exists instead fixes it, except for Oracle without doing anything else because BabySqueel does not convert well the not_exists to SQL for Oracle. It can be done using bare SQL for the not_exists as we are doing for BackendApi.not_used_by, but we did that because it was urgent back then but it is a bad idea and we should fix the BabySqueel conversion to SQL of not_exists.
Now I discovered that the problem is just that it attempts to add an 'order by' in the subquery by default but in a weird format that it makes it crash (the error is ORA-00907 missing right parenthesis: SELECT * FROM (SELECT "BACKEND_APIS".* FROM "BACKEND_APIS" WHERE (NOT EXISTS(SELECT "BACKEND_API_CONFIGS".* FROM "BACKEND_API_CONFIGS" WHERE "BACKEND_API_CONFIGS"."BACKEND_API_ID" = "BACKEND_APIS"."ID" ORDER BY "BACKEND_API_CONFIGS"."ID" ASC)) ORDER BY "BACKEND_APIS"."ID" ASC ) WHERE ROWNUM <= 1000), and we do not even need the 'order' at all, so I unscoped the order for the subquery and it works this way.

codecov · 2019-11-22T15:35:03Z

Codecov Report

Merging #1450 into master will decrease coverage by 14.44%.
The diff coverage is 62.5%.

@@             Coverage Diff             @@
##           master    #1450       +/-   ##
===========================================
- Coverage   91.71%   77.27%   -14.45%     
===========================================
  Files        2310     1054     -1256     
  Lines       75215    32293    -42922     
===========================================
- Hits        68983    24953    -44030     
- Misses       6232     7340     +1108

Impacted Files	Coverage Δ
app/lib/fields/fields.rb	`95.42% <ø> (-3.93%)`	⬇️
app/models/backend_api.rb	`88.33% <60%> (-9.95%)`	⬇️
lib/tasks/backend_api.rake	`83.33% <66.66%> (-16.67%)`	⬇️
app/lib/csv/applications_exporter.rb	`16% <0%> (-84%)`	⬇️
app/helpers/vertical_nav_helper.rb	`12.22% <0%> (-83.34%)`	⬇️
test/test_helpers/xml_assertions.rb	`13.98% <0%> (-82.38%)`	⬇️
test/test_helpers/proxy.rb	`20% <0%> (-80%)`	⬇️
app/helpers/buyers/fields_definitions_helper.rb	`25% <0%> (-75%)`	⬇️
app/lib/three_scale/private_module.rb	`25% <0%> (-75%)`	⬇️
app/lib/csv/messages_exporter.rb	`16.66% <0%> (-75%)`	⬇️
... and 1672 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1698897...a4e71ca. Read the comment docs.

- Use not_exists instead of not_in for the possible null values. - Do it through BabySqueel and unscoping the order in the subquery to make it work in Oracle. - Test .orphans with disable_transactional_fixtures! to not have null values like in the real execution.

Martouta · 2019-12-18T14:36:58Z

app/models/backend_api.rb

  scope :not_used_by, ->(service_id) {
-    # TODO: Baby Squeel
-    # It should be:
-    # where.has do
-    #   not_exists BackendApiConfig.by_service(service_id).by_backend_api(BabySqueel[:backend_apis].id).select(:id)
-    # end
-    # And that works for MySQL and Postgres but not Oracle
-    sql_query = <<~SQL
-      (
-        NOT EXISTS (
-          SELECT id
-          FROM backend_api_configs
-          WHERE service_id = ? AND backend_api_configs.backend_api_id = backend_apis.id
-        )
+    where.has do
+      not_exists(
+        BackendApiConfig.except(:order).select(:id)
+          .by_service(service_id)
+          .by_backend_api(BabySqueel[:backend_apis].id)
      )
-    SQL
-    where(sql_query, service_id)
+    end
  }


What generates for Oracle now for BackendApi.not_used_by(service.id).explain :

SELECT "BACKEND_APIS".* FROM "BACKEND_APIS" WHERE ( NOT EXISTS( SELECT "BACKEND_API_CONFIGS"."ID" FROM "BACKEND_API_CONFIGS" WHERE "BACKEND_API_CONFIGS"."SERVICE_ID" = theServiceID AND "BACKEND_API_CONFIGS"."BACKEND_API_ID" = "BACKEND_APIS"."ID" ) )

guicassolato · 2019-12-18T14:43:55Z

app/models/backend_api.rb

-  scope :orphans, -> { where.has { id.not_in(BackendApiConfig.selecting { :backend_api_id }) } }
+  scope :orphans, -> {
+    where.has do
+      not_exists(BackendApiConfig.except(:order).by_backend_api(BabySqueel[:backend_apis].id).select(:id))


This should improve efficiency a lot! Well done!

duduribeiro

Awesome!!

💯 good work 💪

Martouta · 2019-12-18T14:50:36Z

app/models/backend_api.rb

+  scope :orphans, -> {
+    where.has do
+      not_exists(BackendApiConfig.except(:order).by_backend_api(BabySqueel[:backend_apis].id).select(:id))
+    end
+  }


What generates for Oracle now for BackendApi.orphans.explain:

SELECT "BACKEND_APIS".* FROM "BACKEND_APIS" WHERE ( NOT EXISTS( SELECT "BACKEND_API_CONFIGS"."ID" FROM "BACKEND_API_CONFIGS" WHERE "BACKEND_API_CONFIGS"."BACKEND_API_ID" = "BACKEND_APIS"."ID" ) )

Martouta self-assigned this Nov 22, 2019

Martouta changed the title ~~[WIP] Remove outdated comment. It is tested in another file now~~ [WIP] Remove outdated comment + See if CircleCI tests are working properly. Nov 22, 2019

Martouta force-pushed the remove-outdated-comment-fields branch from c7d55b2 to a29cdbf Compare November 22, 2019 15:35

Martouta force-pushed the remove-outdated-comment-fields branch 11 times, most recently from 5a1e532 to 644a829 Compare December 2, 2019 10:15

Martouta changed the title ~~[WIP] Remove outdated comment + See if CircleCI tests are working properly.~~ [WIP] Fix CircleCI failure for: BackendApi#orphans + Remove outdated comment Dec 3, 2019

Martouta changed the title ~~[WIP] Fix CircleCI failure for: BackendApi#orphans + Remove outdated comment~~ [WIP][DO NOT REVIEW - JUST INVESTIGATING SO FAR] Fix CircleCI failure for: BackendApi#orphans + Remove outdated comment Dec 4, 2019

Martouta force-pushed the remove-outdated-comment-fields branch 4 times, most recently from d9f2ffd to 8651319 Compare December 4, 2019 14:16

Martouta force-pushed the remove-outdated-comment-fields branch 5 times, most recently from fe8a2cb to 3637b32 Compare December 16, 2019 01:44

Remove outdated comment. It is tested in another file now

5784bb7

Martouta force-pushed the remove-outdated-comment-fields branch from 3637b32 to 3ed1b2e Compare December 17, 2019 15:41

Martouta force-pushed the remove-outdated-comment-fields branch from 3ed1b2e to a4e71ca Compare December 17, 2019 15:49

Martouta changed the title ~~[WIP][DO NOT REVIEW - JUST INVESTIGATING SO FAR] Fix CircleCI failure for: BackendApi#orphans + Remove outdated comment~~ Fix & Refactor BackendAPI scopes: orphans & not_used_by Dec 17, 2019

Martouta changed the title ~~Fix & Refactor BackendAPI scopes: orphans & not_used_by~~ [WIP] Fix & Refactor BackendAPI scopes: orphans & not_used_by Dec 17, 2019

3scale deleted a comment from duduribeiro Dec 17, 2019

Martouta requested review from duduribeiro, hallelujah and guicassolato and removed request for duduribeiro and hallelujah December 17, 2019 16:04

Martouta changed the title ~~[WIP] Fix & Refactor BackendAPI scopes: orphans & not_used_by~~ Fix & Refactor BackendAPI scopes: orphans & not_used_by Dec 17, 2019

Martouta requested review from a team, guicassolato and hallelujah and removed request for guicassolato and hallelujah December 17, 2019 16:05

hallelujah approved these changes Dec 18, 2019

View reviewed changes

Martouta commented Dec 18, 2019

View reviewed changes

guicassolato reviewed Dec 18, 2019

View reviewed changes

guicassolato approved these changes Dec 18, 2019

View reviewed changes

duduribeiro approved these changes Dec 18, 2019

View reviewed changes

Martouta commented Dec 18, 2019

View reviewed changes

Martouta merged commit 3ca21df into master Dec 18, 2019

Martouta deleted the remove-outdated-comment-fields branch December 18, 2019 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix & Refactor BackendAPI scopes: orphans & not_used_by #1450

Fix & Refactor BackendAPI scopes: orphans & not_used_by #1450

Martouta commented Nov 22, 2019 •

edited

codecov bot commented Nov 22, 2019 •

edited

Martouta Dec 18, 2019 •

edited

guicassolato Dec 18, 2019

duduribeiro left a comment

Martouta Dec 18, 2019

Fix & Refactor BackendAPI scopes: orphans & not_used_by #1450

Fix & Refactor BackendAPI scopes: orphans & not_used_by #1450

Conversation

Martouta commented Nov 22, 2019 • edited

1st commit

The rest

codecov bot commented Nov 22, 2019 • edited

Codecov Report

Martouta Dec 18, 2019 • edited

Choose a reason for hiding this comment

guicassolato Dec 18, 2019

Choose a reason for hiding this comment

duduribeiro left a comment

Choose a reason for hiding this comment

Martouta Dec 18, 2019

Choose a reason for hiding this comment

Martouta commented Nov 22, 2019 •

edited

codecov bot commented Nov 22, 2019 •

edited

Martouta Dec 18, 2019 •

edited