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:

      Gliffy Diagrams

        Issue Links

          Activity

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

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

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

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

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

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

          Show
          Petr Skoda 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 Skoda added a comment -

          thanks!

          Show
          Petr Skoda 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 Skoda added a comment -

          answer is: crazy Oracle database limitations, sorry

          Show
          Petr Skoda 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: