Moodle
  1. Moodle
  2. MDL-29516 DB layer improvements 2.3 META
  3. MDL-32112

implement xmldb editor validation also in database_manager for tables and fields

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Rank:
      38822

      Issue Links

        Activity

        Hide
        Petr Škoda added a comment -

        arrgh, somebody creative did not use XMLDB editor and managed to sneak in

        <FIELD NAME="multiplier" TYPE="number" LENGTH="40" NOTNULL="true" DEFAULT="1.00000000000000000000" SEQUENCE="false" DECIMALS="20" COMMENT="The multiplier for this unit. For example, if the first unit is (1.0, 'cm'), another unit might be (0.1, 'mm') or (100.0, 'm')." PREVIOUS="question" NEXT="unit"/>
        

        in /question/type/numerical/db/install.xml

        Show
        Petr Škoda added a comment - arrgh, somebody creative did not use XMLDB editor and managed to sneak in <FIELD NAME= "multiplier" TYPE= "number" LENGTH= "40" NOTNULL= " true " DEFAULT= "1.00000000000000000000" SEQUENCE= " false " DECIMALS= "20" COMMENT= "The multiplier for this unit. For example, if the first unit is (1.0, 'cm'), another unit might be (0.1, 'mm') or (100.0, 'm')." PREVIOUS= "question" NEXT= "unit" /> in /question/type/numerical/db/install.xml
        Hide
        Petr Škoda added a comment -

        I am going to create new issue for invalid size in number column in numerical question type...

        This change might be a big surprise for users that do not bother to validate manual changes in install.xml through XMLDB editor. Eloy was telling this over and over again, this should teach them something...

        Show
        Petr Škoda added a comment - I am going to create new issue for invalid size in number column in numerical question type... This change might be a big surprise for users that do not bother to validate manual changes in install.xml through XMLDB editor. Eloy was telling this over and over again, this should teach them something...
        Hide
        Tim Hunt added a comment -

        +1 for the concept this is a good idea.

        However, I have a problem with the implementation. It seems that you have re-implemented the checks in new code. You have not re-factored so that the XMLDB editor uses the same validation code. This seems to be a recipe for the two separate lots of valididation code to become different in future.

        I think you should refactor.

        I also think it would have been a good idea to have some debate about this (e.g. DGF or developers chat) before submitting the change for integration.

        At the very least, you should add something to lib/upgrade.txt.

        Show
        Tim Hunt added a comment - +1 for the concept this is a good idea. However, I have a problem with the implementation. It seems that you have re-implemented the checks in new code. You have not re-factored so that the XMLDB editor uses the same validation code. This seems to be a recipe for the two separate lots of valididation code to become different in future. I think you should refactor. I also think it would have been a good idea to have some debate about this (e.g. DGF or developers chat) before submitting the change for integration. At the very least, you should add something to lib/upgrade.txt.
        Hide
        Petr Škoda added a comment -

        Hi, yes, the validation in lib/xmldb was designed to be later used in the XMLDB editor. Hmm, I should make constants for the length limits now. I am not going to refactor the XMLDB editor now, we have discussed it already with Eloy when I introduced the new validation method.

        Please note this is not a change in DDL api, it was already mandatory to use XMLDB editor for ALL operations on install.xml files, NOBODY is supposed to modify these files directly. Eloy keeps repeating that over and over and over and over again.

        And yes, I did propose this new validation because I wanted to annoy/punish developers that completely ignored XMLDB editor.

        Summary:

        • reopening and adding constants for length limits and using them in xmldbeditor (the same way as char field limits).
        • I am going to post in dev forum to warn developers again that manually editing install.xml is wrong

        thanks for the review!

        Show
        Petr Škoda added a comment - Hi, yes, the validation in lib/xmldb was designed to be later used in the XMLDB editor. Hmm, I should make constants for the length limits now. I am not going to refactor the XMLDB editor now, we have discussed it already with Eloy when I introduced the new validation method. Please note this is not a change in DDL api, it was already mandatory to use XMLDB editor for ALL operations on install.xml files, NOBODY is supposed to modify these files directly. Eloy keeps repeating that over and over and over and over again. And yes, I did propose this new validation because I wanted to annoy/punish developers that completely ignored XMLDB editor. Summary: reopening and adding constants for length limits and using them in xmldbeditor (the same way as char field limits). I am going to post in dev forum to warn developers again that manually editing install.xml is wrong thanks for the review!
        Hide
        Petr Škoda added a comment -

        I will wait for Eloy and discuss the limits with him, ciao.

        Show
        Petr Škoda added a comment - I will wait for Eloy and discuss the limits with him, ciao.
        Hide
        Petr Škoda added a comment -

        I have pushed one more commit that adds constants for the limits.

        Show
        Petr Škoda added a comment - I have pushed one more commit that adds constants for the limits.
        Hide
        Petr Škoda added a comment -

        rebased and added the limits to XMLDB editor the same way we did CHAR length limits recently

        Show
        Petr Škoda added a comment - rebased and added the limits to XMLDB editor the same way we did CHAR length limits recently
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Added one extra commit to fix mysql sql generator that was leading to one (interim) non-valid field name on rename.

        Show
        Eloy Lafuente (stronk7) added a comment - Added one extra commit to fix mysql sql generator that was leading to one (interim) non-valid field name on rename.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Grrr, installation failed. It seems that MDL-27469 arrived with another manual edition of install.xml files, leading to one non-supported definition.

        See http://tracker.moodle.org/browse/MDL-27469?focusedCommentId=150307#comment-150307

        About to fix it in separate issue...

        Show
        Eloy Lafuente (stronk7) added a comment - Grrr, installation failed. It seems that MDL-27469 arrived with another manual edition of install.xml files, leading to one non-supported definition. See http://tracker.moodle.org/browse/MDL-27469?focusedCommentId=150307#comment-150307 About to fix it in separate issue...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Done, MDL-32202 has fixed the wrong definitions.

        So, for testers:

        1) DB tests are passing under mysqli, postgres, mssql and oracle. So only sqlsrv is pending, I've not it here.
        2) Install and upgrade is working ok under mysql, feel free to test in another DB.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Done, MDL-32202 has fixed the wrong definitions. So, for testers: 1) DB tests are passing under mysqli, postgres, mssql and oracle. So only sqlsrv is pending, I've not it here. 2) Install and upgrade is working ok under mysql, feel free to test in another DB. Ciao
        Hide
        Petr Škoda added a comment -

        thanks!

        Show
        Petr Škoda added a comment - thanks!
        Hide
        Aparup Banerjee added a comment -

        test passing : installs + upgrades from 2.1 --> master (integration copies) all work fine on SQSRV.

        however i'm facing problem with function DB unit tests so i could not complete that, this problem s also in upstream already anyway:

        Fatal error: Maximum execution time of 30 seconds exceeded in C:\server\workspace\mssql\lib\dml\sqlsrv_native_moodle_database.php on line 1330

        some thing about stored procedure sp_getapplock there.

        i'll open another issue for the above.

        Show
        Aparup Banerjee added a comment - test passing : installs + upgrades from 2.1 --> master (integration copies) all work fine on SQSRV. however i'm facing problem with function DB unit tests so i could not complete that, this problem s also in upstream already anyway: Fatal error: Maximum execution time of 30 seconds exceeded in C:\server\workspace\mssql\lib\dml\sqlsrv_native_moodle_database.php on line 1330 some thing about stored procedure sp_getapplock there. i'll open another issue for the above.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has landed upstream, finally! Yay!

        תודה רבה && شكرا جزيلا



        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has landed upstream, finally! Yay! תודה רבה && شكرا جزيلا Closing, ciao
        Hide
        Fred Woolard added a comment - - edited

        I'd like to ask about the char/varchar column limit value CHAR_MAX_LENGTH, if I may. Why 1333? Especially in view of the docblock mentioning the four most likely to be used DB engines having a max column length well above 1333.

        Please forgive for my ignorance if it's otherwise, but is 1333 a magic number?

        I've written an enrollment plugin that makes use of staging tables to store IMS-based messages. The IMS spec has the course description max length at 2048.

        Rather than have to create a text/clob column, it is preferable I think to create a varchar2 column. I realize the likelihood of an IMS course description exceeding 1000 or so characters is slim, but not everybody adheres the specs.

        I'd like to suggest the CHAR_MAX_LENGTH value be adjusted to 4000 to match the Oracle max length.

        Submitted MDL-33748

        Show
        Fred Woolard added a comment - - edited I'd like to ask about the char/varchar column limit value CHAR_MAX_LENGTH, if I may. Why 1333? Especially in view of the docblock mentioning the four most likely to be used DB engines having a max column length well above 1333. Please forgive for my ignorance if it's otherwise, but is 1333 a magic number? I've written an enrollment plugin that makes use of staging tables to store IMS-based messages. The IMS spec has the course description max length at 2048. Rather than have to create a text/clob column, it is preferable I think to create a varchar2 column. I realize the likelihood of an IMS course description exceeding 1000 or so characters is slim, but not everybody adheres the specs. I'd like to suggest the CHAR_MAX_LENGTH value be adjusted to 4000 to match the Oracle max length. Submitted MDL-33748
        Hide
        Petr Škoda added a comment -

        answer is: crazy Oracle database limitations, sorry

        Show
        Petr Škoda added a comment - answer is: crazy Oracle database limitations, sorry

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: