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

Annotations on the first owned entity type reference are lost #10911

Closed
Bitsonthefloor opened this issue Feb 7, 2018 · 10 comments
Closed

Annotations on the first owned entity type reference are lost #10911

Bitsonthefloor opened this issue Feb 7, 2018 · 10 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@Bitsonthefloor
Copy link

Bitsonthefloor commented Feb 7, 2018

I discovered an odd regression that has occurred between build 28148 and 300047. I will have to spend some time tracking down when it occurred, I will try to do that tomorrow.

The issue is that my model has sub-owned types and suddenly the migration would stop generating correctly. I have tracked it down to two issues in the ModelSnapshot code, below is a Gist of a sample snapshot.

https://gist.github.com/mrswain/f975e4a2538eaea8d9b06d26d961cecc

The end result of the model should be 4 entities and 7 owned entities, and all entities should be mapped to 4 schema.table pairs.

What happens is I now get 6 entities (two owned ones become unowned) and some of the owned entities lose their table relational attributes.

I dropped the test back to build 28148 and it gives the correct output so some change has caused this.

NOTE:
My test program is a simple

using System;

namespace EFTest
{
    class Program
    {
        static void Main(string[] args)
        {
            var snapshot = new SnapshotTest();

            var model = snapshot.Model;

        }
    }
}

I have a breakpoint on the model assignment so I can view the results.

@smitpatel
Copy link
Member

@Mrswain - Please share your model classes and OnModelCreating code.

@Bitsonthefloor
Copy link
Author

I tracked the break point down to.. 2.1.0-preview2-28212 -- commit e4c8bbb

2.1.0-preview2-28189 -- commit 273662d
is the last version I get the correct results with, so some change after this commit broke the snapshot.

@smitpatel
What I provided is only a portion of my actual model snapshot, the portion that demonstrates and exhibits the problem. Given that this is entirely reproducible with the code given, I do not see how providing the full model or the code used to configure it is relevant, unless you believe the snapshot is in error.

@smitpatel
Copy link
Member

f2dc8c0 is the commit which would have highest impact causing this. It has very little to do with model snapshot. It is model building part which is generating a different model now. Hence I asked for model code.

@Bitsonthefloor
Copy link
Author

Bitsonthefloor commented Feb 12, 2018

The model snapshot I provided was generated in 300047 so if it is an issue with the model snapshot it is because the latter code broke it. Also the code I provided is only a small subset of my model and posting the complete model and model generation code would need its own repository as it consists of many files and supporting libraries. I just distilled it down to the essence of what was needed to reproduce the issue.

I still fail to see what the model or OnModelCreating has to do with a problem with parsing the snapshot structure.

I feel the problem lies in extension code that detects if a type is an owned type, not properly detecting in the model created from the snapshot, or the snapshot to model builder not setting up enough of the structure for detection.

FYI: I also re-gerenated the migration in 28189 and the snapshot is nearly identical (just a slightly different order to some properties) and works without any errors.

@AndriySvyryd
Copy link
Member

Seems that some annotations on the first owned entity type reference for each owned type are being lost. Here's a minimal repro:

var modelBuilder = new ModelBuilder(SqlServerConventionSetBuilder.Build());
modelBuilder.Entity("A").OwnsOne("O", "O1").ToTable("T", "S");
modelBuilder.Entity("B").OwnsOne("O", "O2").ToTable("T", "S");

foreach (var entityType in modelBuilder.Model.GetEntityTypes().Where(t => t.IsOwned()))
{
    Assert.Equal("S", entityType.SqlServer().Schema);
    Assert.Equal("T", entityType.SqlServer().TableName);
}

@AndriySvyryd AndriySvyryd self-assigned this Feb 14, 2018
@AndriySvyryd AndriySvyryd added this to the 2.1.0 milestone Feb 14, 2018
@AndriySvyryd AndriySvyryd changed the title Migration "regression" Annotations on the first owned entity type reference are lost Feb 14, 2018
@AndriySvyryd AndriySvyryd removed their assignment Feb 15, 2018
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 15, 2018
@FrederickBrier
Copy link

@Mrswain Thank you so much for filing this bug. I was pretty sure it was updating to a nightly build, because my tests and code broke so radically. I am using owned types as well. Going back to 2.1.0-preview2-28189 fixed it. Again, thank you. And thank you Andriy!

@AndriySvyryd
Copy link
Member

@Mrswain @FrederickBrier This fix should be in the next nightly build or the one after it, please try it out and let me know if there is anything else that's still broken.

@Bitsonthefloor
Copy link
Author

Bitsonthefloor commented Feb 16, 2018

preliminary tests with rebuilding the migrations seem to work so far, at least as far as the second stage. I still have two more stages to rebuild but I think this may work. I will follow up once I get them rebuilt.

@fbrier
Copy link

fbrier commented Mar 4, 2018

This did not get broken again, did it? In 2.1.0-preview1-final?

I had been getting nightly builds at: https://dotnet.myget.org/F/dotnet-core/api/v3/index.json

But then I saw that it was at: https://dotnet.myget.org/F/aspnetcore-dev/api/v3/index.json

So I snagged 2.1.0-preview2-30230 and it is still giving me the same errors that I saw previously, which was indicative that the table was created incorrectly.

Error Message:
Initialization method ARM.ServerTest.ResourceManagerTest.Initialize threw exception. System.AggregateException: One or more errors occurred. (The pro
perty 'TargetedCredential.NetworkAddress' could not be mapped, because it is of type 'string' which is not a supported primitive type or a valid entit
y type. Either explicitly map this property, or ignore it using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating
'.) ---> System.InvalidOperationException: The property 'TargetedCredential.NetworkAddress' could not be mapped, because it is of type 'string' which
is not a supported primitive type or a valid entity type. Either explicitly map this property, or ignore it using the '[NotMapped]' attribute or by us
ing 'EntityTypeBuilder.Ignore' in 'OnModelCreating'..

@AndriySvyryd
Copy link
Member

@fbrier That looks like a new regression. Could you file a separate issue with a small repro?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

5 participants