Fix ModelProducer to handle multiple APIs with different DbContexts #529
Conversation
|
Hi @robertmclaws, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
| if (efEntityContainers.Count > 1) | ||
| { | ||
| var containerNames = efEntityContainers.Aggregate("", (current, next) => next.Name + ", "); | ||
| throw new Exception("This project has multiple EntityFrameworkApis using different DbContexts, and the correct contect could not be loaded. \r\n" + |
There was a problem hiding this comment.
spell: contect -> context
I'm not sure if C# 6 syntax could pass compile, Vincent?
There was a problem hiding this comment.
OK, I fixed the spelling issue. Thanks for catching that.
I'm not sure how you are compiling, but if you're on VS2015 it should be fine. Can always do a string.Format() if necessary.
There was a problem hiding this comment.
Our internal signed build server is still VS2013, we can not use c# 6 syntax.
There was a problem hiding this comment.
Is it possible to upgrade the build server? It seems like the VS2015 compiler with all of the updates would be able to take advantage of more optimizations / compilation improvements than a 4 year old runtime. If this project is going to be moved to be community-driven, people are not going to want to deal with legacy systems.
There was a problem hiding this comment.
I do not think it will happen in short time consider the server is maintained and used by much wide d groups. We do already initiate such discussion
There was a problem hiding this comment.
Alright, I'll take it out when we move the text into a Resource.
| if (efEntityContainer == null) | ||
| { | ||
| if (efEntityContainers.Count > 1) | ||
| { |
There was a problem hiding this comment.
Do we know which case we will have two container, what will each container will have? If we fix like this, and EF do have two containers, each container will have some entity set (DbSet), then the logic will be wrong, we will need to iterate the container to get the information.
I am OK to add this as a temp fix but please help to add a TODO comment here for further improvement.
There was a problem hiding this comment.
Yes, I know which case there will be more than one container, that's why I had to write this code. If you register 2 different APIs that each use a totally different DbContext in the same website, then the call to efModel.GetItems<EntityContainer>(DataSpace.CSpace) will return more than one container, even though they are in separate EDMX files in separate DbContexts. One would think that wouldn't be the case, so maybe this exposed a flaw somewhere deeper in the OData or EF framework. But that is OK, because the code above these lines should always return the right container for the particular context, even if you find and fix the underlying problem.
This particular set of highlighted code is only there in case, for some presently unknown reason, the query to get the matching context fails, It's a defensive measure to help developers understand what is going on, instead of the current code just assuming everything will work, until it doesn't, and not giving you any clue as to why.
There was a problem hiding this comment.
I believe that this code would work even if an individual EDMX file ever had multiple containers (though I don't know if that is actually even possible). Given the fact that right now, the code is pulling containers from totally separate DbContext instances and putting them in the same collection, one would think that multiple Containers in the same Context would also apply. However, if at some point in the future it didn't, the logic for efEntityContainers could be adjusted easily; and that's why I broke that call out from the .FirstOrDefault() call.
There was a problem hiding this comment.
OK, thanks. Can you help to add some comments here which will help people who maintain these codes.
There was a problem hiding this comment.
Done. Hopefully those help. I will see about moving the exception messages to the resources over the weekend.
| var containerNames = efEntityContainers.Aggregate("", (current, next) => next.Name + ", "); | ||
| throw new Exception("This project has multiple EntityFrameworkApis using different DbContexts, and the correct context could not be loaded. \r\n" + | ||
| $"The contexts available are '{containerNames.Substring(0, containerNames.Length - 2)}' but the Container expects '{efEntityContainer.Name}'."); | ||
| } |
There was a problem hiding this comment.
The exception string is not hard coded, we need to put into property files, refer to other exception for sample.
There was a problem hiding this comment.
I hard-coded the string for now so that you could see the exception that is thrown, and there can be a possible discussion about the language, if necessary. I also wanted the team to see how to throw an informative exception (most of the exceptions in Restier are not very informative). We can move the strings into the property files after everyone is OK with it.
There was a problem hiding this comment.
For the message part, you are native speaker, you are much better than me on this :)
BTW, as I will start vacation tomorrow, I will response to this PR after my vacation.
Added explanations for: - why the bug is happening. - how we work around it that attempts to ensure it keeps working in the future. - how we are informing the user in detail about any problems they might encounter, and how to solve them.
|
Hey everyone, Just wanted to see where we're at on this. Thanks! |
|
I am not sure whether you have a chance to move message to resource file, after you have done this, I will merge the changes, but we do not have a solid plan for next release now. |
|
Sorry for the delay in this, @chinadragon0515. I've made the changes so that now the Exception uses the Resources instead of a hard-coded string. Ideally, this situation still needs a test case, and at some point I will endeavor to write one. But for now, we should be able to merge this change, |
|
Merge the PR |
Issues
This pull request fixes issue #527.
Description
New code checks the collection of objects returned by
efModel.GetItems<EntityContainer>(DataSpace.CSpace)for theEntityContainerthat matches the expectedDbContext. If none is found, throws an informative Exception that helps the user identify the problem.Checklist (Uncheck if it is not completed)
Additional work necessary
Proper tests need to be added to make sure that GetModelAsync() works in various situations. There is not enough direct code coverage for that method.