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

Use configured quoter in SQLiteColumn #1712

Merged
merged 2 commits into from Feb 6, 2024
Merged

Conversation

lrobin136
Copy link
Contributor

PR for the issue: #1657

  • Use configured quoter in SQLiteColumn to support injecting non-default quoter

@lrobin136 lrobin136 marked this pull request as ready for review January 9, 2024 14:43
@jzabroski
Copy link
Collaborator

  1. Create an ISQLiteQuoter, and have the existing concrete SQLiteQuoter and whatever one you wish to inject yourself added there. SQLiteGenerator would then take an ISQLiteQuoter, and the AddSQLite() DI helper would inject the concrete SQLiteQuoter by default
    1. This would serve two purposes - allowing others to create their own custom quoter, and also allowing FluentMigrator.Tests assembly to have a clear contract on the scope of regression testing by defining the set of SQLite-applicable IQuoters.
  2. This PR is missing the BinaryGuid formatting logic and associated regression tests, which I thought was the point of the original PR idea.
    1. As discussed previously, add a UseBinaryGuid flag to the existing quoter, and the default constructor for SQLiteQuoter would not break existing clients. Then you would just need a non-ABI breaking change to AddSQLite DI helper, which would allow you to configure UseBinaryGuid = true. The regression tests could then handle quoting both ways. You can see how this is done for Snowflake Quoter in the Snowflake tests, as it regression tests both scenarios.

Overall, this would be the ideal API and maximize community benefit. The changes as presented seem to only immediately benefit your organization, and anyone who then wants to re-implement the same logic.

@lrobin136
Copy link
Contributor Author

lrobin136 commented Jan 11, 2024

Thanks for your feedback.

The new implementation contains the BinaryGiud formatting logic and a regression test that ensures that an inserted GUID can be read again. The guid formatting logic is different from the initial approach because of the convention how guids are stored with SQLite and .NET.

The formatting logic can be configured in the AddSQLite() DI helper using an optional flag.

@jzabroski
Copy link
Collaborator

Thanks. I think this looks good upon initial glance. I plan to look at it in a bit more detail this weekend and see if anything needs fixing, but this is basically what I was expecting.

@jzabroski jzabroski added this to the 5.1.0 milestone Feb 6, 2024
@jzabroski jzabroski merged commit 5163fa3 into fluentmigrator:main Feb 6, 2024
1 check passed
@jzabroski
Copy link
Collaborator

Thanks, I need to fix a breaking change with 5.0.0 related to SQLite STRICT table behavior, and then I will ship 5.1.0 hotfix.

@jzabroski jzabroski self-assigned this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants