Opened 8 years ago

Closed 2 years ago

#3997 closed Bug (fixed)

Missing default value causes exception on save

Reported by: Henrik Vendelbo <info@…> Owned by: PhiR
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: doc
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Using MySQL I have noticed a common cause for save to fail. Especially BooleanField's with no default value. If there is no default value, MySQL will return a Warning for data truncation, which will cause the save to fail. I believe that I tried to catch MySQLdb.Warning and pass on it with no improvement.

1) A warning should not cause the transaction to fail
2) If a default value is effectively required, validate should check for it

Attachments (2)

3997.diff (571 bytes) - added by PhiR 8 years ago.
note in the model api doc about that …
3997_2.diff (656 bytes) - added by PhiR 7 years ago.

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by PhiR

note in the model api doc about that ...

comment:1 Changed 8 years ago by PhiR

  • Has patch set
  • Keywords doc added
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to PhiR
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Ready for checkin

The patch only adds a note to the doc. An empty Boolean fields will cause validate() to fail and save() to raise as expected.

comment:2 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

A couple of things about this patch:

  1. validate() doesn't work totally at the moment, so documenting it's existence would be premature. That is work-in-progress.
  2. We should try to work out 'which' types of error save() might raise, if possible, so that code can catch them if they want to. IntegrityError from the db layer is one.

I'm not totally onboard with MySQL's idea that storing inconsistent data is only a warning and not an error. I'm not sure we're behaving incorrectly there, at least in the sense that trying to save invalid data is bad.

Changed 7 years ago by PhiR

comment:3 Changed 7 years ago by PhiR

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin
  1. removed the reference to validate()
  2. I added the exceptions I would find, and mentionned that it'd DB dependent.

Until we stop relying on the DB to do the checking for us there's nothing more we can do here.

comment:4 Changed 7 years ago by telenieko

  • Triage Stage changed from Ready for checkin to Accepted

Saying "database independant" is a bit ugly, we should catch the different exceptions and raise one common for every backend unless we want people to either catch only the one they care about (the backend they're using) or have to deal with all of them.

Anyway I'd go for setting up "traditional mode" in MySQL by default... here, note the joke in "Make MySQL behave like a “traditional” SQL database system. A simple description of this mode is “give an error instead of a warning” when inserting an incorrect value into a column."

comment:5 Changed 5 years ago by adamnelson

  • Patch needs improvement set

Seems like there's still dispute on the best way to solve this.

comment:6 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:9 Changed 2 years ago by timo

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top