saketkc / pysradb

Package for fetching metadata and downloading data from SRA/ENA/GEO
https://saketkc.github.io/pysradb
BSD 3-Clause "New" or "Revised" License
307 stars 50 forks source link

Solves Issue #26 using uri filenames. #27

Closed DaasDaham closed 4 years ago

DaasDaham commented 4 years ago

Line 27 in basedb.py was causing this issue by creating a new file if it didn't exist and did not check if the file existed or not thus except block on line 152 in sradb.py was never activated. I changed the argument for sqlite3.connect() command using uri syntax which allows customizing the course of action if a file already exists and if doesn't. Changed it to throw an error if file is not found which is caught at line 182 in sradb.py. Use case

from pysradb import SRAdb
db = SRAdb('this_shouldnt_exist')
df = db.sra_metadata('SRP098789')
df.head() 

It throws an error and returns

(pysradb) saad@DaasDaham:~/Desktop/pysradb$ python sample.py
this_shouldnt_exist not a valid SRAmetadb.sqlite file.
Please download one using `pysradb metadb`.
saketkc commented 4 years ago

Thanks @DaasDaham! We want to get rid of the database approach eventually. Would you be able to write a couple of test cases to ensure that this behavior does not break anything with the current usage of SRAmetadb.sqlite?

There is a template here which was in use until recently: https://github.com/saketkc/pysradb/blob/master/tests/_test_sradb.py It has all the barebone structure you would need. You can move it to test_sradb.py and see if the tests pass. And then add additional tests for the proposed changes.

Let me know if you have any questions. And thanks once again for your contribution!

DaasDaham commented 4 years ago

Thanks for your reply! I have added new test cases and updated uri code in other PR #31 . However, there is one thing that I noticed, its that there is no write protection on SRAmetadb.sqlite file and I was able to edit the database using SRAdb('SRAmetadb.sqlite').query("sql query to change database"). I just wanted to confirm if this behaviour is intended because it is a local copy and the user can do whatever he wants with the database?

saketkc commented 4 years ago

However, there is one thing that I noticed, its that there is no write protection on SRAmetadb.sqlite file and I was able to edit the database using SRAdb('SRAmetadb.sqlite').query("sql query to change database"). I just wanted to confirm if this behaviour is intended because it is a local copy and the user can do whatever he wants with the database?

I am not sure I understand. Were you able to modify the SRAmetadb.sqlite file? That can always be done by the user given that it is like any other text file.

DaasDaham commented 4 years ago

However, there is one thing that I noticed, its that there is no write protection on SRAmetadb.sqlite file and I was able to edit the database using SRAdb('SRAmetadb.sqlite').query("sql query to change database"). I just wanted to confirm if this behaviour is intended because it is a local copy and the user can do whatever he wants with the database?

I am not sure I understand. Were you able to modify the SRAmetadb.sqlite file? That can always be done by the user given that it is like any other text file.

I mean I could change the database by using SQL commands. Basically sqlite3.connect() command has rwc (read-write-create) mode i.e it gives both read and write permission for the database to user. I wanted to confirm if we really want to give the user both read and write permission? These permissions can be changed by using uri syntax and setting mode to ro (read-only).