Fix scaffolding NRT issues

[roji]

That only surface in preview versions of VS.

I've fixed this by using GetFieldValue instead of GetValueOrDefault when reading the relevant database values. I've taken a look (e.g. here) and to my understanding, the affected columns (table/index/key/constraint name, delete referential action) can never be null in the database. In at least some of the cases, our own code would anyway throw immediately afterwards if null was ever returned (@bricelam can you please confirm this looks OK?).

Fixes #19496

[smitpatel]

Our scaffolding code takes care of constraint names being null in database. We don't throw exception for that. It may not be possible to have null value for SqlServer provider but assumption that it cannot be null in DatabaseModel can be incorrect.

[smitpatel]

on delete action can be null. There is nothing in our code says it has be to present.

[roji]

The question here is about whether SQL Server can return null or not, not about our model or code. That doesn't seem to be the case according to the docs, but if we feel there's a risk SQL Server will return a NULL here I don't have anything against allowing for that - let me know what you prefer.

[smitpatel]

Then consumer of this code can move away from handling value we cannot encounter. Currently it returns null. If we want to make our code future proof if SqlServer change any of this then we need to open that another value can come in. I don't have preference on either way how we read value. Value read and value processing should align on properly, which is not case right now.

[roji]

I'm not sure I follow (see some answers below). The question of how DatabaseModel looks like is separate from how SQL Server specifically scaffolds its information, no?

[smitpatel]

Thinking more about this

• We should only use GetFieldValue if we are certain that we won't encounter null in database. (Bit hard of assumption since future version of SqlServer could break things). Otherwise better to read using default and throw if we see null value. GetFieldValue would throw highly un-useful exception message.
• Scaffolding for most part ignore things which it cannot handle. So if we see null constraint name, we could log a warning and ignore that constraint.

Above is irrespective of DatabaseModel. In DatabaseModel

• It is abstraction over all relational providers, are we certain that all providers have required constraint name?
• We could also define a default behavior like, if there is no name in database, we define default one in runtime. Our scaffolding is not fully round trip-able anyway. We try to do stuff which provide most value. And scaffolding such constraint could be more useful than ignoring.
• If we decide that 2 is not worth then we can throw exception or skip during scaffolding rather than let provider think what should they do with empty name constraint.

[roji]

We should only use GetFieldValue if we are certain that we won't encounter null in database. (Bit hard of assumption since future version of SqlServer could break things). Otherwise better to read using default and throw if we see null value. GetFieldValue would throw highly un-useful exception message.
Scaffolding for most part ignore things which it cannot handle. So if we see null constraint name, we could log a warning and ignore that constraint.

In many cases, being very defensive against future nulls seems a bit much - I doubt we'll ever get rows describing tables where the name is null, for instance. If we're talking about throwing a more informative exception, we could simply fix that if/when it ever actually happens. Existing code before my PR already contained various assumptions about non-nulls coming back, but if we feel it's important to be more defensive against future changes in scaffolding I can make this change.

It is abstraction over all relational providers, are we certain that all providers have required constraint name?

See previous discussion in #19321 (comment) and @bricelam's suggestion in #19321 (comment). As far as I know constraint names are a standard part of SQL, I'm not aware of any exceptions.

This is also somewhat related to the conversation in #19478 (comment) - since I can't imagine an actual valid empty string value, we use that to convey that there was no name in the database, rather than having two possible values (null and empty string). This could be used to implement your suggestion of a default constraint name at runtime.

[smitpatel]

Filed #19513

[roji]

@smitpatel tested on VS 16.5.0 Preview 1.0, builds fine (how do I get on the preview 2 channel?). Applied decisions from design:

• Names for indices, foreign keys etc. are now nullable (also referential action).
• Table name is non-nullable, and we assume it comes non-null from the database.

PR changed as discussed in design meeting