Chado Properties Field

[spficklin]

Issues Addressed

This PR should be merged after PR #271 as it relies on code in that PR.

Issue #265 Issue #129 Issue #271

Description

This PR adds the properties field. And makes some other improvements that were necessary for the properties field to work. Specifically:

1. The TripalFieldItemBase class was missing the form elements for setting/changing the controlled vocabulary term of a field. This is needed because unlike other fields that have a hard-coded CVterm, the property field does not. It must be set when the field is created.
2. A new tripal/src/Controller/CVTermAutoCompleteController.php file was added that adds the autocomplete controller for looking up terms. It functions differently than in Tripal v3. In Tripal v3, the autocomplete would provide possible candidate term names and then the user would get a radio button to select the best term. This controller simplifies term selection by giving the IDsapce and accession (term ID) with the term so it's as simple as selecting the term--no radio button required.
3. The property field can be used for any <base table>prop table. Also, users should be able to add new properties via the web interface so they need to be able to specify the Chado table. In Tripal v3, the entity always pointed to a single base table, but in Tripal v4 the chado information is part of the field and the entity doesn't store information about the tables. So, this PR adds a new base table select box for the fields. Once set it can't be changed.
4. This PR adds the 3 classes for a new field for properties.
5. There was a bug in the ChadoIdSpace plugin. In order to save on slow queries it was written to cache the vocabularies that belong to an IdSpace. When the Drupal cache is cleared this cache also got cleared and it never got repopulated. So code is added to do a lookup in the database if the set of vocabularies for an IDspace is not cached.

Note, that the property field does not get added to content types automatically during the Chado prepare step because they only get added one of two ways:

1. When content is published (which is not yet working)
2. Manually by the user via the web interface.

Considerations

In the process of writing this field I came to the conclusion that we should probably find a way to specify fields that should be created during the prepare stage using a YAML configuration file. Right now we have to write a function for every complex field we create that goes in the Chado Prepare step. It seems inflexible.

Second, the Chado storage plugin can't yet deal with deleting data in fields. This doesn't matter for fields that map to columns in base tables, but for properties you should be able to delete a row out of a prop table by removing it from the list. I started to implement this but realized it needs some better design and thought we s hould talk on it. So, as you test this PR keep in mind you can't delete properties after you add them. But adding a property field, adding values to it (editing) and displaying should all be working.

Third, it seems somewhere along the way the Chado Prepare task is no longer adding ontologies and improrting terms. We need to look into why not.

Testing

1. Start with a completely fresh installation of Drupal/Tripal.
2. Install chado
3. Prepare Chado
4. Create an organism record via the web interface
5. Create a gene feature via the web interface
6. Add a new property term
   • Go to Admin > Structure > Tripal Content Types. Under the Gene dropdown, selecte "manage fields".
   • Click the "+ Add field" button
   • Select "Chado Property" in the "Add a new field" and set whatever label you want. As an example use "Version" for the label.
   • Click "Save and continue"
   • Next, select the "feature" table in the 'Chado Base Table' dropdown.
   • If you expand the "Storage Settings" you should see empty values, this is normal at this point.
   • Changed "Allwoed number of values" to "unlimiited.
   • Click "Save field settigns"
   • Next it will tell you there is no vocabulary term for this field and you should set one. Scroll to the section titled "Controlled Vocabulary Term".
   • Enter "version" in the 'Set Term Name' box. It should give you a set of autocomplete suggestions. Selecte "version number (IAO:0000129)"
   • Leave all other fields as they are and click "Save settings"
   • You should now have a new field add to the gene content type!
   • To verify settings, click on the "Edit" button next to the new field.
   • Expand the "Controlled Vocabulary Term" field set. It should now describe the cvterm you selected ealier. It should also give you a box for changing the term if you want.
   • Scroll to the top of the page and click the "Field Settings " tab.
   • You should see that the base table dropdown is no longer editable but it should say "feature".
   • Open the "Storage settings" fieldset. It should list the base table name and give some default settings for the property. These won't make sense to the end-user but will make sense to a developer. We should probably format those nicer. It's just JSON format.
7. Add version properties to a gene.
   • Go to the gene entity you created earlier (Admin > Content > Tripal Content).
   • Click edit. You should now see a "Version" textarea at the bottom of the form. Put whatever text you want there.
   • Click 'Save"
   • You should now see at the bottom of the page the Version listed with the value you entered.
   • Edit the page again and add another "Version" property value. After saving, you should now see two elements listed on the page.
