running with a non "dbo" schema (Rails 5.2.4)

KDGundermann commented 2 years ago

Issue

I'm trying to migrate my application from Rails 4.2 to 5.2 The application is accessing a SQL-Server with an existing database from our ERP system The user we are using has a default schema 'lager' where some views for my application are defined Most of my test are failing after the upgrade

When starting the Rails application :

ActiveRecord::ModelSchema::ClassMethods runs load_schema!
ActiveRecord::ConnectionAdapters::SchemaCache runs 'columns(table_name)'
ActiveRecord::ConnectionAdapters::SQLServer::SchemaStatements runs 'view_exists?(table_name)' which calls
data_source_sql(view_name, type: "VIEW") which calls
quoted_scope(name, type) and quoted_scope sets the schema to "dbo" as the simple view name has no explicit schema source line

so it is caching all table/view names from schema 'dbo' and therefore can not request column_infos from schema 'lager' ( which gives you a 'Table doesn't exist' if you try to access the table in the rails console )

Tried to fix

by setting ActiveRecord::Base.table_name_prefix = 'lager'

Suggested fix

when connecting to the database run SELECT schema_name() once and store it as the default_schema name for this connection and use it in quoted_scope

Details

Rails version: 5.2.4.4
SQL Server adapter version: 5.2.4.4
TinyTDS version: 1.3.6

wpolicarpo commented 2 years ago

@KDGundermann do you mind creating a minimal reproducible script following these steps or maybe a test app highlighting the issue?

That version of the adapter is not supported anymore (README is wrong) but I'd be happy to review a PR, guide you through a solution or investigate the issue myself further if you could help with a reproducible example.

KDGundermann commented 2 years ago

Hi, I am trying to prepare a pull request, but currently struggling how to write a test. For testing I would need a second user with a different default schema. Currently there is only the rails user. Could I create a temporary login/user in the tests ?? How can I create another (temporary) connection with this new user ?

I would like to test CRUD operations on a table with the same name as one existing in the dbo schema. Looking at sqlserver_specific_schema there is already as table sst_schema_columns. May I use this ?

wpolicarpo commented 2 years ago

Why do you need a second user?

If you really need something like that than you could use test/schema/sqlserver_specific_schema.rb to prepare your test and add tests to test/cases/schema_test_sqlserver.rb.

KDGundermann commented 2 years ago

I need a user with a different, non "dbo" schema, as currently the code assumes every user is in the dbo-schema.

aidanharan commented 2 years ago

The same user can use multiple different schemas. For example, if you look at the test in https://github.com/DCrow/activerecord-sqlserver-adapter/commit/78ba894766ba615ed6a2d055936866b283b4c49c you can see that a single user is able to create schemas dbo1 and dbo2 and can create tables in each of them.

If you follow the approach taken in this test can you create a reproducible script that demonstrates your issue?

aidanharan commented 2 years ago

@KDGundermann I had a quick look at https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1008. Is the issue you are having something like the following.

The database has table called dbo.accounts.
The database has a view called dbo2.accounts, which is a view of the records in the dbo.accounts table.
You want to create a ActiveRecord model that will query the dbo2.accounts view. I assume there is logic in the view which explains why you don't want to query the dbo.accounts table instead.

KDGundermann commented 2 years ago

I assume there is logic in the view which explains why you don't want to query the dbo.accounts table instead.

Yes there is some logic

the database is part of our ERP (Enterprise Ressource Planning) software
the Rails application is only an addon , which should only see a small subset of all the tables
the Rails application has some tables and views which are private to the rails application and do not belong into the dbo schema
so the db user for the Rails application has a non-dbo schema as default but the activerecord-adapter assumes it is always dbo
btw. the Rails application was created with Rails3 in 2011 and I am now trying to upgrade from Rails4 -> Rails5

aidanharan commented 2 years ago

@KDGundermann You should be able to use table_name_prefix to query the view in the other schema. I have created https://gist.github.com/aidanharan/08f58700b1b965c872ce1c0895926ddc to demonstrate how a model can be configured to query a view in a different schema from the original table.

KDGundermann commented 2 years ago

Hi Aiden, thank you for preparing the gist. But you are still using the user 'rails' which has "dbo" as default schema. Please use another user which has a non-dbo schema as default (have a look at my tests)

aidanharan commented 2 years ago

@KDGundermann From my understanding the default schema of the user shouldn't prevent you from querying tables/views in other schemas. Please see https://gist.github.com/aidanharan/056606b0ac89b2440b6db967997f0d19

In the gist you can see that:

The default schema of the 'rails' user is dbo.
The table dbo1.accounts can be queried using ActiveRecord by configuring the model's table_name_prefix.
The view dbo2.accounts (which is a view of the dbo1.accounts table) can be queried using ActiveRecord by configuring the model's table_name_prefix.

Is there a specific reason why using table_name_prefix in this way will not work with your existing database?

KDGundermann commented 2 years ago

The default schema of the 'rails' user is dbo.

Yes, and the current tests are always running with this user in 'dbo'

But when you have another user 'xxx' in schema 'test' you will have problems. (e.g. when inserting data)

Try to create another user with CREATE LOGIN [activerecordtestuser] WITH PASSWORD = '', CHECK_POLICY = OFF, DEFAULT_DATABASE = [activerecord_unittest] USE [activerecord_unittest] DROP USER IF EXISTS [activerecordtestuser] CREATE USER [activerecordtestuser] FOR LOGIN [activerecordtestuser] WITH DEFAULT_SCHEMA = test GRANT SELECT, INSERT, UPDATE, DELETE ON SCHEMA :: test TO [activerecordtestuser] GRANT VIEW DEFINITION TO [activerecordtestuser] (or try to run the first commit from my pull request)

And what is the problem in changing the line scope[:schema] = identifier.schema || 'dbo' to scope[:schema] = identifier.schema || @default_schema ???

aidanharan commented 2 years ago

The issue with changing the line scope[:schema] = identifier.schema || 'dbo' is that other users of the SQL Server adapter could be relying on the schema being defaulted to dbo instead of the user's default schema. Changing that one line could fix your issue but break other peoples applications. So without a strong argument for the change then it cannot be made.

I ran your tests in https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1008 and to make them pass you just need to include the schema in your table_exists? and view_exists? calls.

    it 'tables and views in different schemas are visible' do
      assert @conn2.table_exists?('dbo.accounts'), "table 'accounts' (in dbo) is visible"
      assert !@conn2.view_exists?('dbo.accounts'), "view 'accounts' (in dbo) does not exist"

      assert @conn2.view_exists?('test.accounts'), "view 'accounts' (in test) is visible"
      assert !@conn2.table_exists?('test.accounts'), "table 'accounts' (in dbo) does not exist"
    end

But when you have another user 'xxx' in schema 'test' you will have problems. (e.g. when inserting data)

Not sure what problems you mean. The gist https://gist.github.com/aidanharan/056606b0ac89b2440b6db967997f0d19 shows a user can insert records into a table that is not in their default schema. Could you give tests that demonstrate the problems?

Note: Support for Rails 5.2 of the SQL Server adapter has ended. So if there is an issue with it, you may need to monkey-patch the adapter rather than relying on a new release of it.

KDGundermann commented 2 years ago

Just a small modification to your gist: (https://gist.github.com/KDGundermann/13f0123ca7a0562cc69a3c7ca14c4a86)

using gem "activerecord-sqlserver-adapter", "6.1"

aidanharan commented 2 years ago

The tests in your gist (https://gist.github.com/KDGundermann/13f0123ca7a0562cc69a3c7ca14c4a86) were failing because you hadn't set the primary key of the Account model. See https://gist.github.com/aidanharan/fb933eed08191ff46fcc38380a379332

class Account < ActiveRecord::Base
  self.table_name = "active_accounts"
  self.table_name_prefix = 'dbo2.'

  self.primary_key = 'account_id'
end

KDGundermann commented 2 years ago

It would neither be neccessary to set self.table_name_prefix = 'dbo2.' nor self.primary_key = 'account_id' as the activerecord-sqlserver-adapter would normally handle this fine.

But there is this single line: scope[:schema] = identifier.schema || 'dbo' which causes that the code is looking for a view "dbo.active_accounts" which does NOT exist and so can NOT determine the primary key.

And your argument 'Changing that one line could fix your issue but break other peoples applications' is moot, as this line would change NOTHING for people runing their code in dbo. And also this line has already broken my code which is running fine with activerecord-sqlserver-adapter 4.x

So I understand that you are not willing to change a single line of code to remove a bug but only suggesting workarounds. I've never expected this..

rails-sqlserver / activerecord-sqlserver-adapter