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

Consider breaking up ModelValidator #15660

Open
ajcvickers opened this issue May 8, 2019 · 4 comments
Open

Consider breaking up ModelValidator #15660

ajcvickers opened this issue May 8, 2019 · 4 comments

Comments

@ajcvickers
Copy link
Member

ajcvickers commented May 8, 2019

Currently ModelValidator has protected method for each area to validate. These could be instead separate services that can be resolved from D.I. and composed. This could make it easier for non-providers to add custom validation in the same way they can add custom conventions.

Note however that order is important--if we make this change, the current order in which validations happen should not be changed.

Update: the main value here is allowing plugins to do model validation. For example, see change-detecting proxies. See comments below.

@ajcvickers
Copy link
Member Author

Note from triage. Consider a different factoring such that validation is split up by what needs to be verified. For example, for things that need to validate a property, we should iterate over the properties once and call out to each validator in that loop.

@ajcvickers
Copy link
Member Author

Thinking about this again in relation to the PR for change tracking proxies.

Currently there is no way for a non-provider extension to influence model validation because the single IModelValidator service is already overridden by providers. We could implement something similar to IConventionSetPlugin that would allow additional validation to be added. However, the plugin needs to be able to disable or override existing validation, not just add extra validation. Also, the provider may already have overridden the core/relational validation, which may mean that the plugin is overriding/disabling something that it does not know about.

At a high level, it seems that validation should be factored into small discrete blocks. Ideally a provider would then not add additional validation by overriding existing validation. This could be combined with the refactoring mentioned in the note from triage above.

Removing from the backlog to discuss with the team both in terms of the overall plan, the scheduling of the change, and what are immediate guidance is for the change tracking PR.

/cc @AndriySvyryd @orionstudt

@ajcvickers ajcvickers removed this from the Backlog milestone Jan 7, 2020
@ajcvickers ajcvickers mentioned this issue Jan 7, 2020
3 tasks
@ajcvickers ajcvickers added this to the Backlog milestone Jan 10, 2020
@ajcvickers ajcvickers self-assigned this Jan 10, 2020
@ajcvickers
Copy link
Member Author

Note to implementer: Change detecting proxies (PR #19437) should be updated when this issue is fixed.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Sep 7, 2021

Note to implementer: Also break up RelationalModelValidator and provide discovered mappings to facilitate validation of relational model. And call ValidateJsonEntities from ValidateSharedTableCompatibility

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