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

Guid wrongly generated for alternate key Guid columns #6048

Closed
roji opened this issue Jul 11, 2016 · 6 comments
Closed

Guid wrongly generated for alternate key Guid columns #6048

roji opened this issue Jul 11, 2016 · 6 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

@roji
Copy link
Member

roji commented Jul 11, 2016

The following test fails on SqlServer and PostgreSQL:

[Fact]
public void DefaultGuid()
{
    using (var testStore = SqlServerTestStore.CreateScratch())
    {
        using (var context = new DefaultGuidContext(testStore.ConnectionString))
        {
            context.Database.EnsureCreated();
            var logic = new DefaultGuidContext.GuidEntity();
            context.GuidEntities.Add(logic);
            context.SaveChanges();
            Assert.Equal(Guid.Empty, logic.GuidProperty);
        }
    }
}

class DefaultGuidContext : DbContext
{
    readonly string _connectionString;

    public DefaultGuidContext(string connectionString)
    {
        _connectionString = connectionString;
    }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlServer(_connectionString);

    protected override void OnModelCreating(ModelBuilder builder)
    {
        builder.Entity<GuidEntity>().Property(l => l.GuidProperty)
            .HasDefaultValue(Guid.Empty);

        builder.Entity<GuidEntity>().HasAlternateKey(l => new
        {
            EvaluatedAnswer = l.GuidProperty,
        });
    }

    public DbSet<GuidEntity> GuidEntities { get; set; }

    public class GuidEntity
    {
        public int Id { get; set; }
        public Guid GuidProperty { get; set; }
    }
}

The test fails, since instead of having an empty (all-zeros) Guid EFCore generates a random one. Checking the PostgreSQL logs clearly shows EFCore sending a random generated Guid, although that isn't configured on the model in any way. Removing the HasAlternateKey makes the problem go away, as does removing HasDefaultValue or adding ValueGeneratedNever() on the property. If an explicit value is set on the property which isn't Guid.Empty, it gets sent correctly.

It would seems like the HasDefaultValue is implying ValueGeneratedOnAdd, which makes EFCore auto-generate random Guids rather than sending Guid.Empty. I'm not sure what the HasAlternateKey has to do with it though.

@ajcvickers
Copy link
Contributor

@roji EF currently requires alternate keys to be populated with unique values in the same way that primary keys are. (Note that this is not the case for something that is just a unique index. There is usually no point in telling EF that something is an alternate key because it will always be implied by the relationship API if it is needed, but that's a different debate... If you just want a unique constraint, then don't use HasAlternateKey. HasAlternateKey means that the property is expected at the principal end of an FK constraint.)

Given this, what would you expect the behavior to be in this case? By saying the property is a Guid and an alternate key, you are getting the default value generation behavior for a Guid key--to generate client side Guids. The call to HasDefaultValue doesn't change this, but maybe it should? But if it did, then on the second entity that used the default there would be an exception because of conflicting key values. That doesn't seem better. Maybe we should throw in model validation for this scenario? Interested to hear what you think?

@roji
Copy link
Member Author

roji commented Jul 11, 2016

Ah, I understand - I indeed didn't understand HasAlternateKey as implying full key-ness, including ValueGeneratedOnAdd by default.

I think that EFCore should try to stick as close as possible to what the user expresses on the model - if the user chooses to say HasDefaultValue on a key, that should take precedence. In other words, if the user inserts two entities with a non-explicit (i.e. default) value, a unique constraint violation should be triggered. Even though this is weird, it follows the user's wishes to the letter and shouldn't have much surprise effect. There may even be some far-fetched scenario in which this make sense (although I admit I don't have one).

Throwing a model validation exception is another good option if you really think this makes no sense whatsoever.

@sleepingneon
Copy link

@ajcvickers I'm sorry in advance if I got it wrong.
What if the alternate key has two or more fields? I.e. it doesn't need every single field to be unique but their combination should be unique. At the same time I'd like to have an ability to specify default values.

@ajcvickers
Copy link
Contributor

@roji What you said makes sense, but not sure how high priority it is. We will discuss in triage.

@sleepingneon That should be fine..

@rowanmiller
Copy link
Contributor

Discussed and decided we should raise a warning in this case

@roji
Copy link
Member Author

roji commented Jul 15, 2016

OK, thanks.

@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 Jul 25, 2016
@ajcvickers ajcvickers modified the milestones: 1.1.0-preview1, 1.1.0 Oct 15, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
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

4 participants