Database limits no longer set in schema

[alex-handley]

I was looking at upstreaming some improvements over the weekend and noticed some inconsistencies that I showed to @jordan-brough and we both agreed might need some thought.

Rails 4.1 changed the behaviour of the limit attribute so now by default in Postgres it is no longer set - rails/rails#19001
This behaviour can be seen here - https://github.com/alex-handley/solidus/commit/2b99ea4cace80d29f79fcfa0d8010b0d0a5b0a43
The spec in the branch will pass for both Sqlite and Postgres as they no longer have a field limit being set by default but will fail for MySQL because it still sets limits by default.

This leaves us with 3 potential issues:

• MySQL silently truncates data that is over the limit (thanks for pointing out @jordan-brough) - http://www.davidpashley.com/2009/02/15/silently-truncated/
• Older Solidus Postgres apps: raise an ugly DB-level exception if a length is exceeded as they have limits historically set by default.
• New Solidus Postgres apps: allow data of any length in a lot of fields.
  This could lead to attackers dumping tons of data into fields as they have no limits.
  Or more legitimately an address line1 could be 800 characters which would never fit on a label :)

There are a number of possible solutions:

• Implementing the DbMaximumLengthValidator on all string fields, this would solve the issue for older apps.
• Setting a sane field limit on new app's created with Postgres and maybe include a note that new fields require a limit in the dev docs.

Has anyone encountered this already?

[fredericboivin]

It's an issue for slugs because historically, there's a 255 maximum length but it's not covered by the validations (only a min, no max) so if it's exceeded you get an Error 500 with ActiveRecord::StatementInvalid.

I was looking into updating the validations to fix the issue and I noticed the same thing as you : limit is no longer set for newer apps.

I'd say it's better to have limits than not because without them, you get "undefined behavior" depending on your database engine.

[Senjai]

I would prefer to have limits in the schema instead of using application level constraints if possible. We could add application validations where they make sense, but I dont think we should use them to solve the database level issues with limits. With respect to the mysql concern, setting strict sql mode will make all warnings become errors, but that has its own draw back (this is the default when running migrations though).

[alex-handley]

@Senjai Thanks for the mysql tip, I have been using pg for a while now so haven't come across this before.

Does this sound like a reasonable plan:

• Add limit to existing migrations (this will fix the issue for future apps)
• Start to add DbMaximumLengthValidator constraint to important fields.
• Look at best migration path for existing apps that are missing limit

[cbrunsdon]

Thanks a lot for the writeup, this is a pretty tricky issue and the clarity is very much appreciated.

I'd prefer to do a new migration adding lengths rather than editing the existing migrations. I'm worried about the states different stores are going to end up in. Adding a single migration to ensure everything is in the right state seem like the right way to go for me. Of course we'll need to decide what that "right state" is and what limits we should have everywhere.

I'm not a big fan on the DbMaximumLengthValidator (Assuming we're talking about the one that was added into spree 3), but more on principal than objecting to the implementation.

[Senjai]

I am also 👍 on one migration to make everything "correct" but also agree on the concerns around that.

I'd be 👎 on the maximum length validator approach, just because I'd want to ensure consistency at the database layer instead of in the application layer.

[alex-handley]

@cbrunsdon Yep, makes sense to apply it as a new migration but we will need to come up with a solution for all of the records that will become invalid for existing app's.

@Senjai I may be mistaken but DbMaximumLengthValidator just uses the db level constraint so we can show a pretty Rails error rather then fail at db level.

[cbrunsdon]

I'd like to throw this in as a potential way to set the validations. It will use the limit from the database if one exists, or fall back to the hardcoded value if nothing is set.

module Spree
  class Address < Spree::Base
    validates :firstname, length: { maximum: columns_hash["firstname"].limit || 100 }
  end
end

[alex-handley]

@cbrunsdon looks like a good solution, should this only be on the user entered fields for now? e.g. addresses

[alex-handley]

@cbrunsdon I was looking at DbMaximumLengthValidator it is only used in one place, what don't you like about it? potentially we could just remove it as having a custom validator used in one place with its own specs doesn't make sense :)

[cbrunsdon]

@alex-handley ha, that was my thinking too. I prefer my inline to using the DbMaximumLengthValidator (if for no other reason than we can provide a default length), which would let us kill the validator from core (or at least deprecate and kill in the future).

@jhawthorn opinion?

[alex-handley]

@cbrunsdon The only feature I like about DbMaximumLengthValidator is its maintainability if say the way limit is retrieved changes in Rails.

[fredericboivin]

Should a limit be added to every string column or only to a subset of those fields like names and slugs ?

[alex-handley]

@dangerdogz I am only going to add to user facing fields. I should have the migrate ready tomorrow for some feedback.

[alex-handley]

Morning, this is my first attempt at creating the migration:
https://github.com/alex-handley/solidus/commit/f6e39b1d195446b9ee84b846fb609fc094dd18fe

I am going to create a db dump from our prod and see if I encounter any issues.
Any objection/suggestions to the included fields?

[cbrunsdon]

@alex-handley we're going to need some kind of check/guard/something at the top of this migration to do a sql max() length of the fields to ensure we don't accidentally truncate anyones data.

[alex-handley]

Hey, just to check in I have been pretty busy the last few weeks so only just got round to finishing this.
I have built the migration with truncation protection and added the validates statements but one thing I have noticed is columns_hash crashes any script/task that runs when a DB/table doesn't exist e.g. db:create or bash build.sh.
This is because columns_hash tries to access the table and it doesnt exist yet which throws a ActiveRecord::StatementInvalid exception.

Has anyone come across this before?

[fredericboivin]

Maybe this is why something like DbMaximumLengthValidator could be useful? Unless there's a better way, I think we would need to check that columns_hash exists and has a value (if so, use it) otherwise use a default value.

Adding this to every single validates would be tedious to change and error prone, and using columns_hash as is would cause installation nightmares.

@cbrunsdon Thoughts? (since you want to kill DbMaximumLength)

[alex-handley]

Agreed @dangerdogz I think we should stick with something like DbMaximumLengthValidator for the reasons you mentioned.

[seand7565]

I think the action items here should be:

• Add a default limit to db_maximum_length_validator
• Add db_maximum_length_validator to any user input fields
• Optionally, add a config option for maximum db length so the user can specify their own.

Based on the conversations above, that seems like the best solution here. It doesn't solve the silent truncation issue with mySQL, but does solve the possibility of users overflowing user input fields with like a million lines of junk. How's that sound to everyone?

[aldesantis]

@seand7565 I think that could be a good way forward. I'd just be mindful of how it will impact existing apps that currently rely on the absence of limits. With a proper deprecation approach, though, it shouldn't be a problem.

Based on the conversations above, that seems like the best solution here. It doesn't solve the silent truncation issue with mySQL

I'm not sure why you say that? If we have a limit on the length of the field at the application level, strings longer than the limit will not even reach the DB, unless I'm missing something.

[seand7565]

My proposal was to add the validator only to user input fields, not to admin input fields - Some stores that I've worked on overuse the product description field for instance, so putting a limit there might cause issues. In this case, the mySQL truncation would still happen on overly long fields for admin fields. Unless we want to just apply the validator to everything.