search

timabell / ef-enum-to-lookup

Generates lookup tables from enum references in Microsoft Entity Framework 6.1
https://www.nuget.org/packages/ef-enum-to-lookup
69 stars 29 forks source link

Views support (#52) #53

Closed sergey-netdev closed 8 years ago

sergey-netdev commented 8 years ago

Added extra condition to generate FKs only for user tables. #52

timabell commented 8 years ago

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.

timabell commented 8 years ago

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.

sergey-netdev commented 8 years ago

Good idea! I'll add a few lines.

timabell commented 8 years ago

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?

sergey-netdev commented 8 years ago

I'm still a bit new to github so I can only guess what the squash is (rebase maybe :)

timabell commented 8 years ago

yeah, nothing to do with github, good old fashioned git rebase --interactive

timabell commented 8 years ago

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).

sergey-netdev commented 8 years ago

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.

timabell commented 8 years ago

Are you using those Up() / Down() methods that EF provides or some other migration system?

P.s. take a look at ready-roll, it's very good.

timabell commented 8 years ago

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

sergey-netdev commented 8 years ago

Yes, we use Up/Down methods and it's really painful to mix arbitrary sql text with native EF migrations.