[D7] Tripal Multi-Chado extension [#2825986]

Context:
Tripal is a toolkit for construction of online biological (genetics, genomics, breeding, etc), community database. Tripal provides by default integration with the GMOD Chado database schema (PostgreSQL) and Drupal. When you use Tripal module, it only allows the use of one single Chado instance to manage biological data.

Purpose:
This module enables the use of several Chado schema instances inside a same database used by Drupal/Tripal. It answers to a common feature request by research institutes and companies: being able to serve both public and private biological data on a same site (sharing a same configuration, users, tools,...).
It also allows a site to serve several public Chado instances, for instance when hosting several versions of a genome or several species.

Note: if you are not familiar with PostgreSQL, then you need to know that a PostgreSQL database can store several "schema" that can be seen as "sub-databases". With Tripal module, Drupal database is stored in the "public" schema and Chado in the "chado" schema. Other schemata are also instantiated by Tripal and used by Chado: "frange", "genetic_code" and "so". The Multi-Chado module simply adds the possibility to create other Chado schema and use them just like if they where the "chado" schema.

Project page: https://www.drupal.org/sandbox/guignonv/2429515

Git clone:

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/guignonv/2429515.git tripal_multi_chado
cd tripal_multi_chado

Comments

Comment #1

9 November 2016 at 10:12

guignonv created an issue. See original summary.

Comment #2

PA robot CreditAttribution: PA robot commented 9 November 2016 at 11:23

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Comment #3

PA robot CreditAttribution: PA robot commented 18 December 2016 at 17:26

Status:

Needs review

» Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxguignonv2429515git

I'm a robot and this is an automated message from Project Applications Scraper.

Comment #4

guignonv CreditAttribution: guignonv as a volunteer commented 5 January 2017 at 15:28

Status:

Needs work

» Needs review

Errors reported by automated review tools have been fixed except for the following:

./api/tripal_mc.api.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function hook_tripal_mc_sync_tables_alter(array &$sync_tables) {
function hook_tripal_mc_user_connection_alter(&$connection, &$tripal_mc_user) {
function hook_tripal_mc_user_connections_alter(array &$connections, &$tripal_mc_user) {

Fake hook functions respect the hook definition documentation guidelines ( https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... ).

DrupalSecure has found some issues with your code (please check the Writing secure core handbook).

FILE: /root/repos/pareviewsh/pareview_temp/api/tripal_mc.api.inc
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
99 | WARNING | Possible SQL injection in db_query() via variable
| | $schema_name
188 | WARNING | Possible SQL injection in db_query() via variable
| | $sync_table
--------------------------------------------------------------------------

The 2 related queries are specific to PostgreSQL (which is a prerequisite of this module anyway since it works with Tripal).
The 2 indicated variables are checked against a regexp that guarantee they can only contain word characters and dots and anyway, these variables are not fed from user inputs.

I also consider reviewing other modules once I'm done with some other priorities.

Comment #5

harishh CreditAttribution: harishh commented 8 February 2017 at 06:53

Please check the following in your code and correct it.

tripal_mc.api.inc

severity: criticalreview: sql_curlyLine 85: table names should be enclosed in {curly_brackets} [sql_curly]
'SELECT schema_name FROM information_schema.schemata WHERE schema_name = :schema_name;',

severity: criticalreview: security_2Line 770: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_2]
drupal_set_message(str_replace("\n", "
\n", $output));

severity: criticalreview: security_dsmLine 788: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_dsm]
drupal_set_message($output);

Comment #6

guignonv CreditAttribution: guignonv as a volunteer commented 8 February 2017 at 08:43

Thanks for the review.

I didn't know that curly brackets would work on schema-qualified system tables (but I used them in all my other queries as you may have seen). It's fixed now.

I also added the "check_plain()" calls needed. Thanks again, I missed that.

Comment #7

guignonv CreditAttribution: guignonv as a volunteer commented 8 February 2017 at 09:02

By the way, I still got a doubt about using curly in that case. According to the doc
( https://api.drupal.org/api/drupal/includes%21database%21database.inc/gro... ):

Curly braces are used around "node" to provide table prefixing via DatabaseConnection::prefixTables()

and I don't think prefixing a system table makes any sens. Anyway, the system tables should not appear in the list of tables to prefix so it should be fine as DatabaseConnection::prefixTables() would not consider those tables anyway.

Comment #8

girishpanchal07 CreditAttribution: girishpanchal07 as a volunteer and commented 1 March 2017 at 06:51

Status:

Needs review

» Needs work

@guignonv

Still automated review is not clear https://pareview.sh/node/696

Please update those errors and warnings.

Manually review

Please remove blank function like hook_disable() and hook_enable() those are unnecessary.

Comment #9

guignonv CreditAttribution: guignonv as a volunteer commented 6 March 2017 at 14:02

Status:

Needs work

» Needs review

Thanks for the review.

I removed the empty implementations of hook_disable() and hook_enable(). Those implementations were here as place holders for future features of the module that need to perform actions when some extensions are enabled or disabled. Current code standard does not allow commenting those blocks (which I wanted to keep as a reminder for future implementations... now they are removed and I added an issue https://www.drupal.org/node/2858108 as a reminder).

About the automated review, a new messages appeared about spacing on a variable assignation has been fixed ('=' was aligned with the one of the variable assignation above).

And a warning message is still there and will remain:

FILE: /root/repos/pareviewsh/pareview_temp/tripal_mc.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
66 | WARNING | The use of function eval() is discouraged
----------------------------------------------------------------------

This part of code dynamically generates hook implementations for hook_chado_node_sync_form_submit() for Chado tables that are managed by this Tripal MC module. The code processed by eval is previously checked with regular expressions to avoid security issues. I added some comments in the source code to give more details about this approach.

About the automated tests: writing tests using simpletests for this kind of module is really complicated as it requires another module, Tripal, to be enabled but also have it install Chado in several PostgreSQL schema. Chado schema installation is quite long (and can't be processed by Drupal batch process API) and it requires to be run by Drush from the command line. It is feasible but requires too much time investment for a module that is not released as a "real" module yet.