Nullability annotation for scaffolding #19321
Conversation
src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs
Show resolved
Hide resolved
|
|
||
| /// <summary> | ||
| /// The foreign key constraint name. | ||
| /// </summary> | ||
| public virtual string Name { get; [param: CanBeNull] set; } | ||
| public virtual string Name { get; [param: NotNull] set; } |
There was a problem hiding this comment.
Not sure about the nullability of this (also for other constraints/index).
There was a problem hiding this comment.
Thinking about this again, we may want to simply leave the names of constraints and index nullable, for databases which don't support them? Let me know what you think.
There was a problem hiding this comment.
Better err on the nullable side
There was a problem hiding this comment.
Looked at this again. Before my changes, name is "nullable" (has [CanBeNull]) on DatabaseForeignKey, DatabasePrimaryKey, but not on DatabaseIndex and DatabaseUniqueConstraint.
As @bricelam suggested, am going to leave all these non-nullable with the idea that providers can set these to empty string if they don't support a name. Otherwise we'd have two options - both null and empty string - which doesn't seem to make much sense... We can change this if anyone has any strong feelings about it.
src/EFCore.Design/Scaffolding/Metadata/Internal/DatabaseForeignKeyExtensions.cs
Show resolved
Hide resolved
src/EFCore.Design/Scaffolding/Metadata/Internal/DatabaseColumnExtensions.cs
Show resolved
Hide resolved
roji
force-pushed
the
ScaffoldingNullability
branch
4 times, most recently
from
9e7cab4 to
e58e2d7
Compare
|
@roji @AndriySvyryd @bricelam What would be a good way forward with this? Should we discuss the non-obvious/questionable cases in a design meeting? That might also help the team develop a consistent approach to these things. |
|
To be honest the open questions seem a bit minor to me (in the specific case of scaffolding), I think @bricelam could probably just provide some of the answers and we could finalize here in writing. But we can definitely discuss tomorrow, shouldn't take to long. |
| @@ -47,21 +56,19 @@ public void Creates_entity_types() | |||
| { | |||
| Tables = | |||
| { | |||
| new DatabaseTable | |||
| new DatabaseTable(Database, "tableWithSchema") | |||
There was a problem hiding this comment.
There was code somewhere in the tests to fix up back references. It can be removed now.
There was a problem hiding this comment.
I think you're talking about FakeScaffoldingModelFactory.Create - removed that method. We can maybe kill FakeScaffoldingModelFactory altogether (it doesn't do anything anymore), but leaving it in for now.
There was a problem hiding this comment.
Actually ended up bringing the fixup back, since we in our tests we create "incorrect" models, which this code rectifies; for example, in Column_ordinal_annotation we create a new DatabaseTable, but the columns inserted into it refer back to the test suite's global Table instance (not the same). We could go over tests and fix this, but it would make test code more cumbersome as we wouldn't be able to use object initializers as now.
If we this is worth it I'll do it, for now will merge with the fixup code as before.
| var resultingFiles = new ScaffoldedModel(); | ||
| if (options.ContextName == null) | ||
| { | ||
| throw new ArgumentException($"{nameof(options.ContextName)} cannot be null", nameof(options)); |
There was a problem hiding this comment.
To be super pedantic about it, if options.ContextName is null, it's problematic for GenerateModel to throw ArgumentNullException, since the argument itself isn't null but rather some property on it :)
Will leave like this for now, but am fine changing if anyone cares enough about it.
src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs
Show resolved
Hide resolved
roji
force-pushed
the
ScaffoldingNullability
branch
from
e58e2d7 to
f481b97
Compare
roji
force-pushed
the
ScaffoldingNullability
branch
from
f481b97 to
a356758
Compare
|
@roji I'm reverting this because it's preventing me from building, which is spoiling my vacation!
|
|
Huh? That's weird... You've got the latest VS and whatever "other IDE-related tools" you're using right? Can you at least confirm whether a cmdline build work with the latest dotnet SDK 3.1? Otherwise if this commit is preventing you from working during vacation it may be a good commit :) But sure, if stuff doesn't work we can investigate later and re-merge. |
Saturday project: NRT annotations for all of scaffolding.