8. Go to Chado and look in the featureprop table. You should see the two records you just entered.

Note: because we don't support delete yet if you remove the field and re-add it with the same term, you must also clear out the orphaned records in the featureprop table or you'll get an SQL error when you try to insert the term again (unique constraint violoation).

[risharde]

Summary comment: Overall working based on the instructions, great job! One possible quirk found - to be confirmed and / or resolved.

[GOOD] 1 through 5, works great!

[GOOD with possible quirk] 6 - One hopefully minor possible bug is every time the save field button is clicked, on page reload of the edit field, I notice an additional default box is added. If this is normal, please ignore.

[GOOD with possible quirk] 7 works as intended based on instructions. There was no way to remove a version value so I just cleared it. When I saved it, the following extra boxes appeared - it doesn't break functionality but maybe can be resolved to show only the necessary boxes required to enter data.

[GOOD with possible quirk] #8 works as intended - values have been added to featureprop. The above issue in 7 causes empty featureprops to be added.

@spficklin

[spficklin]

Ah yes. It shouldn't keep adding empty boxes. That's annoying.

[laceysanderson]

I did a quick manual test to ensure that the functionality is still working after the merge conflict fix.

I confirmed the instructions for testing above worked as expected ✅ and I also experienced the quirk that @risharde mentioned in this review. Since it is inserting empty records, I do consider it a bug.

Additionally, I tried to create a new gene with properties and got the dreaded "The website encountered an unexpected error. Please try again later." 😬 The following errors showed up in the recent log messages related to this event:

PDOException: SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table "featureprop" violates foreign key constraint "featureprop_feature_id_fkey" DETAIL: Key (feature_id)=(0) is not present in table "feature". in Drupal\tripal\Entity\TripalEntity->preSave() (line 518 of /var/www/drupal9/web/modules/contrib/tripal/tripal/src/Entity/TripalEntity.php).

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23503]: Foreign key violation: 7 ERROR: insert or update on table "featureprop" violates foreign key constraint "featureprop_feature_id_fkey" DETAIL: Key (feature_id)=(0) is not present in table "feature". in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 811 of /var/www/drupal9/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

@risharde and @spficklin: did either of you test creating a new gene after adding the property field? I'm just wondering if this was caused when resolving the merge conflict or whether it was an existing issue.

[spficklin]

@laceysanderson

did either of you test creating a new gene after adding the property field? I'm just wondering if this was caused when resolving the merge conflict or whether it was an existing issue.

No, I didn't test that, but I suspect it's probably not a side-effect of the merge conflict. I'll try to take a look at it this weekend.

[spficklin]

Okay. I just pushed a new commit that should the following problem.ss

1. Adding a new gene with property field now works
2. Empty values are no longer repeatedly added to property table.

The other thing this commit does is provide better messaging to the end-user if a failure occurs. There is no longer a WSOD. But, this is problematic and I can roll it back if you think we need to. Here's the problem.

The documentation for the SqlContentEntityStorage->preSave() function, which is where we do all our TripalStorage stuff says that we should throw an Exception if the entity should not be saved. But unfortunately, Drupal doesn't handle that gracefully and gives a WSOD. However, by adding a nice message to the user in our TripalEntity->preSave() function to prevent a WSOD Drupal will continue to save the field values and we get an out-of-sync situation that will cause problems for the entity.

So, what's better a nice error message that basically renders the entity un-editable or a WSOD for the user? We may be able to set a state in the preSave() on the entity indicating it should not be saved and then handle it gracefully elsehwere (needs some digging). But that's the problem.

This PR is again ready for testing.

[laceysanderson]

The other thing this commit does is provide better messaging to the end-user if a failure occurs. There is no longer a WSOD. But, this is problematic and I can roll it back if you think we need to. Here's the problem.

The documentation for the SqlContentEntityStorage->preSave() function, which is where we do all our TripalStorage stuff says that we should throw an Exception if the entity should not be saved. But unfortunately, Drupal doesn't handle that gracefully and gives a WSOD. However, by adding a nice message to the user in our TripalEntity->preSave() function to prevent a WSOD Drupal will continue to save the field values and we get an out-of-sync situation that will cause problems for the entity.

So, what's better a nice error message that basically renders the entity un-editable or a WSOD for the user? We may be able to set a state in the preSave() on the entity indicating it should not be saved and then handle it gracefully elsehwere (needs some digging). But that's the problem.

