Views support (#52) #53
Conversation
| } | ||
|
|
||
| /// <summary>Context containing a view</summary> | ||
| public class ViewDbContext : DbContext |
There was a problem hiding this comment.
this should be in its own file in the db/ folder
|
Nice work! Just a couple of trivial comments then I expect this can go in. I'll probably give it a poke on my own machine first just to be sure. Seeing as this fixes a bug in a new use case I think this can probably go out in the next point release rather than waiting for a minor or major release. |
|
Another thought, it would be worth adding something to the readme explaining the that although ef can use views, the fks won't be created and that you'll be on your own for linking any tables ef doesn't know about. |
|
Good idea! I'll add a few lines. |
|
I'm going to test it, squash the first two commits, and probably add my own take to the readme, then merge. Unless you want to do do the squash? |
|
I'm still a bit new to github so I can only guess what the squash is (rebase maybe :) |
|
yeah, nothing to do with github, good old fashioned |
|
I've had a proper play with this now and in looking closer at the test you've provided I can see now that it's very tightly tied to the existing tests, reusing the same database. I'm not happy with this as it makes the test suite much harder to maintain in the long run. I had a go at disentangling them from each other, but it seems that ef really doesn't expect you to use views, so I quickly ran into serious problems. I'm guessing you aren't using the automatic database generation that the tests use as that seems to make it impossible to do cleanly. As it stands, I'd consider taking a patch to just the generated sql + the readme, without the additional test as I think views are an edge case and this is the first report I've had relating to them. So long as it doesn't break the existing tests (which it currently doesn't) then I see no harm. To add test coverage, I think the best approach might be to create a completely unrelated dbcontext, model and test class which matches more closely your real use of the library, including whatever method you're using to build your db (migrations? raw sql scripting?). Up to you what you want to do now, feel free to open a fresh PR from a new branch (make sure you base it on the v1 branch this time) with a more minimal patch, or an alternate approach to the tests (which would be appreciated but isn't required). |
|
As we fight performance issues we lean toward db first approach. We still use migrations but the number of SPs, views and functions grows so I expect we'll switch to completely db first in a few months. You're right that the test is dependent on the existing context but so are views on existing tables (which are often just a bunch of heavy joins and the related entity classes are often inherited). That's why it took me some time to figure out how to add the test. Probably adding a script to generate unrelated dbcontext is a good idea to simulate db first approach but the current test definitely reflects the scenario we got. |
|
Are you using those P.s. take a look at ready-roll, it's very good. |
|
It seems you can't mix the automatic db generation with migrations easily. http://stackoverflow.com/questions/20862807/mapping-database-views-to-ef-5-0-code-first-w-migrations |
|
Yes, we use Up/Down methods and it's really painful to mix arbitrary sql text with native EF migrations. |
Added extra condition to generate FKs only for user tables. #52