Moodle
  1. Moodle
  2. MDL-21828

SQL error with ambiguous column definition

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.7
    • Fix Version/s: 1.9.9
    • Component/s: Questions
    • Labels:
      None
    • Database:
      Any
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      26634

      Description

      There is an invalid query in the ./lib/question.php file with a reference to an ambiguous column definition which appears twice in the select. Oracle will not accept this as a valid query and errors with :

      SQL ORA-00918: column ambiguously defined in /usr/local/moodle-src/rel-20100113-4/lib/dmllib.php on line 481. STATEMENT: SELECT n.questionid as question, s.*, n.sumpenalty FROM m_question_states s, m_question_sessions n WHERE s.id = n.newgraded AND n.attemptid = '67706' AND n.questionid = '31777' with limits (-1, 1), referer: http://moodle.cqu.edu.au/mod/quiz/view.php?id=47336

      I have fixed this in the two places within that file by removing the reference to the duplicated column and have included a patch.

      Also, i have not submitted a patch before so i would be willing to have any comments about format of patch....etc??

      1. patch_ambiguous_col_2.0.patch
        2 kB
        Tim Hunt
      2. patch_ambiguous_col.patch
        2 kB
        Tim Hunt
      3. patch_ambiguous_col.patch
        2 kB
        David Binney
      4. patch_lib_questionlib.php
        1 kB
        David Binney

        Activity

        Hide
        Tim Hunt added a comment -

        That patch won't work. In SQL queries in Moodle, the first column is significant. It gets used to index the array that is returned.

        I think the correct solution it to explicitly write out all the s.... columns (except for question) to avoid the duplication.

        The patch looks find. The only comment I would make is that it is best to give it a file extension different from .php. I often use .patch.txt.

        Show
        Tim Hunt added a comment - That patch won't work. In SQL queries in Moodle, the first column is significant. It gets used to index the array that is returned. I think the correct solution it to explicitly write out all the s.... columns (except for question) to avoid the duplication. The patch looks find. The only comment I would make is that it is best to give it a file extension different from .php. I often use .patch.txt.
        Hide
        David Binney added a comment -

        Ok cool, I was not sure of the implication or why it was done this way. I will update the code again and re-create the patch for you.

        Show
        David Binney added a comment - Ok cool, I was not sure of the implication or why it was done this way. I will update the code again and re-create the patch for you.
        Hide
        David Binney added a comment -

        This is the updated patch file with the changes specified in the tracker

        Show
        David Binney added a comment - This is the updated patch file with the changes specified in the tracker
        Hide
        David Binney added a comment -

        Hey Tim, I made the specified changes but was just unsure of the replacement of the questionid for the quesiton but i am sure you will let me know if that is no good

        Show
        David Binney added a comment - Hey Tim, I made the specified changes but was just unsure of the replacement of the questionid for the quesiton but i am sure you will let me know if that is no good
        Hide
        Tim Hunt added a comment -

        Well, tabs for indent a bad (according to Development:Coding_style#Indentation). But apart from that it looks fine. I will try to commit it soon, but today is testing day before the weekly build.

        Show
        Tim Hunt added a comment - Well, tabs for indent a bad (according to Development:Coding_style#Indentation). But apart from that it looks fine. I will try to commit it soon, but today is testing day before the weekly build.
        Hide
        David Binney added a comment -

        Good eyes tim. Since switching to the mac I have not even looked for switch spaces to tabs on textmate (soft tabs).. damn

        Show
        David Binney added a comment - Good eyes tim. Since switching to the mac I have not even looked for switch spaces to tabs on textmate (soft tabs).. damn
        Hide
        Tim Hunt added a comment -

        Oops, I forgot all about this.

        Attached is a cleaned up patch. I was about to commit this to CVS, when I remembered that it is testing day again.

        Hopefully I will remember tomorrow.

        Show
        Tim Hunt added a comment - Oops, I forgot all about this. Attached is a cleaned up patch. I was about to commit this to CVS, when I remembered that it is testing day again. Hopefully I will remember tomorrow.
        Hide
        Tim Hunt added a comment -

        And I think this is the correct merge to Moodle 2.0, where the code has changed a bit.

        Show
        Tim Hunt added a comment - And I think this is the correct merge to Moodle 2.0, where the code has changed a bit.
        Hide
        Tim Hunt added a comment -

        Finally committed. Thanks David.

        Show
        Tim Hunt added a comment - Finally committed. Thanks David.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: