-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Unexpected behavior when calling Model.find_by_id
with true
or false
#51727
Comments
@olepalm I believe this behavior is expected since there are test cases for it in ActiveModel Integer, where we serialize boolean values to integers. rails/activemodel/test/cases/type/integer_test.rb Lines 41 to 45 in 343e781
We need to await comments from the Rails core team regarding whether this behavior is expected or necessitates a fix. If a fix is indeed required, then we'll need to update the serialize method accordingly. I'll submit a pull request once we receive confirmation from the Rails maintenance team. rails/activemodel/lib/active_model/type/integer.rb Lines 65 to 68 in 343e781
to def serialize(value)
return if value.is_a?(::TrueClass) || value.is_a?(::FalseClass)
return if value.is_a?(::String) && non_numeric_string?(value)
ensure_in_range(super)
end Reproduce Script: # frozen_string_literal: true
require "bundler/inline"
require "debug"
gemfile(true) do
source "https://rubygems.org"
# 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.define do
create_table :posts do |t|
t.string :title
t.timestamps
end
end
class Post < ActiveRecord::Base
end
class BugTest < Minitest::Test
def test_find_methods
post = Post.new(title: "Example Post")
post.save
post = Post.last
assert_nil Post.find_by(id: true)
assert_nil Post.find_by_id(true)
assert_raises do
Post.find(true)
end
end
end |
@byroot / @fatkodima Does this need to be fixed, or is it the desired behavior? |
I'd say ideally it should be fixed, but I fear this may break apps, and I'm not convinced this is a big enough problem to take that risk. If you have time it would be worth to use git blame to find the commit that introduced this behavior to see if there was any historical reason to do so. |
Some databases didn't had boolean, so you needed to transform them in integer. That is the reason for this behavior. SQLite is one of them
|
@rafaelfranca, I believe the issue lies in how the The complication arises with databases like SQLite, lacking a distinct Boolean storage class. Consequently, we're converting Boolean values to integers. When passing a Boolean to these methods, incorrect results occur: True returns records with id equal to 1. Indeed, the desired behavior would be for the methods to return nil when passed a Boolean value. However, currently, they incorrectly return records with id equal to 1 for True and 0 for False |
Steps to reproduce
Assuming a model with id 1 exists in the database
Expected behavior
Empty result (
nil
), as no Model hastrue
as id.More or less the same result as one would get if no records with id 1 exists.
These changes should also be reflected in
Model.find(true)
and similar find methods. (Model.find_by(:id => true)
)Alternatively, if this behavior is intended to support databases without explicit boolean data types (like MySQL), maybe the documentation on https://api.rubyonrails.org/classes/ActiveModel/Type/Integer.html should be updated.
Specifically
Which is not the behavior observed.
Actual behavior
true
is converted to1
, which results in query like this:Model Load (1.2ms) SELECT "models".* FROM "models" WHERE "models"."id" = $1 LIMIT $2 [["id", 1], ["LIMIT", 1]]
Which is somewhat surprising, as instead of returning
nil
you may get an instance ofModel
withid
== 1, which is usually very likely to exist.System configuration
Rails version: 7.1.3.2
Ruby version: 3.2.0
The text was updated successfully, but these errors were encountered: