Skip to content

Make the gem work without ActiveRecord #195

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.

Sign up for GitHub

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

Merged
goodviber merged 7 commits into from
Apr 23, 2025
Merged

Make the gem work without ActiveRecord #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc00450
Merge pull request #1 from DFE-Digital/main
hrmtl Apr 9, 2025
c8c97da
Make the gem work even when ActiveRecord is not available.
hrmtl Apr 10, 2025
8008b76
Update logger message
hrmtl Apr 14, 2025
5724f7b
Remove unrelated gem version updates
hrmtl Apr 14, 2025
7fc0d6a
Remove .DS_Store from gitignore
hrmtl Apr 15, 2025
3acbed0
Fix spec
hrmtl Apr 15, 2025
75b719c
Merge branch 'main' into make-activerecord-optional
goodviber Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions lib/dfe/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,10 @@
require 'i18n'
require 'httparty'
require 'google/cloud/bigquery'
require 'dfe/analytics/activerecord' if defined?(ActiveRecord)
require 'dfe/analytics/event_schema'
require 'dfe/analytics/fields'
require 'dfe/analytics/entities'
require 'dfe/analytics/services/entity_table_checks'
require 'dfe/analytics/services/checksum_calculator'
require 'dfe/analytics/services/generic_checksum_calculator'
require 'dfe/analytics/services/postgres_checksum_calculator'
require 'dfe/analytics/shared/service_pattern'
require 'dfe/analytics/concerns/requestable'
require 'dfe/analytics/event'
Expand All @@ -29,7 +26,6 @@
require 'dfe/analytics/big_query_api'
require 'dfe/analytics/big_query_legacy_api'
require 'dfe/analytics/azure_federated_auth'
require 'dfe/analytics/transaction_changes'
require 'dfe/analytics/api_requests'

module DfE
Expand Down Expand Up @@ -98,19 +94,21 @@ def self.configure
end

def self.initialize!
unless defined?(ActiveRecord)
# bail if we don't have AR at all
Rails.logger.error('ActiveRecord not loaded; DfE Analytics not initialized')
return
end

unless Rails.env.production? || File.exist?(Rails.root.join('config/initializers/dfe_analytics.rb'))
message = "Warning: DfE Analytics is not set up. Run: 'bundle exec rails generate dfe:analytics:install'"
Rails.logger.error(message)
puts message
return
end

if defined?(ActiveRecord)
Copy link
Copy Markdown
Collaborator

@asatwal asatwal Apr 14, 2025

Choose a reason for hiding this comment

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

We may want to introduce a config item, such as send_db_events to control this rather than just relying on ActiveRecord being present or not. TBD.

Copy link
Copy Markdown
Contributor Author

@hrmtl hrmtl Apr 14, 2025

Choose a reason for hiding this comment

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

I did try that but the gem code would throw an error while loading in the Rails app. I agree that a config item would be nice, but maybe we can do it as an improvement in the future.

Copy link
Copy Markdown
Contributor

@goodviber goodviber Apr 15, 2025

Choose a reason for hiding this comment

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

What was the error?

Copy link
Copy Markdown
Contributor Author

@hrmtl hrmtl Apr 15, 2025

Choose a reason for hiding this comment

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

I got a NameError for ActiveRecord when starting the Rails server. I think that happened when the gem code was loaded before the config got read.

Copy link
Copy Markdown
Contributor

@goodviber goodviber Apr 17, 2025

Choose a reason for hiding this comment

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

@asatwal I think we could look at the config issue later, I will have a tilt at it myself. If this has been tested @hrmtl with an app running ActiveRecord we can approve

Copy link
Copy Markdown
Contributor Author

@hrmtl hrmtl Apr 17, 2025

Choose a reason for hiding this comment

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

@goodviber Thanks. I haven't tested it with an app running ActiveRecord. Does the test suite not cover that?

setup_entities
else
Rails.logger.info('ActiveRecord not loaded; DfE Analytics will only track non-database requests.')
end
end

def self.setup_entities
if Rails.version.to_f > 7.1
ActiveRecord::Base.with_connection do |connection|
raise ActiveRecord::PendingMigrationError if connection.pool.migration_context.needs_migration?
Expand Down
7 changes: 7 additions & 0 deletions lib/dfe/analytics/activerecord.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

require 'dfe/analytics/services/entity_table_checks'
require 'dfe/analytics/services/checksum_calculator'
require 'dfe/analytics/services/generic_checksum_calculator'
require 'dfe/analytics/services/postgres_checksum_calculator'
require 'dfe/analytics/transaction_changes'
12 changes: 11 additions & 1 deletion spec/dfe/analytics_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
it 'recovers and logs' do
hide_const('ActiveRecord')

expect(Rails.logger).to receive(:error).with(/ActiveRecord not loaded/)
expect(Rails.logger).to receive(:info).with(/ActiveRecord not loaded; DfE Analytics will only track non-database requests./)
expect { DfE::Analytics.initialize! }.not_to raise_error
end
end
Expand Down Expand Up @@ -330,4 +330,14 @@
expect(DfE::Analytics.all_entities_in_application.first.to_s).to match(/with_model_candidates/)
end
end

describe 'when ActiveRecord is not available' do
before do
hide_const('ActiveRecord')
end

it 'initializes without errors' do
expect { DfE::Analytics.initialize! }.not_to raise_error
end
end
end