-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
[wtforms] Using wtforms-json which prevents encoding empty strings in the database #5445
[wtforms] Using wtforms-json which prevents encoding empty strings in the database #5445
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5445 +/- ##
==========================================
+ Coverage 56.07% 56.07% +<.01%
==========================================
Files 526 526
Lines 23253 23255 +2
Branches 2783 2783
==========================================
+ Hits 13038 13040 +2
Misses 9806 9806
Partials 409 409
Continue to review full report at Codecov.
|
When you say sub-optimal, it seems to me like the difference |
@mistercrunch I updated the description. |
ping @mistercrunch |
e89b1e0
to
0629cba
Compare
@mistercrunch @betodealmeida would you mind looking at this PR? We have numerous inconsistencies in our production database due to lack of uniqueness constraints (#5449, #5451, #5452, #5453) which we would like to resolve and this PR is a necessary upstream requirement. |
4726575
to
4eb8389
Compare
Taking a look. |
This PR fell in a crack. Let's revive it. Seems like the long term solution is not using WTForms at all as we rip out more of the MVC and FAB scaffolding over time. So I get the |
@mistercrunch as far as I'm aware it doesn't impact the users however it does pollute the database records if querying and/or writing a migration, i.e, at times one needs to write logic to handle both |
I'm just afraid of the db migration that updates a significant portion of all the cells in the database. Clearly we should recommend scheduling downtime for this one in |
if getattr(record, col.name) == '': | ||
setattr(record, col.name, None) | ||
|
||
session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a hell of a commit. I'd suggest proceeding carefully and committing on a per-record (only if needed) basis. Maybe on a per-table basis, but that could represent very large commits too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mistercrunch I move the commit to occur after each table. I've also updated the UPDATING.md
.
e4ecfb9
to
e8af925
Compare
LGTM once it passes the build. Curious to see how long that migration will take in your environment. |
@mistercrunch I need to update the downgrade version to the laster on master. I’ll test again on our development environment. |
e8af925
to
4d14519
Compare
@mistercrunch the migration took ~ 90 seconds in our environment. |
4d14519
to
e9acc81
Compare
Sweet. I think this is ready to go! |
(cherry picked from commit e1b9077)
@john-bodley @mistercrunch, heads up: we had 2 dashboards with title |
@betodealmeida apologies for the breakage and thanks for the heads up. |
This issue has been plaguing me for some time, though the actual fix was somewhat trivial. FAB uses WTForms which regrettably coerces a
None
value into an empty string per here. This means that for the database tables associated with forms string/text columns include a combination ofNULL
of empty strings (depending on how they were populated) which creates inconsistencies in the the schema, i.e., to identify non-custom SQL tables one would need to use:as opposed to:
This is also troublesome for uniqueness constraints as currently the following schema, table tuples would be considered different (even thought the represent the same entity):
NULL
,my_table
)my_table
)The fix is to use the wtforms-json package which adds supports for
None
types per here. The package monkey patches the processing of the field and assigns the default value (None
) when the value evaluates to false (which includes empty strings).This PR also performs the necessary migration to retroactively resolve all forms which have encoded empty strings in the database.
to: @betodealmeida @fabianmenges @graceguo-supercat @michellethomas @mistercrunch @timifasubaa