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

Added a ModelBuilder.FinalizeModel method to support building outside the context #13455

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

ajcvickers
Copy link
Contributor

@ajcvickers ajcvickers commented Sep 30, 2018

Fixes #11738

Naming to be discussed.

This runs the model-based conventions, and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Sep 30, 2018
@AndriySvyryd
Copy link
Member

FinalizeModel should also call IModelValidator.Validate, otherwise using it would be a pit of failure

@ajcvickers
Copy link
Contributor Author

@AndriySvyryd We should discuss whether or not this call should actually do validation--@divega had some string opinions on it in triage. That aside, we need a way to get the IModelValidator to the place it is needed, which requires some plumbing. So I don't want to do this if we decide we shouldn't validate in this method.

@ajcvickers ajcvickers removed the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 2, 2018
@ajcvickers
Copy link
Contributor Author

@AndriySvyryd New version up that runs validation as part of FinalizeModel when the ModelBuilder is constructed externally to the context.

@@ -59,7 +59,7 @@ protected virtual IConventionSetBuilder CreateConventionSetBuilder(DbContext con

protected virtual void Validate(ModelBuilder modelBuilder)
{
modelBuilder.GetInfrastructure().Metadata.Validate();
modelBuilder.FinalizeModel();
var context = CreateContext();
context.GetService<IModelValidator>().Validate(modelBuilder.Model);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other calls to IModelValidator.Validate() should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ajcvickers ajcvickers force-pushed the OutOfContext0930 branch 2 times, most recently from 88499e4 to 67f08ad Compare October 2, 2018 20:26

Dependencies.ModelCustomizer.Customize(modelBuilder, context);

_onModelCreating(modelBuilder, context);

model.Validate();
modelBuilder.FinalizeModel();

validator.Validate(model);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is still needed. The TestModelSource doesn't add the validator as a convention. It's only when the ModelBuilder is created externally from the context using the static CreateConventionSet method that the validator is added as a convention.


Dependencies.ModelCustomizer.Customize(modelBuilder, context);

model.Validate();
modelBuilder.FinalizeModel();

validator.Validate(model);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And search for other usages

@ajcvickers
Copy link
Contributor Author

@AndriySvyryd New version

@@ -76,18 +75,15 @@ public virtual IModel GetModel(DbContext context, IConventionSetBuilder conventi
Check.NotNull(validator, nameof(validator));

var conventionSet = CreateConventionSet(conventionSetBuilder);
conventionSet.ModelBuiltConventions.Add(new ValidatingConvention(validator));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to CreateConventionSet

… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
@ajcvickers ajcvickers merged commit de17e6c into release/2.2 Oct 3, 2018
@smitpatel smitpatel deleted the OutOfContext0930 branch October 3, 2018 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants