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

When reporting PendingModelChangesWarning, look into including information on the model discrepancy #35292

Closed
pdevito3 opened this issue Dec 8, 2024 · 10 comments
Assignees
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@pdevito3
Copy link

pdevito3 commented Dec 8, 2024

@roji as discussed in #35285 i was able to make a small project to reproduce the PendingModelChangesWarning and it's not even clear what the cause is (potentially my biggest gripe with this almost blocking error).

To repro, clone down this project and run the integration tests.

For background, all i have in my web api is a db context with one very simple entity (base entity doesn't even have a guid default).

The integration tests are setup with the TestFixture to spin up a testing db and run migrations. Each test then gets a service collection to use for testing. This has worked for years for me, but .NET 9 breaks it

If you want to run the web api, you can do a docker compose to spin up a PG db and just run the api project. Migrations will automatically apply

Happy to answer any questions or iterate ont his with you

@pdevito3 pdevito3 changed the title Unclear PendingMOdelChangesWarning in .NET 9 Unclear PendingModelChangesWarning in .NET 9 Dec 8, 2024
@stevendarby
Copy link
Contributor

Lack of UseSnakeCaseNamingConvention in tests?

@pdevito3
Copy link
Author

pdevito3 commented Dec 9, 2024

Lack of UseSnakeCaseNamingConvention in tests?

Not sure what you mean? The tests just call a migration. The context setup is using the same service registration as the api

@stevendarby
Copy link
Contributor

Is it?

https://github.com/pdevito3/EfDotnetNineRepro/blob/main/RecipeManagement/tests/RecipeManagement.IntegrationTests/TestFixture.cs#L46

It's created here, with options that don't have the convention

@pdevito3
Copy link
Author

pdevito3 commented Dec 9, 2024

Is it?

https://github.com/pdevito3/EfDotnetNineRepro/blob/main/RecipeManagement/tests/RecipeManagement.IntegrationTests/TestFixture.cs#L46

It's created here, with options that don't have the convention

Sorry, commented pre-coffee while distracted with the kiddo. You're totally right and that does resolve it 🍻

With that said, still going to leave this open for now because I'd say the root ask of needing more clarity for errors like this is still there now that we have a blocking error here we'd otherwise need to escape with new code without knowing the root cause.

@roji roji changed the title Unclear PendingModelChangesWarning in .NET 9 When reporting PendingModelChangesWarning, look into including information on the model discrepancy Dec 9, 2024
@roji
Copy link
Member

roji commented Dec 9, 2024

@pdevito3 I'm simply not sure it's feasible for EF to automatically determine that you forgot to do UseSnakeCaseNamingConvention...

We may be able to include some information (e.g. what the migration operation would be if we were to generate it), but that would need some more thinking.

@pdevito3
Copy link
Author

pdevito3 commented Dec 9, 2024

@pdevito3 I'm simply not sure it's feasible for EF to automatically determine that you forgot to do UseSnakeCaseNamingConvention...

We may be able to include some information (e.g. what the migration operation would be if we were to generate it), but that would need some more thinking.

@roji I'm not necessarily thinking that specific but more-so calling out the change it sees. If I'm understanding right, it's doing a diff to see if there are changes, so maybe it could show that deviation so we at least have a hint of what's happening?

@roji
Copy link
Member

roji commented Dec 9, 2024

Maybe that's feasible... This is something we're discussing internally. We can indeed use this issue for further comments on this.

@roji
Copy link
Member

roji commented Dec 10, 2024

We discussed this in the team. Basically, any information we include here would be similar (but worse!) than the information the user would get by simply adding a migration, and examining that migration to understand what the model discrepancy is. 9.0.1 will also contain #35221, which provides better exception messages around this error, and also specifically guides the user to add a migration as a way of understanding what's going on.

So while in theory we could add some info on the specific discrepancy into the error message, that would involve adding a string representation to all of our migration operations (e.g. CreateTableOperation), which is quite an effort and probably too much code to include in a patch; and starting with 9.0.1 we believe the situation may be good enough.

So I'll go ahead and close this for now, but we'll monitor the situation after 9.0.1 is released, and if we see people still having lots of trouble with this, we can revisit.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2024
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed needs-design labels Dec 10, 2024
@stevendarby
Copy link
Contributor

@roji Everything you've said there sounds good, just want to note that in this particular case, I think a new migration would be blank, as the issue boiled down to running the migration in tests with a differently set up context to the one used to generate the migrations. Perhaps a scenario to just be aware of for future troubleshooting and/or documentation.

@roji
Copy link
Member

roji commented Dec 11, 2024

Yep, you're right. We may revisit this if we see that people continue to run into this, and the new 9.0.1 messages aren't sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants