Skip to content
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

Let sqlite strict config read from database.yml or config #45373

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

ghiculescu
Copy link
Member

Per this discussion: #45346 (comment)

The adapter should read from database.yml, if a value is provided for strict. If no value is provided then it should fall back to config.active_record.strict_strings_by_default.

cc @fatkodima

Per this discussion: rails#45346 (comment)

The adapter should read from `database.yml`, if a value is provided for `strict`. If no value is provided then it should fall back to `config.active_record.strict_strings_by_default`.

Co-authored-by: fatkodima123 <fatkodima123@gmail.com>
@ghiculescu
Copy link
Member Author

The flakey tests seem unrelated, they aren't on the SQlite adapter.

@yahonda
Copy link
Member

yahonda commented Jun 16, 2022

Restarted flaky two jobs.

What do you think about removing strict: below?

sqlite3:
arunit:
database: <%= FIXTURES_ROOT %>/fixture_database.sqlite3
timeout: 5000
strict: true
arunit2:
database: <%= FIXTURES_ROOT %>/fixture_database_2.sqlite3
timeout: 5000
strict: true

@ghiculescu
Copy link
Member Author

I think it’s fine to remove. @fatkodima agree?

@fatkodima
Copy link
Member

It is used, as described here #45346 (comment). sqlite3_adapter_strict_strings_by_default is false, so we need to somehow test active record that it is working when this mode is enabled. For example, code for check constraints was previously broken in this mode.

For example, you can revert locally the changes in this file - https://github.com/rails/rails/pull/45346/files#diff-a583ecafedcde366950515cba5e7a808c51d85688d3b1de18fafe69f92838899 and set strict: true in a database.yml in a sample app and observe an error.

@ghiculescu
Copy link
Member Author

Ohh right, now I follow. @yahonda FYI, we should keep the config as is.

@byroot
Copy link
Member

byroot commented Jun 16, 2022

Looks good to me. @ghiculescu is this missing anything?

@fatkodima
Copy link
Member

@byroot This is good to merge.

@byroot byroot merged commit 2077524 into rails:main Jun 16, 2022
@ghiculescu ghiculescu deleted the sqlite3-strict-database-yml branch June 16, 2022 20:04
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.

None yet

4 participants