Ouch 🤦‍♀️ Really Drupal!? It sounds like we need to bring the WSOD back, provide some really good logging for the administrator in the preSave() in case this happens and do much more thorough validation during the form validation stages to do everything in our power to ensure this isn't triggered 😬

I'm working in the Tripal DBX right now so haven't gotten the chance to do digging to see how feasible this proposition is though 😬

[spficklin]

Okay. I'll put it back to a WSOD. I did fix the logging so that now the SQL error does show up in the logs to help with debugging.

[spficklin]

I'm wondering if we could fix this, by setting a state in the entity during the preSave, then override the save and see if an error has been set, and then preempt the parent class save with a nice message instead. That should do the trick. I'll see if I can do that.

[spficklin]

So, I managed to fix the problem of the WSOD with a new push to this PR. It adds a validateValues() function to the TripalStorage Interface and implements the validate() function in the TripalEntity. The validate() function gets called before the entity tries to save itself. If anything fails validation check then it refuses to save. So, this means that in the validateValues() function of the storage class we need to check every possible thing that could go wrong with an insert/update of values. I have added the following checks

1. For base table records
   • Check that the unique constraint is met
   • Checks that the type of value matches the column type
   • Checks that the size of the value is not larger than what is expected
   • Checks that the foreign key value actually exists.
2. For non-base table records
   • No checks because the ChadoStorage class removes any linked data (e.g. properties) prior to any insert/update to make sure that only those values provided are present.

These validation checks mean that even if the programmer botches up the fields, these checks should hopefully stop all of the problems with inserts/updates that would create a WSOD.

This PR is still heavily failing the functional tests. I don't have time to dig into them and my week is crazy and I can't get to it again until next weekend. If anyone has time to try and fix them to prevent this PR from just sitting here that would be great. In the meantime, if I can find more time I need to work on issue #291 because I can't add functional tests to this PR until that gets fixed.

[laceysanderson]

Thanks for the update @spficklin! I'll look into the automated testing and look at your new fixes ASAP.

[laceysanderson]

This latest commit should fix the automated testing.

The tests were failing because the query builder was not realizing these were chado tables. It seems the Tripal DBX fix works if you are using ->query() but not yet if you are using the query builder (e.g. ->insert()). To get around this I prefixed the tables with 1: so that Tripal DBX will use Chado and will open a separate issue to work a bit more with Tripal DBX detecting chado through the query builder. Note: Tripal DBX works great using the query builder if you prefix the table names.

[spficklin]

Oh, right... Duh. I don't know why I didn't think to put the '1:' in those queries. Thanks for the fix.

[laceysanderson]

I just did some more manual testing:

• ✅ Can add new gene with properties
• ✅ Can add/remove/update properties from an existing gene
• ✅ Do not get additional empty records added to the table

However, when I tried to test the new validation by entering a word into the type_id field I was greeted by a WSOD still and not the error message I expected. I'll dig into it further including starting a fresh build/site to ensure there's not residual caching/what-not throwing it out of wack.

One step closer!

[laceysanderson]

Steps to Reproduce

1. Pull this branch
2. Build a new Image: docker build --tag=tripalproject/tripaldocker:drupal9.4.x-dev --build-arg drupalversion='9.4.x-dev' ./
3. Create a new container: docker run --publish=80:80 --name=t4d8fields -tid --volume=pwd:/var/www/drupal9/web/modules/contrib/tripal tripalproject/tripaldocker:drupal9.4.x-dev
4. Start the database: docker exec t4d8field service postgresql restart
5. Navigate to localhost + login
6. Create an organism
7. Create a gene but enter "gred" instead of a cvterm_id for "AdditionalType"
8. Click "Save" and experience the WSOD.
9. Go to the log messages (Admin > Manage > Reports > Recent Log messages) and see the following:

Errors from the log above:

Warning: Undefined array key "type_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateUnique() (line 971 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

0 /var/www/drupal9/web/core/includes/bootstrap.inc(347): _drupal_error_handler_real(2, 'Undefined array...', '/var/www/drupal...', 971)

1 /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php(971): _drupal_error_handler(2, 'Undefined array...', '/var/www/drupal...', 971)

2 /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php(1216): Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateUnique(Array, 'organism', 0, Array, Array)

3 /var/www/drupal9/web/modules/contrib/tripal/tripal/src/Entity/TripalEntity.php(679): Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateValues(Array)

