search

Opencast-Moodle / moodle-local_chunkupload

Plugin to upload big files to Moodle
6 stars 6 forks source link

strange non-standard use of ID field #11

Open danmarsden opened 4 years ago

danmarsden commented 4 years ago

This is not typical of a Moodle table: https://github.com/learnweb/moodle-local_chunkupload/blob/master/classes/chunkupload_form_element.php#L246

It also feels a bit misleading to call this a "token" when it's just a random number.

I can't see us blocking approval in the plugins db on this point, but I'd expect the integration team to have something to say about it...

Laur0r commented 4 years ago

Hey Dan, thanks for your feedback! The usage of the id field for the token was maybe not an intuitive design decision. However, we do not see problems with this decision. If you consider it a bigger issue, we can adjust it. We have the plugin running and working and did not experience any problems with this. Furthermore, we did not find any application case which would require an additional id to the token. Please comment whether we should change that before having a new release candidate for the plugin directory. Cheers

danmarsden commented 3 years ago

the fact that no other Moodle tables work like this (not using sequence based id field) suggests that it is likely to trip someone up along the way, @mudrd8mz - can you think of any other potential issues with this implementation?

mudrd8mz commented 3 years ago

Thank you Dan for pinging me.

Coincidentally I've been researching the question of tables primary keys recently while working on the Moodle Plugin Development Basics. There is a place in the docs the explicitly says that every table must have auto-incremented int10 field used as the primary key "id" - see the very first point at https://docs.moodle.org/dev/Database. There is also a record of older discussions on this topic linked on that page leading to https://docs.moodle.org/dev/IdColumnReasons - although strictly speaking, the conclusion there is that every table just must have some primary key that would also work as an array key of the returned lists.

So yeah. I would personally not do it and I would discourage from doing it just because it is somewhat exceptional. There can be a situation or code that simply does not expect that. Various hard-to-debug things can happen, e.g. if some code somewhere casted these random strings to integers etc.

Additionally, the way it is currently implemented is still subject of potential race-condition (two processes both generate same random number at the same time and use it as ID) and even though we program web based LMS and not a control system for nuclear reactors, it can be easily seen suspicious. It is one of those things that people raise when they review plugins internally before deciding whether to deploy them in their institutions (just like you did here).

But after all as always, we are here to give well meant advises, not to block plugins from being published.

danmarsden commented 3 years ago

thanks David!

@Laur0r - David and I both agree that this won't block approval in the plugins db but we still encourage you to improve it to comply better with the normal guidelines.

Feel free to "ignore" that advice and just fix up #10 which I think was the main thing I wanted to see tidied up before approval.

thanks!