Possible issue in RelationalTypeMappingExtensions.CloneWithFacetedName

[roji]

While synchronizing the Npgsql EF Core provider with the latest dev branch, I ran into an issue. The BuiltInDataTypes tests have a MaxLengthDataTypes entity, with a byte[] property whose MaxLength is set via the fluent API. Now, PostgreSQL's byte[] type (called bytea) doesn't support max length - no parentheses - but the code generates bytea(5) etc.

In a nutshell, it seems that CloneWithFacetedName tacks on maxlength (and possible precision/scale) without really checking if the type actually supports them. So if a user happens to erroneously define them on a type, that will cause a bad type name to be generated - I think the desired behavior is to ignore them instead (at least that was the previous behavior). This should be easily reproducible in PostgreSQL by setting maxlength on some type that doesn't support it.

Hope I didn't miss anything...

[roji]

/cc @ajcvickers

[roji]

npgsql/efcore.pg#300 may also be a bit relevant here... If we need to allow extensibility, i.e. adding mappings not known to the TypeMappingSource, then we can't expect the TypeMappingSource to know everything about the mappings (which ones accept a length, a precision/scale)... It may be better to have that information on RelationalTypeMapping itself - just like a mapping specifies how to generate a literal, it can specify whether it accepts a length or not.

An alternative to this is to say that when mappings are added externally, it's also the user's responsibility to replace the TypeMappingSource with one that's aware of the mappings and their different properties. That would be more cumbersome and less object-oriented (it does seem better for the mappings themselves to contain all the information), but less impact on the type mapping API at this stage.

[ajcvickers]

@roji The idea for this was to create a utility method that could be used by providers that need to do this but which does not need to be used by other providers. So, for example, SQL Server and Oracle use it, but SQLite does not. From our conversations I was assuming that Postgres probably wouldn't use it, but maybe it is needed for some types and not for others?

That being said, I like the idea of having this information in the type mapping itself and I will look into making that change.

[roji]

Thanks @ajcvickers. But doesn't SQL Server and Oracle have this issue as well? In other words, if some user mistakenly sets .HasMaxLength(5) on a type that doesn't support it, I think the current behavior would be to append (5) to the type name and to send that to the database. It's not the end of the world (the model configuration can be said to be mistaken), but it seems better for EF Core to be aware that it's not supported and ignore instead (cross-database, probably also backwards compatibility).

(tried proving this in a test but running the latest EF Core doesn't seem to work on Linux/preview2 for some reason).