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 Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      35858

      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.

        Activity

        Hide
        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
        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
        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
        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
        Petr Škoda added a comment -

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

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

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

        Show
        Eloy Lafuente (stronk7) added a comment - ah, I was thinking really "private", cannot that be done?
        Hide
        Petr Škoda 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
        Petr Škoda 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
        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
        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
        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
        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
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        Eloy Lafuente (stronk7) added a comment -

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

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

        Done!

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved: