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

Interception tests don't normalize SQL #16701

Closed
roji opened this issue Jul 23, 2019 · 6 comments
Closed

Interception tests don't normalize SQL #16701

roji opened this issue Jul 23, 2019 · 6 comments
Assignees
Labels
area-test closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. punted-for-3.1 type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Jul 23, 2019

The new interception tests send un-normalized SQL to the database, so identifiers get cast to lowercase in PostgreSQL. Pass through RelationalTestStore.NormalizeDelimetersInRawString.

/cc @ajcvickers

@roji roji added the area-test label Jul 23, 2019
@roji roji self-assigned this Jul 23, 2019
@ajcvickers
Copy link
Member

@roji What's the functional impact of this? (I intentionally wrote the tests with what I thought would be universally acceptable SQL to reduce the need for provider-specific validation in the tests.)

@roji
Copy link
Member Author

roji commented Jul 23, 2019

PostgreSQL as the peculiar feature of folding unquoted identifiers to lowercase. So running these tests fails because a table with the name singularity couldn't be found - the uppercase S got lost.

We've encountered this several times over the years :) From the PostgreSQL point of view, universally acceptable SQL would contain only all-lowercase identifiers - but that may be a ugly. On the other hand the FromSql tests already utilize a normalization thing the turns square brackets to whatever the provider needs, so if you write [Singularity] in your test, it will be converted to "Singularity" and all is well.

@ajcvickers
Copy link
Member

@roji Specifically which tests are failing? (I think we might be talking cross-purposes.)

@roji
Copy link
Member Author

roji commented Jul 24, 2019

Here are the failing tests (all from CommandInterceptionTestBase):

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_one_app_and_one_injected_interceptor(bool async) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_passively(bool async, bool inject) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_to_mutate_command(bool async, bool inject) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_to_replace_execution(bool async, bool inject) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_to_replaceresult(bool async, bool inject) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_with_explicitly_composed_app_interceptor(bool async) => null;

[ConditionalTheory(Skip = "https://github.com/aspnet/EntityFrameworkCore/issues/16701")]
public override Task Intercept_non_query_with_two_injected_interceptors(bool async) => null;

All fail because of SQL with an unquoted Singularity table name. This should all be easy to fix, just wanted to scope it out for now.

BTW these tests seem like they have less value as spec tests, as there's nothing providers need to actually do for them to work. In other words, they're exercising a core feature, but they need to exercise it on some provider, which is why they're in the spec tests. Obviously no big deal, but if we wanted to we could have a separate test project for this kind of thing, to keep the spec test burden on providers lower.

@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog Jul 26, 2019
@ajcvickers ajcvickers self-assigned this Jul 26, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 3.1.0 Sep 4, 2019
@ajcvickers ajcvickers modified the milestones: 3.1.0, Backlog Oct 11, 2019
@lajones lajones self-assigned this Feb 7, 2020
@lajones
Copy link
Contributor

lajones commented Feb 10, 2020

Fixed with #19837. @roji Do you want me to leave this open to remind you to update the npgsql test code above?

@roji
Copy link
Member Author

roji commented Feb 10, 2020

Nope - ok to close, am tracking this on my side too.

@lajones lajones closed this as completed Feb 12, 2020
@lajones lajones added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 12, 2020
@lajones lajones modified the milestones: Backlog, 5.0.0 Feb 12, 2020
roji added a commit to roji/efcore.pg that referenced this issue Feb 17, 2020
* Convention change from EFCore 3574cfbddfdf485548b9e217f8e01a5ffeaf8562
* Unskipped command interception tests (dotnet/efcore#16701)
* Un-overridden test (dotnet/efcore#19855)
roji added a commit to npgsql/efcore.pg that referenced this issue Feb 17, 2020
* Convention change from EFCore 3574cfbddfdf485548b9e217f8e01a5ffeaf8562
* Unskipped command interception tests (dotnet/efcore#16701)
* Un-overridden test (dotnet/efcore#19855)
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-test closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. punted-for-3.1 type-bug
Projects
None yet
Development

No branches or pull requests

3 participants