search

vanderbilt-redcap / auto-dags-module

REDCap External Module that automatically creates, renames, and assigns records to DAGs based on a specified field.
MIT License
5 stars 7 forks source link

Call to AbstractExternalModule::getChoiceLabel() results in read of ALL record data #6

Closed lsgs closed 3 years ago

lsgs commented 3 years ago

It's cropped up in a couple of large projects where we use this module that the server runs out of memory when the Auto-DAG process gets triggered. I've traced it to this call in AutoDAGsExternalModule::setDAGFromField() (line 31):

$fieldLabel = $this->getChoiceLabel($dagFieldName, $fieldValue);

That function expects the first argument to be an array containing a record_id element in order to restrict the results of a call to REDCap::getData() to the current record. Because this is not passed by the module (v1.1.3) it REDCap::getData() ends up trying to read in all data for the entire project, and if the project is sufficiently large in terms of the number of records, fields and/or complexity (e.g. with events, repeating instruments) it will fail.

I have deployed a patched version of the module in our instance where the call to AbstractExternalModule::getChoiceLabel() is replaced by a call to the global function parseEnum() which appears to perform the task perfectly well:

// $fieldLabel = $this->getChoiceLabel($dagFieldName, $fieldValue);
$dagFieldChoices = \parseEnum($Proj->metadata[$dagFieldName]['element_enum']);
$fieldLabel = $dagFieldChoices[$fieldValue];

(It is not clear to me why the data read in AbstractExternalModule::getChoiceLabel() is necessary. Should an issue be raised for that too?)

mmcev106 commented 3 years ago

@lsgs, since you have a handle on this already, are able to create a quick pull request to resolve it as you see fit? Feel free to remove that data read if it seems unnecessary, and I will review it to make sure it doesn't have any unintended side affects.

lsgs commented 3 years ago

No worries @mmcev106. Done: https://github.com/vanderbilt-redcap/auto-dags-module/pull/7

mmcev106 commented 3 years ago

Hmm, I was assuming that would make sense as a PR to the module framework to improve this for all modules. I don't have time to think about this more right now, but hope to in the next few days.

mmcev106 commented 3 years ago

@lsgs, are you able to open a PR to the module framework instead, improving getChoiceLabel() for all modules?

lsgs commented 3 years ago

Sorry for the delay @mmcev106. Here you go: https://github.com/vanderbilt/redcap-external-modules/pull/414

mmcev106 commented 3 years ago

This should be resolved by this PR: https://github.com/vanderbilt/redcap-external-modules/pull/427