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

varchar(n) fields get truncated on insert without validation errors #357

Closed
myrup opened this issue Mar 30, 2018 · 13 comments
Closed

varchar(n) fields get truncated on insert without validation errors #357

myrup opened this issue Mar 30, 2018 · 13 comments

Comments

@myrup
Copy link

myrup commented Mar 30, 2018

Hey.

If I

  • Use the [StringLength(n)] attribute on a string field of an entity,
  • Fill it with a string with length that exceeds n.
  • Add the entity to a collection
  • SaveChanges

The string is truncated without validation errors. As far as I can google it should be throwing some validation errors like I get when I try to insert with sql statements.

Thanks!

@roji
Copy link
Member

roji commented Mar 30, 2018

When [StringLength(n)] is specified, Npgsql indeed creates a column of type varchar(n) - which enforces that only strings up to length n gets inserted (and generates an error otherwise). However, EF Core itself causes this value to be truncated by setting DbParameter.Size.

See dotnet/efcore#4134, and also #342.

Closing this since the behavior is by design, but if you have further questions/objections feel free to post back here.

@roji roji closed this as completed Mar 30, 2018
@roji roji added the invalid label Mar 30, 2018
@myrup
Copy link
Author

myrup commented Mar 30, 2018

Oh ok, I'm a bit surprised on that EF Core decision, it seems so unlike the C# ecosystem to have magic truncations happening instead of forcing the developer to deal with it explicitly. Can I somehow use Npgsql EF Code first to generate the field with length limit, but not have EF set DbParameter.Size? If not I guess I'm expected to do a validationcheck before inserts/updates if I want to make sure truncations never occur?

@roji
Copy link
Member

roji commented Mar 30, 2018

@myrup I was a bit surprised too, but it does make sense to avoid sending lots of useless data to the database, especially if it'll just error back because it's too long... Don't forget this is a real network operation with performance consequences, not just a string reference being passed around in memory.

In any case, there's nothing much you can do without doing some pretty invasive changes in the provider (replacing the TypeMappingSource service, overriding the string mapping to not set the Size...). I'd definitely recommend you do your own validation checks instead.

@ajcvickers you may be interested in this conversation (somewhat related to the one we just had in #342).

@ajcvickers
Copy link

@roji It's not intended that this happen--in fact, I remember discussing this explicitly a couple of years ago and deciding that the provider throwing is a desirable behavior. So, either:

  • SQL Server and Postgres differ in the way this case is handled
  • or The SQL Server string type mappings result in different behavior than the Postgres string type mappings
  • and/or there is a bug in EF Core

/cc @divega

@ajcvickers
Copy link

Worth also noting that it is by-design that EF Core itself does not do any validation. Only that EF Core should not do anything to get in the way of the provider/database doing validation.

@roji
Copy link
Member

roji commented Apr 6, 2018

Ok, I'll dig into this a little, thanks for the info.

@roji roji reopened this Apr 6, 2018
@roji roji added bug Something isn't working and removed invalid labels Apr 6, 2018
@roji roji added this to the 2.1.0 milestone Apr 6, 2018
@roji
Copy link
Member

roji commented Apr 7, 2018

Confirmed that this is the Npgsql behavior at the ADO.NET, closing this issue in favor of npgsql/npgsql#1881.

@roji roji closed this as completed Apr 7, 2018
@roji roji removed this from the 2.1.0 milestone Apr 7, 2018
@roji roji added invalid and removed bug Something isn't working labels Apr 7, 2018
@roji roji reopened this Apr 8, 2018
@roji
Copy link
Member

roji commented Apr 8, 2018

Reopening as this isn't an ADO.NET issue - SQL Client also truncates values client-side when SqlParameter.Size is specified (see npgsql/npgsql#1881 (comment)).

@roji
Copy link
Member

roji commented Apr 8, 2018

@ajcvickers @divega OK I think I understand what's going on.

The default behavior in RelationalTypeMapping is to set the parameter's Size to the mapping's Size, if one is specified. Since DbParameter.Size means truncating the value, that means that by default EF Core tells the underlying ADO.NET provider to perform client-side truncation based on the mapping's size. This behavior isn't overridden in StringTypeMapping.

However, the SqlServer provider does override this behavior for strings and for byte arrays. Both seem to set DbParameter.Size to either maxSpecificSize (if the value length is smaller than then max column length) or to -1. This way, the parameter data is never truncated and the backend returns an error as expected.

If this is the behavior that you're looking for across providers (and I see no reason for this to vary), then I think you may want to change RelationalTypeMapping to no longer truncate by default.

In the meantime I'll modify the Npgsql provider behavior to match the SqlServer one, but we may want to find a more cross-provider way to handle this.

@roji roji closed this as completed in 7743d87 Apr 8, 2018
@roji
Copy link
Member

roji commented Apr 8, 2018

Reopening to discuss with EF Core team, let me know if you want an issue on your repo.

@roji roji reopened this Apr 8, 2018
@divega
Copy link

divega commented Apr 8, 2018

@roji I agree it is strange that the correct behavior is implemented in the SQL Server provider. Sounds like it should happen on Relational or that at least we should have a spec test that enforces this behavior for other providers.

@divega
Copy link

divega commented Apr 8, 2018

@roji did you file an issue already on our repo?

@roji
Copy link
Member

roji commented Apr 8, 2018

@divega opened dotnet/efcore#11591

@roji roji closed this as completed Apr 8, 2018
austindrenski pushed a commit to austindrenski/npgsql-efcore.pg that referenced this issue May 13, 2018
Strings are no longer truncated according to their property's MaxLength
before being sent to the database.

Fixes npgsql#357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants