Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added testConnection feature in CSV adapter. Issue #450 #470

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Ahmed-Elgamel
Copy link

@Ahmed-Elgamel Ahmed-Elgamel commented Mar 3, 2024

fixes #450

Summary

I added a method called testConnection in the DataSource abstarct in order to test the connection of a specific Data Source.
The method could be abstract so that every class has to provide it implementation but for now it is not a priority.The CSV adaptor was not checking whether the uploaded file is a valid CSV file or not ,so I added this in the testConnection method.I tested and the CSV adaptor will not add invalid CSV files.This has to be done also for all the classes that extends the DataSource abstract class.I need somebody to guide how to do so in the other DataSources: Ethereum,GoogleSheet

Changes

  • testConnection method in all concrete classes that extends AbstractJdbcSource checks if the connection is valid by creating a connection handler and doing a trivial query "Select 1"

  • testConnection method in CsvSource class checks if the the csv source provided has the valid file extension

  • testConnection method in ExcelSource class checks if the the excel source provided has the valid file extension

  • testConnection method in Qfs class checks if the the root file source provided exists


ToDo

  • Add testConnection to CSV
  • Add testConnection to Excel
  • Add testConnection to GoogleSheet
  • Add testConnection to Qfs
  • Add testConnection to AbstractJdbcSource
  • Add testConnection to Ethereum

all the classes tha extend this DataSource class will provide their implementation
There was no validation whether the csv file is valid or not,I tried and I could add any file as a CSV adapter.The method was tested by calling the testConnection() in the CsvSource constructor and it indeed does not allow invlaid CSV file upload.
@hennlo
Copy link
Member

hennlo commented Mar 4, 2024

@Ahmed-Elgamel If this PR is not yet ready to be reviewed please mark it as WIP or Draft, thanks.

@Ahmed-Elgamel Ahmed-Elgamel marked this pull request as draft March 4, 2024 20:14
@hennlo
Copy link
Member

hennlo commented Mar 10, 2024

@Ahmed-Elgamel Regarding the remaining sources you should first focus on the AbstarctJdbc ones first.However I'm not sure if its sufficient to only implement it there alone.
Since every system is different and has different capabilities I would recommend executing a dummy SQL query to one of those systems that does not actuially retrieve data.

For Postgres this could be: select 1;

@Ahmed-Elgamel
Copy link
Author

Is the testConnection method supposed to return boolean or just throws an exception if the connection is invalid?

@Ahmed-Elgamel
Copy link
Author

Hi @hennlo , can you guide me on the other ethereum and googleSheet DataSources.Also will the testConnection() be called automatically in every DataSource constructor or we will add a button in the UI to call this method?

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.

Add 'Test Connection' on Data Sources
2 participants