Moodle Community Sites
  1. Moodle Community Sites
  2. MDLSITE-1969

Plugins database rejects submissions based in DB table name

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Component/s: moodle.org/plugins
    • Labels:
      None
    • Rank:
      45084

      Description

      I just tried to upload a new plugin to the plugins database. But it was rejected because the table name did not start with the frankenstyle component name. The problem is that since Moodle 2.3, table names can be no longer than 28 letters. But if your plugin is a 2.3 assignment submission type, the frankenstyle name must begin with the whopping long assignsubmission_ . My plugin's frankenstyle name is assignsubmission_onlinepoodll , but I had to change the table name to assignsubmission_onlinepood , to get under 28 characters. So the frankenstyle name is longer than the table name.

      The end result is that I can't upload my plugin to the database, without changing the frankenstyle name, and so a whole lot of code. Worse all the users who have installed it already won't be able to upgrade out of the situation because the newly named plugin will be identified as a completely different plugin.

      Do we really need to have this check? Or can we loosen it?

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          Thanks for the report Justin.
          I think for the short term we could possibly relax this but i don't think that's a proper solution so i'll get a few watchers here.

          I can see this becoming quiet annoying with our current limitations nd as you've put it upgrades will be affected which really means the upgrading infrastructure is in effect broken now for some plugins. I'll have to check how many plugins might have been affected here..

          I think we should have some convention that saves table name lengths within core so that theres more space for other plugins since we know what we've got in core and conflicts are less likely within core than when compared to the variations possible for plugins...

          Show
          Aparup Banerjee added a comment - Thanks for the report Justin. I think for the short term we could possibly relax this but i don't think that's a proper solution so i'll get a few watchers here. I can see this becoming quiet annoying with our current limitations nd as you've put it upgrades will be affected which really means the upgrading infrastructure is in effect broken now for some plugins. I'll have to check how many plugins might have been affected here.. I think we should have some convention that saves table name lengths within core so that theres more space for other plugins since we know what we've got in core and conflicts are less likely within core than when compared to the variations possible for plugins...
          Hide
          Martin Dougiamas added a comment -

          I think it's fine to just make the check better so that it only checks the first 28 characters.

          Show
          Martin Dougiamas added a comment - I think it's fine to just make the check better so that it only checks the first 28 characters.
          Hide
          Justin Hunt added a comment -

          Yes, that would be a good solution to the problem.

          I will probably still have to change my table name, since I cut it at assignsubmission_onlinepood rather then the possibly confusing assignsubmission_onlinepoodl . But that is much better than changing the frankenstyle name.

          Show
          Justin Hunt added a comment - Yes, that would be a good solution to the problem. I will probably still have to change my table name, since I cut it at assignsubmission_onlinepood rather then the possibly confusing assignsubmission_onlinepoodl . But that is much better than changing the frankenstyle name.
          Hide
          Justin Hunt added a comment -

          Its pushing 2 months now. I do know there are a lot of issues out there, and this is just one. But it would only take 5 minutes, and the PoodLL Assignment Submission Type is in limbo till its done. And it would double the number of 2.3 assignment submission types available in the plugins database.....

          And if that's not enough, I promise to write a tutorial on how to write a Moodle 2.3 assignment submission type. But I do know there are a lot of issues out there, and this is just one....

          Show
          Justin Hunt added a comment - Its pushing 2 months now. I do know there are a lot of issues out there, and this is just one. But it would only take 5 minutes, and the PoodLL Assignment Submission Type is in limbo till its done. And it would double the number of 2.3 assignment submission types available in the plugins database..... And if that's not enough, I promise to write a tutorial on how to write a Moodle 2.3 assignment submission type. But I do know there are a lot of issues out there, and this is just one....
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well... really I don't get the point about enforcing the complete match at all. Or I'm wrong or that restriction (table names must start with the complete component name) does not exist in moodle since ages ago. Although I can see the point about doing so in contrib plugins (to avoid conflicts).

          Also, it sounds a bit crazy to "reserve" 17 characters just for the plugin type ("assignsubmission_"). That makes everybody to run out so easily... at some point I think we should reduce that to, say, "assignsub_" (10 chars) or so. That would make things far easier for contributions.

          Finally, perhaps, we could explore some solution, like allowing contrib plugins to declare their "prefix space" (say 1-10 chars), for example Justin decides he's going to use "justin_" for his "assignsubmission_onlinepood" tables. And then he goes and register the plugin and the "prefix space", so nobody else can use it. And then the validator verifies that all the tables are using that "prefix space".

          So, in summary:

          1) Let's review and reduce some component names.
          2) By default, enforce component prefix (as it's now) by the checker.
          3) Allow the contributor to specify one alternative, unique, not-changeable, prefix (for DB tables only). Enforce it by the checker.

          Just my 2 cents...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well... really I don't get the point about enforcing the complete match at all. Or I'm wrong or that restriction (table names must start with the complete component name) does not exist in moodle since ages ago. Although I can see the point about doing so in contrib plugins (to avoid conflicts). Also, it sounds a bit crazy to "reserve" 17 characters just for the plugin type ("assignsubmission_"). That makes everybody to run out so easily... at some point I think we should reduce that to, say, "assignsub_" (10 chars) or so. That would make things far easier for contributions. Finally, perhaps, we could explore some solution, like allowing contrib plugins to declare their "prefix space" (say 1-10 chars), for example Justin decides he's going to use "justin_" for his "assignsubmission_onlinepood" tables. And then he goes and register the plugin and the "prefix space", so nobody else can use it. And then the validator verifies that all the tables are using that "prefix space". So, in summary: 1) Let's review and reduce some component names. 2) By default, enforce component prefix (as it's now) by the checker. 3) Allow the contributor to specify one alternative, unique, not-changeable, prefix (for DB tables only). Enforce it by the checker. Just my 2 cents...ciao
          Hide
          Petr Škoda added a comment -

          Do not dare to change existing component names! The main reason for component prefixes is prevention of collisions between contrib plugins and core. It is also used during uninstallation. It helps developers too.

          You are wrong Eloy, it was ALWAYS required to use proper frankenstyle prefix in table names. Please stop breaking stuff because of the unsupportable Oracle and let's hack around the table length limit inside Oracle driver only. It is technically possible, who cares how ugly it looks...

          Show
          Petr Škoda added a comment - Do not dare to change existing component names! The main reason for component prefixes is prevention of collisions between contrib plugins and core. It is also used during uninstallation. It helps developers too. You are wrong Eloy, it was ALWAYS required to use proper frankenstyle prefix in table names. Please stop breaking stuff because of the unsupportable Oracle and let's hack around the table length limit inside Oracle driver only. It is technically possible, who cares how ugly it looks...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm not wrong, Petr. Both install and uninstall do their work from XMLDB DEFINITIONS (in fact I think that you proposed that like 2 years ago, from the previous "blind, by component" alternative). And later, fallback to components and/or orphaned is used (that said from memory).

          About renaming some plugins to shorter versions, I would not call it at all "break" anything. Just rename to give more space to developers.

          And about Oracle, yes, it's the culprit. But why should we hack it so badly? To have another reason to blame it later? 30 should be pretty enough for any table name (it's > 3 regular full words in English). The problem is that just the component is "eating" 2/3 of that total length.

          So I don't retire my 2 cents above (nor impose them). Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm not wrong, Petr. Both install and uninstall do their work from XMLDB DEFINITIONS (in fact I think that you proposed that like 2 years ago, from the previous "blind, by component" alternative). And later, fallback to components and/or orphaned is used (that said from memory). About renaming some plugins to shorter versions, I would not call it at all "break" anything. Just rename to give more space to developers. And about Oracle, yes, it's the culprit. But why should we hack it so badly? To have another reason to blame it later? 30 should be pretty enough for any table name (it's > 3 regular full words in English). The problem is that just the component is "eating" 2/3 of that total length. So I don't retire my 2 cents above (nor impose them). Ciao
          Hide
          Petr Škoda added a comment - - edited

          You simply can not rename components, it would break all over the place starting with amos and ending with config tables. And no, you are wrong, frankenstyle rules apply to database tables too.

          Oracle support seems to be good for advertising only, so who cares what mess is inside the driver if it helps everybody else? Every identifier in SQL over 30 chars would be half-hashed-truncated and problem solved, it is not that difficult to parse our subset of SQL syntax and we could cache the results in MUC...

          Show
          Petr Škoda added a comment - - edited You simply can not rename components, it would break all over the place starting with amos and ending with config tables. And no, you are wrong, frankenstyle rules apply to database tables too. Oracle support seems to be good for advertising only, so who cares what mess is inside the driver if it helps everybody else? Every identifier in SQL over 30 chars would be half-hashed-truncated and problem solved, it is not that difficult to parse our subset of SQL syntax and we could cache the results in MUC...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm sure components can be renamed, it can be more or less complex, but for sure it's computable. 100%. Aren't we able to detect all their uses? Once done, it's a simple update for each.

          About Oracle, yes, partially hashing the long names may be a solution, but are you sure that parsing tablenames in all SQLs is also 100% computable? And they are not only tables, but columns, aliases... not to talk about how SQLs will look like with those looong table names allowed.

          About install and uninstall, sorry, but BOTH are done from xmldb definitions. Only on uninstall, if the admin has deleted the code before uninstalling, the fallback to component prefix is used. If the xmldb definition is still there then the component is unused. At some point we could enforce the need to uninstall always properly, i.e. before deleting the code and that would make, automatically, xmldb definitions to be independent of official component names.

          In any case it would be really easy to say that "asssub" is a (reserved) equivalent for "assignsubmission" (only to be applied to table names). Allowing people to have still > 20 chars for their subplugin tables. That without changing the name of the component nor by changing the 28/30 chars limit. Just introduce the concept of "allowed reserved equivalents for DB tables". Just that.

          Show
          Eloy Lafuente (stronk7) added a comment - I'm sure components can be renamed, it can be more or less complex, but for sure it's computable. 100%. Aren't we able to detect all their uses? Once done, it's a simple update for each. About Oracle, yes, partially hashing the long names may be a solution, but are you sure that parsing tablenames in all SQLs is also 100% computable? And they are not only tables, but columns, aliases... not to talk about how SQLs will look like with those looong table names allowed. About install and uninstall, sorry, but BOTH are done from xmldb definitions. Only on uninstall, if the admin has deleted the code before uninstalling, the fallback to component prefix is used. If the xmldb definition is still there then the component is unused. At some point we could enforce the need to uninstall always properly, i.e. before deleting the code and that would make, automatically, xmldb definitions to be independent of official component names. In any case it would be really easy to say that "asssub" is a (reserved) equivalent for "assignsubmission" (only to be applied to table names). Allowing people to have still > 20 chars for their subplugin tables. That without changing the name of the component nor by changing the 28/30 chars limit. Just introduce the concept of "allowed reserved equivalents for DB tables". Just that.
          Hide
          Dan Poltawski added a comment -

          In any case it would be really easy to say that "asssub" is a (reserved) equivalent for "assignsubmission" (only to be applied to table names).

          +1, in fact I suggested the same thing in MDL-36739

          Show
          Dan Poltawski added a comment - In any case it would be really easy to say that "asssub" is a (reserved) equivalent for "assignsubmission" (only to be applied to table names). +1, in fact I suggested the same thing in MDL-36739
          Hide
          Justin Hunt added a comment -

          Hmmm....I had simplistically thought this was a problem limited to the plugins submission process.

          I think that the 28 character limit on Oracle table names is a drag, but that table names shouldn't really get that long anyway. The problem here is that some component names are really long in this case and I guess in one or two other cases. If it is possible to change component names once and for all, then assign_submission, should really be assign_sub. But I would think that were quite a task for a problem that only affects a small number of people.

          Having reserved equivalent table names is not a bad idea. Is Martin's proposal, that the table name be the first 28 characters of the component name though really different to this? This would be pretty simple to implement, and relatively intuitive. If this change was accompanied by a change in the plugin install process that added a check for consistency on this, plugin devs would hit the error much earlier than they do now. ie now the check is in the plugin submission process, but not the install process.

          Show
          Justin Hunt added a comment - Hmmm....I had simplistically thought this was a problem limited to the plugins submission process. I think that the 28 character limit on Oracle table names is a drag, but that table names shouldn't really get that long anyway. The problem here is that some component names are really long in this case and I guess in one or two other cases. If it is possible to change component names once and for all, then assign_submission, should really be assign_sub. But I would think that were quite a task for a problem that only affects a small number of people. Having reserved equivalent table names is not a bad idea. Is Martin's proposal, that the table name be the first 28 characters of the component name though really different to this? This would be pretty simple to implement, and relatively intuitive. If this change was accompanied by a change in the plugin install process that added a check for consistency on this, plugin devs would hit the error much earlier than they do now. ie now the check is in the plugin submission process, but not the install process.
          Hide
          David Mudrak added a comment -

          I can fully understand the reporter's frustration. I have similar issue with Workshop module subplugins. Not being much aware of consequences, I called one of the subplugins 'workshopallocation' (18 chars) which does not leave too much space for subplugin name and the table name, separated by underscores Lesson learnt, but does not help too much to solve current issues.

          I can see Petr's point, too. Being able to easily decide what plugin/subsystem a given table belongs to is a must - even when the plugin's code is not available any more, thence purely name-based.

          My +1 to revert the time flow, go back to the past, prevent introducing these long subplugin names, and http://xkcd.com/1063/

          Show
          David Mudrak added a comment - I can fully understand the reporter's frustration. I have similar issue with Workshop module subplugins. Not being much aware of consequences, I called one of the subplugins 'workshopallocation' (18 chars) which does not leave too much space for subplugin name and the table name, separated by underscores Lesson learnt, but does not help too much to solve current issues. I can see Petr's point, too. Being able to easily decide what plugin/subsystem a given table belongs to is a must - even when the plugin's code is not available any more, thence purely name-based. My +1 to revert the time flow, go back to the past, prevent introducing these long subplugin names, and http://xkcd.com/1063/
          Hide
          Anthony Borrow added a comment -

          Just throwing out ideas, this seems a bit like the DOS (as in the OS) file naming issue (sorry to date myself) when extended names began to be supported but the underlying system would not allow it. Would a similar approach be helpful where we have the actual name being used in the database (which would be limited to 28 characters) but one that does not change the actual, longer, official name of the plugin? Because of the impact that this is having on addons I do believe the priority should be bumped that we move toward some type of resolution. Peace - Anthony

          Show
          Anthony Borrow added a comment - Just throwing out ideas, this seems a bit like the DOS (as in the OS) file naming issue (sorry to date myself) when extended names began to be supported but the underlying system would not allow it. Would a similar approach be helpful where we have the actual name being used in the database (which would be limited to 28 characters) but one that does not change the actual, longer, official name of the plugin? Because of the impact that this is having on addons I do believe the priority should be bumped that we move toward some type of resolution. Peace - Anthony
          Hide
          Aparup Banerjee added a comment -

          yes Anthony, some mapping will be needed. i've just linked MDL-36739.

          Show
          Aparup Banerjee added a comment - yes Anthony, some mapping will be needed. i've just linked MDL-36739 .
          Hide
          Aparup Banerjee added a comment -

          Alright, this hasn't moved at all so i've just done the suggested fix :
          table name prefix checks are only against the first 28 characters of a component name. This does increase likelihood of clashes but hopefully not at all that often.
          In any case the proper fix would happen somewhere along the chain in MDL-36739.

          Hopefully more plugins can atleast be uploaded now.

          Show
          Aparup Banerjee added a comment - Alright, this hasn't moved at all so i've just done the suggested fix : table name prefix checks are only against the first 28 characters of a component name. This does increase likelihood of clashes but hopefully not at all that often. In any case the proper fix would happen somewhere along the chain in MDL-36739 . Hopefully more plugins can atleast be uploaded now.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development