4 /var/www/drupal9/web/core/lib/Drupal/Core/Entity/ContentEntityForm.php(188): Drupal\tripal\Entity\TripalEntity->validate()

5 [internal function]: Drupal\Core\Entity\ContentEntityForm->validateForm(Array, Object(Drupal\Core\Form\FormState))

6 /var/www/drupal9/web/core/lib/Drupal/Core/Form/FormValidator.php(82): call_user_func_array(Array, Array)

7 /var/www/drupal9/web/core/lib/Drupal/Core/Form/FormValidator.php(275): Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object(Drupal\Core\Form\FormState))

8 /var/www/drupal9/web/core/lib/Drupal/Core/Form/FormValidator.php(118): Drupal\Core\Form\FormValidator->doValidateForm(Array, Object(Drupal\Core\Form\FormState), 'tripal_entity_b...')

9 /var/www/drupal9/web/core/lib/Drupal/Core/Form/FormBuilder.php(588): Drupal\Core\Form\FormValidator->validateForm('tripal_entity_b...', Array, Object(Drupal\Core\Form\FormState))

10 /var/www/drupal9/web/core/lib/Drupal/Core/Form/FormBuilder.php(320): Drupal\Core\Form\FormBuilder->processForm('tripal_entity_b...', Array, Object(Drupal\Core\Form\FormState))

11 /var/www/drupal9/web/core/lib/Drupal/Core/Controller/FormController.php(73): Drupal\Core\Form\FormBuilder->buildForm(Object(Drupal\tripal\Form\TripalEntityForm), Object(Drupal\Core\Form\FormState))

12 [internal function]: Drupal\Core\Controller\FormController->getContentResult(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\RouteMatch))

13 /var/www/drupal9/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)

14 /var/www/drupal9/web/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber{closure}()

15 /var/www/drupal9/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))

16 /var/www/drupal9/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)

17 /var/www/drupal9/vendor/symfony/http-kernel/HttpKernel.php(169): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber{closure}()

18 /var/www/drupal9/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)

19 /var/www/drupal9/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

20 /var/www/drupal9/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

21 /var/www/drupal9/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

22 /var/www/drupal9/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)

23 /var/www/drupal9/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

24 /var/www/drupal9/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

25 /var/www/drupal9/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

26 /var/www/drupal9/web/core/lib/Drupal/Core/DrupalKernel.php(709): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)

27 /var/www/drupal9/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))

28 {main}

Warning: Undefined array key "organism_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateRequired() (line 907 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "type_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateRequired() (line 907 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "type_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateFKs() (line 1039 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "organism_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateTypes() (line 1090 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "type_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateTypes() (line 1090 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "organism_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateSize() (line 1164 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Warning: Undefined array key "type_id" in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateSize() (line 1164 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php)

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for type bigint: "gred" LINE 4: ... '1') AND ("uniquename" = 'fdsdfs') AND ("type_id" = 'gred') ^: SELECT "ct".* FROM chado.feature "ct" WHERE ("organism_id" = :db_condition_placeholder_0) AND ("uniquename" = :db_condition_placeholder_1) AND ("type_id" = :db_condition_placeholder_2); Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => fdsdfs [:db_condition_placeholder_2] => gred ) in Drupal\tripal_chado\Plugin\TripalStorage\ChadoStorage->validateUnique() (line 984 of /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php).

(Backtrace only included for the first error. Very similar for all subsequent errors. Errors listed in the order they were produced.)

I'll dig some more tomorrow and see if I can get this fixed :-)

[spficklin]

Hmmm.... the new updates to the Chado Storage class for validating field should have caught that. I'll take a look.

[spficklin]

Oka @laceysanderson . The problem is fixed. The validation was working correctly, but the problem was that even though it noticed that the type ID (additional type field) wasn't an integer it still proceeded to evaulate the unique contraint (which the type_id is part of) which caused the problem. It should have short-circuted. I've fixed it. So I think its finally ready (unless you find something else).

[codeclimate[bot]]

Code Climate has analyzed commit 1972fa5f and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	10

The test coverage on the diff in this pull request is 40.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 34.3% (0.8% change).

View more on Code Climate.

[spficklin]

As per the discussion with @laceysanderson and as documented in issue #310 I'm going to merge this as it has passed all CI tests and has been fully tested by @laceysanderson and @risharde . The missing functional tests that were holding up merging of this PR will be added when the PR for issue #310 is submitted.