Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate orphan objects #3732

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nidhi-soni1104
Copy link
Contributor

@nidhi-soni1104 nidhi-soni1104 commented Mar 21, 2024

What this PR does / why we need it:

We need a rake task to go over all tables with the tenant_id field and make check whether that tenant still exists, reporting and optionally removing these on the way.

complete description is in JIRA

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-10855

Note: I have commented the deleted line for now but added puts so whoever will run it while reviewing can see the puts

@nidhi-soni1104 nidhi-soni1104 self-assigned this Mar 21, 2024
@nidhi-soni1104
Copy link
Contributor Author

@akostadinov Updated

@@ -6,72 +6,27 @@ namespace :database do
puts 'Checking and removing orphaned objects...'

# Tables to exclude from orphaned objects check
excluded_tables = ['accounts', 'audits', 'categories', 'category_types', 'cms_templates', 'legal_term_acceptances', 'legal_term_bindings',
'legal_term_versions', 'proxy_logs', 'schema_migrations', 'service_cubert_infos', 'slugs', 'taggings', 'tags']
excluded_tables = ['cms_templates']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exclude this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this exclusion can be removed


if orphaned_objects.exists?
puts "Found orphaned objects in #{model.table_name}:"
orphaned_objects.each { |obj| puts "- ID: #{obj.id}, Tenant ID: #{obj.tenant_id}" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use find_each here instead of each to use batching.

@github-actions github-actions bot added the Stale label May 11, 2024
@3scale 3scale deleted a comment from github-actions bot May 11, 2024
@github-actions github-actions bot removed the Stale label May 12, 2024
@jlledom jlledom force-pushed the THREESCALE-10855-eliminate-orphan-objects branch from 89a470f to 8e47c60 Compare June 10, 2024 12:30
@jlledom
Copy link
Contributor

jlledom commented Jun 10, 2024

I rebased this, It was too old.

Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A wrote a couple of comments but this LGTM. I haven't tried to run the script locally though.

It would be good to write a couple of tests. Do we have tests for rake tasks?

@@ -0,0 +1,32 @@
# frozen_string_literal: true

namespace :database do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use the db namespace? to be consistent with all other DB tasks

@@ -6,72 +6,27 @@ namespace :database do
puts 'Checking and removing orphaned objects...'

# Tables to exclude from orphaned objects check
excluded_tables = ['accounts', 'audits', 'categories', 'category_types', 'cms_templates', 'legal_term_acceptances', 'legal_term_bindings',
'legal_term_versions', 'proxy_logs', 'schema_migrations', 'service_cubert_infos', 'slugs', 'taggings', 'tags']
excluded_tables = ['cms_templates']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this exclusion can be removed


provider_account_ids = Account.where(provider: true).pluck(:id)

ActiveRecord::Base.descendants.each do |model|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better this IMO:

Suggested change
ActiveRecord::Base.descendants.each do |model|
ApplicationRecord.descendants.each do |model|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants