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

Store ValueGenerator directly in IProperty runtime annotation instead of ValueGeneratorCache #31901

Open
AndriySvyryd opened this issue Sep 28, 2023 · 3 comments

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Sep 28, 2023

Related to #31539

Same for EntityMaterializerSource

@ajcvickers
Copy link
Contributor

@AndriySvyryd This is something I have been thinking about a lot, especially in relation to #31866, where the model types are not only in the cache key, but also in the compiled delegate. The reason we haven't done that before is that these things depend on both the model and the current internal service provider. So storing these things in the model is problematic when the same model is used with multiple different context types, or even different context configuration for the same context type. Maybe we need to give the service provider a key, like we gave the model a key in the previous PR? Thoughts?

@AndriySvyryd
Copy link
Member Author

AndriySvyryd commented Oct 2, 2023

@ajcvickers For the same model to be used with different internal service providers the user must call UseModel or UseInternalServiceProvider. And all usages should be with the same provider. So, for the cache to cause any issues one of the services that provide the cached values should've been replaced explicitly with different implementations on different contexts.
It seems like a very corner case, but if you want to provide a better exception if someone does get into this case we could store the internal service provider cache key on the model and when a new context is initialized log a warning if the stored key doesn't match the current key.

@ajcvickers
Copy link
Contributor

@AndriySvyryd So I should just stop worrying about it? :-)

@ajcvickers ajcvickers added this to the Backlog milestone Oct 11, 2023
@ajcvickers ajcvickers self-assigned this Oct 12, 2023
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants