rails / rails

Ruby on Rails
https://rubyonrails.org
MIT License
55.81k stars 21.6k forks source link

Unwanted SQL query and Rails Callback function called when Running ‘find_by_sql’ with LEFT JOIN #51715

Closed limjinsun closed 5 months ago

limjinsun commented 5 months ago

Steps to reproduce

User.find_by_sql("SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id;”)

Expected behavior

It should return array of User Model.

Actual behavior

It executed another SQL query, also executed some Rails callback..

irb(main):044:0> User.find_by_sql("SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id;")
  User Load (4.2ms)  SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id;
  Client Load (0.7ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 11 ORDER BY `clients`.`id` DESC LIMIT 1
  Company Load (0.3ms)  SELECT `companies`.* FROM `companies` WHERE `companies`.`user_id` = 11 LIMIT 1
  UserBalance Load (0.2ms)  SELECT `user_balances`.* FROM `user_balances` WHERE `user_balances`.`user_id` = 11 AND `user_balances`.`currency` = 'EUR' ORDER BY `user_balances`.`id` DESC LIMIT 1
  Client Load (0.5ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 1 ORDER BY `clients`.`id` DESC LIMIT 1
  Client Load (0.4ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 1 AND (currency = 'EUR') ORDER BY `clients`.`id` DESC LIMIT 1
  ContactDetails Load (0.3ms)  SELECT `contact_details`.* FROM `contact_details` WHERE `contact_details`.`user_id` = 1 AND `contact_details`.`contact_details_type` = 'personal' LIMIT 1
  UserBalance Load (0.4ms)  SELECT `user_balances`.* FROM `user_balances` WHERE `user_balances`.`user_id` = 1 AND `user_balances`.`currency` = 'EUR' ORDER BY `user_balances`.`id` DESC LIMIT 1
METHOD : "get"
URL : "https://some-api-***************************"
HEADERS : {
  "content_type": "application/json"
}
-=-=-=-=-=-=-
RESPONSE CODE: 200
RESPONSE: {

System configuration

Rails version:

irb(main):054:0> Rails.version => “7.0.8"

Ruby version:

❯ ruby -v ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-darwin21]

fatkodima commented 5 months ago

Can you provide a reproduction test script using https://github.com/rails/rails/blob/main/guides/bug_report_templates/active_record.rb template?

limjinsun commented 5 months ago

@fatkodima I ran the bug template. but I can’t reproduce the error.

# Running:

D, [2024-05-02T16:12:54.318104 #48134] DEBUG -- :   User Load (0.3ms)  SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id
.

Finished in 0.001494s, 669.3442 runs/s, 669.3442 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

As you can see, It ran only one sql. => which is correct.

Then how come in my case, it ran all other sql queries.

  User Load (4.2ms)  SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id;
  Client Load (0.7ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 11 ORDER BY `clients`.`id` DESC LIMIT 1
  Company Load (0.3ms)  SELECT `companies`.* FROM `companies` WHERE `companies`.`user_id` = 11 LIMIT 1
  UserBalance Load (0.2ms)  SELECT `user_balances`.* FROM `user_balances` WHERE `user_balances`.`user_id` = 11 AND `user_balances`.`currency` = 'EUR' ORDER BY `user_balances`.`id` DESC LIMIT 1
  Client Load (0.5ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 1 ORDER BY `clients`.`id` DESC LIMIT 1
  Client Load (0.4ms)  SELECT `clients`.* FROM `clients` WHERE `clients`.`user_id` = 1 AND (currency = 'EUR') ORDER BY `clients`.`id` DESC LIMIT 1
  ContactDetails Load (0.3ms)  SELECT `contact_details`.* FROM `contact_details` WHERE `contact_details`.`user_id` = 1 AND `contact_details`.`contact_details_type` = 'personal' LIMIT 1
  UserBalance Load (0.4ms)  SELECT `user_balances`.* FROM `user_balances` WHERE `user_balances`.`user_id` = 1 AND `user_balances`.`currency` = 'EUR' ORDER BY `user_balances`.`id` DESC LIMIT 1
fatkodima commented 5 months ago

I am confident that the problem is in the app itself, not in the rails. Try to ask on stackoverflow for the help. So closing for now. If you will be able to provide a reproducible test script, we will reopen.

limjinsun commented 5 months ago

@fatkodima Yes, you were right. But I would like to make some comment on here. So that next person can find the issue easily.

When you run,

User.find_by_sql("SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id;”)

find_by_sql with "JOIN" query, It will look up every column of the table in database. In my case, it's user table and user_balances table.

The Trick thing is, when you have method in your ActiveRecord class that has same name with db column.

My user_balances table has something called "mambu_id" column. and my User ActiveRecord Class has method name same as "mambu_id"

class User < ActiveRecord::Base

  ...
  def mambu_id
    # some logic here to call, another sql, and other methods..
  end
  ...

end

I wanted call something like this.

User.find_by_sql("
          SELECT
            *
          FROM
            users
          LEFT JOIN user_balances ON user_balances.user_id = users.id
          WHERE
            users.user_type = 'Lender' AND users.deleted_at IS NULL AND((user_balances.balance > 0) 
          OR 
          users.id IN( 
            SELECT DISTINCT savings.user_id FROM savings 
            LEFT JOIN investments ON investments.saving_id = savings.id 
            WHERE investments.status = 'completed' OR savings.status = 5)
          )
          ORDER BY
            users.id;
        ")

But I can not. Because it will trigger the method on ActiveRecord class that has same name with db column of JOIN result.

Why should rails behave this way? when running 'find_by_sql' , is it better to just use column value ignoring method in the class? Obviously, User want to use RAW sql query. as name is "find_by_sql"

What do you think? @fatkodima

fatkodima commented 5 months ago

From the docs https://api.rubyonrails.org/classes/ActiveRecord/Querying.html#method-i-find_by_sql

Executes a custom SQL query against your database and returns all the results. The results will be returned as an array, with the requested columns encapsulated as attributes of the model you call this method from.

select * selects all the columns from the joined tables and makes them accessible via accessors on the User. Its unfortunate that you have redefined that accessor method. So, either do a select with alias select *, foo as bar or remove (or rename) that method from User.

limjinsun commented 5 months ago

Ok. thanks for the reply.

by the way. I was trying to write test case for you. I was not able to. Is there anything skip in MiniTest Framework?

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3", "~> 1.4"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema[7.0].define(version: 2024_03_22_154820) do
  create_table "users", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
    t.string "email", default: "", null: false
    t.string "encrypted_password", default: "", null: false
    t.string "first_name"
    t.string "last_name"
    t.datetime "birthday", precision: nil
    t.string "reset_password_token"
    t.datetime "reset_password_sent_at", precision: nil
    t.integer "sign_in_count", default: 0, null: false
    t.datetime "current_sign_in_at", precision: nil
    t.datetime "last_sign_in_at", precision: nil
    t.string "current_sign_in_ip"
    t.string "last_sign_in_ip"
    t.datetime "created_at", precision: nil, null: false
    t.datetime "updated_at", precision: nil, null: false
    t.string "avatar"
    t.string "confirmation_token"
    t.boolean "is_confirmed", default: false
    t.string "invite_loan"
    t.integer "title", default: 0
    t.string "confirmation_email"
    t.string "user_type", null: false
    t.string "user_subtype"
    t.datetime "remember_created_at", precision: nil
    t.string "business_name"
    t.string "middle_name", limit: 300
    t.string "currency", limit: 5
    t.boolean "identity_checked"
    t.boolean "from_invitation", default: false
    t.integer "invited_by"
    t.boolean "director", default: false
    t.boolean "preferred", default: false
    t.string "referral_id", limit: 32
    t.datetime "confirmed_at", precision: nil
    t.datetime "confirmation_sent_at", precision: nil
    t.string "unconfirmed_email"
    t.datetime "password_updated_at", precision: nil
    t.datetime "deleted_at", precision: nil
    t.datetime "balance_activated_at", precision: nil
    t.datetime "personal_details_completed_at", precision: nil
    t.integer "pension_trustee_id"
    t.integer "company_role_id"
    t.boolean "is_spv", default: false
    t.string "heared_from", default: "other"
    t.boolean "withdraw_once_per_month", default: true
    t.integer "broker_id"
    t.datetime "removal_requested_at", precision: nil
    t.datetime "referral_activated_at", precision: nil
    t.string "originator_id", default: "direct"
    t.boolean "can_topup", default: true
    t.boolean "mca_approved", default: false
    t.boolean "secondary_market_approve", default: false
    t.integer "failed_attempts", default: 0
    t.string "unlock_token"
    t.datetime "locked_at", precision: nil
    t.boolean "deactivated", default: false
    t.boolean "block_withdrawal", default: false
    t.string "device_id"
    t.string "registration_source"
    t.string "status"
    t.string "totp_secret_key"
    t.text "device_token"
    t.string "operating_system"
    t.integer "otp_count"
    t.datetime "otp_disabled_time", precision: nil
    t.datetime "last_otp_validated_at", precision: nil
    t.boolean "block_autoflend", default: false
  end

  create_table "user_balances", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
    t.string "currency"
    t.string "mambu_id"
    t.string "encoded_key"
    t.integer "user_id"
    t.datetime "created_at", precision: nil, null: false
    t.datetime "updated_at", precision: nil, null: false
    t.decimal "balance", precision: 15, scale: 4
  end
end

## Active Model
class User < ActiveRecord::Base
  def encoded_key
    raise "error"
  end

  def mambu_id
    raise "error"
  end

  def balance
    raise "error"
  end
end

class UserBalance < ActiveRecord::Base
end

## Test
class BugTest < Minitest::Test

  def test_it
    user1 = User.create!({
      "email"=>"mca-investor@email.com",
      "encrypted_password"=>"$2a$11$1jXOe2G2Z6vD5Xx3MQ9u2uTnaTzYGIM7T66uVmJf774BBftqwFWBO",
      "first_name"=>"MCA-Investor",
      "last_name"=>"Last",
      "birthday"=>nil,
      "reset_password_token"=>nil,
      "reset_password_sent_at"=>nil,
      "sign_in_count"=>2,
      "current_sign_in_ip"=>"127.0.0.1",
      "last_sign_in_ip"=>"127.0.0.1",
      "avatar"=>nil,
      "confirmation_token"=>nil,
      "is_confirmed"=>false,
      "invite_loan"=>nil,
      "title"=>"mr",
      "confirmation_email"=>nil,
      "user_type"=>"Lender",
      "user_subtype"=>"individual_lender",
      "remember_created_at"=>nil,
      "business_name"=>nil,
      "middle_name"=>nil,
      "currency"=>nil,
      "identity_checked"=>true,
      "from_invitation"=>false,
      "invited_by"=>nil,
      "director"=>false,
      "preferred"=>false,
      "referral_id"=>"FLEN11380L554",
      "confirmed_at"=>nil,
      "confirmation_sent_at"=>nil,
      "unconfirmed_email"=>nil,
      "deleted_at"=>nil,
      "personal_details_completed_at"=>nil,
      "pension_trustee_id"=>nil,
      "company_role_id"=>nil,
      "is_spv"=>false,
      "heared_from"=>"other",
      "withdraw_once_per_month"=>true,
      "broker_id"=>nil,
      "removal_requested_at"=>nil,
      "originator_id"=>"admin_panel",
      "can_topup"=>true,
      "mca_approved"=>true,
      "secondary_market_approve"=>false,
      "failed_attempts"=>0,
      "unlock_token"=>nil,
      "locked_at"=>nil,
      "deactivated"=>false,
      "block_withdrawal"=>false,
      "device_id"=>nil,
      "registration_source"=>nil,
      "status"=>nil,
      "totp_secret_key"=>nil,
      "device_token"=>nil,
      "operating_system"=>nil,
      "otp_count"=>nil,
      "otp_disabled_time"=>nil,
      "last_otp_validated_at"=>nil,
      "block_autoflend"=>false
    })

    user_balance1 = UserBalance.create!({
     "currency"=>"EUR",
     "mambu_id"=>"BAL_JJNA253",
     "encoded_key"=>"8a8587588b213a0f018b23cf5c330ab4",
     "user_id"=>1,
     "balance"=>0.8605e6
    })

    assert_raises(StandardError) do
      # This should raise "error". but did not. 
      User.find_by_sql("SELECT*FROM users LEFT JOIN user_balances ON user_balances.user_id=users.id") 
    end
  end

end