Skip to content

Conversation

@aidanharan
Copy link
Contributor

@aidanharan aidanharan commented Oct 10, 2023

Fix for #1099

The issue was caused by case sensitivity when finding a view column's default value following #1073

This PR downcases the column names involved so casing is no longer relevant.

@aidanharan aidanharan changed the base branch from main to 7-0-stable October 10, 2023 14:25
@aidanharan aidanharan changed the title View default function lookup downcase Fix issue with default view value not being found because of case sensitivity Oct 10, 2023
@aidanharan aidanharan marked this pull request as ready for review October 10, 2023 15:31
@aidanharan
Copy link
Contributor Author

@budiljak Could you review this please?

Copy link

@budiljak budiljak left a comment

Choose a reason for hiding this comment

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

I can hardly judge the code changes as I don't know the details of module. But I checked it out and it seems to work for the boolean field.

BUT I just tried it with a default value for a string and that doesn't work. Should I open another issue for that or is this related to that fix here?

@aidanharan
Copy link
Contributor Author

Probably related. Could you add a script that shows the issue for a default value for a string?

@budiljak
Copy link

budiljak commented Oct 11, 2023

Here it is:

require "bundler/inline"

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

  gem "activerecord", "=7.0.4.2"
  gem "activerecord-sqlserver-adapter", git: "https://github.com/rails-sqlserver/activerecord-sqlserver-adapter", ref: "0be80a6565f56c984a74ff11abc2e52dbbae50e2"
  #gem "activerecord-sqlserver-adapter", "=7.0.4"
  #gem "activerecord-sqlserver-adapter", "7.0.3"
end

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

ActiveRecord::Base.establish_connection(
  adapter:  "sqlserver",
  timeout:  5000,
  pool:     100,
  encoding: "utf8",
  database: "test_database",
  username: "SA",
  password: "StrongPassword!",
  host:     "localhost",
  port:     1433,
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  drop_table :bug_test_tables rescue nil

  create_table :bug_test_tables, force: true do |t|
    t.boolean :Bool_field, null: false, default: false
    t.string :string_field, null: false, default: "abc"
  end
  drop_view = "DROP VIEW IF EXISTS bug_tests;"
  create_view = "CREATE VIEW bug_tests AS SELECT id AS id, bool_field AS b, string_field as s FROM bug_test_tables"
  ActiveRecord::Base.connection.execute(drop_view)
  ActiveRecord::Base.connection.execute(create_view)
end

class BugTest < ActiveRecord::Base
end

class TestBugTest < Minitest::Test
  def setup
    # IMPORTANT: partial_inserts is false by default since Rails 7.0
    # without that ActiveRecord will not try to infer default values
    # before creating the record and hence there's no error
    ActiveRecord::Base.partial_inserts = false
    @bug_test = BugTest.new
  end

  
  def test_default_value
    assert_equal false, @bug_test.b
    assert_equal "abc", @bug_test.s
    @bug_test.save!
    assert_equal 1, BugTest.count
  end
end

@budiljak
Copy link

Remark: It's not about case sensitivity here.

@aidanharan
Copy link
Contributor Author

@budiljak The issue with the default string value seems to be a separate issue. It happens in previous versions of the gem before v7.0.4.0. Would you mind opening it as a separate issue and include that script you have above. Thanks

Copy link

@budiljak budiljak left a comment

Choose a reason for hiding this comment

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

Ok.

@aidanharan aidanharan merged commit 0871ed8 into 7-0-stable Oct 12, 2023
@aidanharan aidanharan deleted the view-default-function-lookup-downcase branch October 12, 2023 09:42
aidanharan added a commit that referenced this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants