Raise error for disallowed columns

[squirrelo]

While working on the search object overhaul, I realized that we are not disallowing the specialized column names from being in the templates. This checks for the existence of the specific columns needed for study searches, and if they do exist in a given sample or prep template, raises a QiitaDBColumnError. These columns are parts of known tables, so they should never be passed in as a part of a template and become a column on the dynamic tables.

[josenavas]

👎👎👎

The reason why I strongly disagree with this line is because this columns are disallowed because that is the output of the search engine. If the columns on the search engine are in the metadata template, the search engine breaks because SQL has multiple columns with the same name and its ambiguous.

My recommended solution, which I presented offline to @squirrelo and he disagrees, is importing the list of output columns from the search engine and disallow them here. The main issue is that the list of columns exist in the search engine and here, forcing the developer to remember that if he is modifying the search engine, he has to modify also this list in case that the output columns change.

We already have had an issue on trusting the developer to do the right thing (the purge_filepaths function issue) and I do not agree on introducing another potential spot where this can occur. Thus, I think that minimizing code duplication and developer burden is always the right path to move forward, rather than using a comment on all caps as per suggestion of @squirrelo ...

[squirrelo]

The issue is that, while your idea works for a single object, it does not scale to multiple objects. If we suddenly need disallowed columns in the analysis, job, and ontology objects in the future, what then? There will still need to be manual editing of the function to reflect those new disallowed columns, completely negating the above.

[antgonza]

Can we discuss this during tomorrow's meeting?

[josenavas]

@antgonza 👍

[squirrelo]

@antgonza 👍

[squirrelo]

Note also, though, that Jose's idea violates the prime directive of qiita_db objects never importing from any other qiita_db object file. We've kept that going until now, so we should NOT break that in my opinion.

[squirrelo]

Actually, this is already violated on the metadata_template.py file, so we need to change that ASAP unless we are droppng that directive and creating circular import fun again.

[ElDeveloper]

Would you mind creating an issue about this?

On (Mar-17-15|19:52), Joshua Shorenstein wrote:

@@ -760,6 +760,13 @@ def _check_special_columns(cls, md_template, obj):
The obj to which the metadata template belongs to. Study in case
of SampleTemplate and RawData in case of PrepTemplate
"""

•    # Check disallowed col names
  

•    disallowed = {'study_id', 'processed_data_id'}
  

Actually, this is already violated on the metadata_template.py file, so we need to change that ASAP unless we are droppng that directive and creating circular import fun again.

---

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/992/files#r26634923

[coveralls]

Coverage increased (+0.01%) to 78.82% when pulling 210acfb on squirrelo:more-validation-of-columns into f54acc3 on biocore:master.