Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-14679 META: DB layer 2.0
  3. MDL-20734

insert/update/set_field expected behaviour is to cast empty string to 0 for numeric columns

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Database SQL/XMLDB
    • Labels:
      None
    • Database:
      MySQL, PostgreSQL, Microsoft SQL, Oracle
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Expected behaviour to get cross-db support is to automatically cast empty strings to 0 in the following functions:

      • insert_record
      • update_record
      • set_field

      Rationale: DBs are idiot:

      • MSSQL allows empties on int fields and saves them as 0, but not in decimal fields no matter of the nullability of the field)
      • Oracle allows empties on int fields and saves them as NULL and only in NOT NULL fields. Same for decimal fields.
      • MySQL doesn't allow empties in int/decimal fields (strict mode). Error.
      • PG doesn't allow empties in int/decimal fields). Error.

      So I guess we only can move to the "perform casting" alternative. Is the only that will work cross-db. The other (not perform casting) won't work as far as MSSQL and Oracle sometimes accept those empty strings.

        Gliffy Diagrams

          Activity

          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Test now are checking the expected behaviour (cast empty to 0). These need fix:

          MySQL: insert/update/set_field
          PostgreSQL: set_field

          coming soon...ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Test now are checking the expected behaviour (cast empty to 0). These need fix: MySQL: insert/update/set_field PostgreSQL: set_field coming soon...ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Done,

          implemented the normalise_value() function in mysql and postgresql drives (was already in mssql and oracle) that performs all the default "normalisation" like casting empty strings to cero when working with numeric columns.

          Tests passing now.

          Resolving as fixed.

          Petr, my only concern is if we should, in moodle_database.php:

              /**
               * Normalise values based in RDBMS dependencies (booleans, LOBs...)
               *
               * @param database_column_info $column column metadata corresponding with the value we are going to normalise
               * @param mixed $value value we are going to normalise
               * @return mixed the normalised value
               */
              private abstract function normalise_value($column, $value);

          in order to "force" drivers implementators to have such good habit.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Done, implemented the normalise_value() function in mysql and postgresql drives (was already in mssql and oracle) that performs all the default "normalisation" like casting empty strings to cero when working with numeric columns. Tests passing now. Resolving as fixed. Petr, my only concern is if we should, in moodle_database.php: /** * Normalise values based in RDBMS dependencies (booleans, LOBs...) * * @param database_column_info $column column metadata corresponding with the value we are going to normalise * @param mixed $value value we are going to normalise * @return mixed the normalised value */ private abstract function normalise_value($column, $value); in order to "force" drivers implementators to have such good habit. Ciao
          Hide
          skodak Petr Skoda added a comment -

          you mean "protected abstract" right? +10 for that

          Show
          skodak Petr Skoda added a comment - you mean "protected abstract" right? +10 for that
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          ah, I was thinking really "private", cannot that be done?

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - ah, I was thinking really "private", cannot that be done?
          Hide
          skodak Petr Skoda added a comment - - edited

          private + abstract? looks weird, I doubt you would be able to call it from moodel_database if defined in native drivers. What other use would it have?

          I did not test it, it might actually work in this weird PHP OOP model

          Show
          skodak Petr Skoda added a comment - - edited private + abstract? looks weird, I doubt you would be able to call it from moodel_database if defined in native drivers. What other use would it have? I did not test it, it might actually work in this weird PHP OOP model
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          private + abstract means that ANY DB driver must implement it and only can be used by it. That's what I was exactly trying to do. And has sense. IMO.

          If we do it protected that means that potential childs will be able to use it, but are we going to have childs of native drivers? I don't think so.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - private + abstract means that ANY DB driver must implement it and only can be used by it. That's what I was exactly trying to do. And has sense. IMO. If we do it protected that means that potential childs will be able to use it, but are we going to have childs of native drivers? I don't think so.
          Hide
          mudrd8mz David Mudrak added a comment -

          I do not know if it applies here too but usually I saw protected instead of private for the UnitTesting purposes - ie if you need to have a testable subclass of the native driver. I suppose the DB testing scheme may be different though.

          Show
          mudrd8mz David Mudrak added a comment - I do not know if it applies here too but usually I saw protected instead of private for the UnitTesting purposes - ie if you need to have a testable subclass of the native driver. I suppose the DB testing scheme may be different though.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I don't think unit testing is the key here. We aren't going to have one test_normalise_value() function at all, because it will be covered by all the insert/update/set_field tests instead.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I don't think unit testing is the key here. We aren't going to have one test_normalise_value() function at all, because it will be covered by all the insert/update/set_field tests instead.
          Hide
          skodak Petr Skoda added a comment -

          I do not like forcing people to define something private in drivers - private measn do it in any way you like which imo includes not use nay function for that at all

          Show
          skodak Petr Skoda added a comment - I do not like forcing people to define something private in drivers - private measn do it in any way you like which imo includes not use nay function for that at all
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          That's a good point, oki, declaring it as protected abstract. Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - That's a good point, oki, declaring it as protected abstract. Ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Done!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Done!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10