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

SqliteDict read-only mode should not create table #127

Closed
hholst80 opened this issue Jan 3, 2021 · 6 comments · Fixed by #128
Closed

SqliteDict read-only mode should not create table #127

hholst80 opened this issue Jan 3, 2021 · 6 comments · Fixed by #128

Comments

@hholst80
Copy link
Contributor

hholst80 commented Jan 3, 2021

The current behavior of SqliteDict is to always create the table it should operate on. Would a PR be accepted to change this behavior, for mode='r', to not to create a table?

The behavior in sqlitedict right now is as follows:

$ python3
>>> from sqlitedict import SqliteDict
>>> db = SqliteDict('sqlite3.db', 'nonexistant-table', mode='r')
>>> <ctrl-d>
$ sqlite3 sqlite3.db
sqlite> .tables
nonexistant-table
sqlite> .quit
$
@piskvorky
Copy link
Owner

piskvorky commented Jan 3, 2021

Hm, interesting question. I have no preference either way – how did this come up for you? What's your motivation, use-case where this makes a difference?

CC @mpenkov thoughts?

@mpenkov
Copy link
Collaborator

mpenkov commented Jan 4, 2021

Yeah, I think it wouldn't be a bad idea, because it's quite unexpected to modify something by opening it for reading.

I think the expected behavior in the above case would be something like a ValueError being raised if the table does not exist.

@hholst80
Copy link
Contributor Author

hholst80 commented Jan 4, 2021

I would like to keep things fairly consistent with how things work right now. Raising an exception would be a new workflow.

I think it would be reasonable for .commit to raise an exception on a read-only db but it should be OK to open a SqliteDict without problem even if the table does not exist.

>>> d=SqliteDict('foo.db', flag='r')
>>> d['foo']='bar'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.9/site-packages/sqlitedict.py", line 249, in __setitem__
    raise RuntimeError('Refusing to write to read-only SqliteDict')
RuntimeError: Refusing to write to read-only SqliteDict
>>> d.commit()
>>> d
SqliteDict(foo.db)
>>> dict(d)
{}
>>> 

@hholst80
Copy link
Contributor Author

hholst80 commented Jan 4, 2021

Hm, interesting question. I have no preference either way – how did this come up for you? What's your motivation, use-case where this makes a difference?

CC @mpenkov thoughts?

The use case is somewhat bespoke but IMO not unreasonable. I am using SqliteDict to store user account data and I am bolting on an authentication workflow onto this now. The table name is basically the username and the dict is all the user account data for that user. In the authentication workflow the auth service checks the passwd field in the user account details (stored as a SqliteDict). However, if the username does not exist I do not want the db to change. So, I either have to store valid usernames in a separate db or allow rouge usernames (that is, sqlite tables) to be created by the auth service.

  1. Internet botnet tries to login as "admin", "root" etc.
  2. reverse proxy calls out to auth service
  3. auth service probes SqliteDict('accounts.db', username, flag='r') for the passwd field (pw hash)
  4. If they match, allow request, otherwise deny.

The problem right now is that in step 3 the auth service will populate the db with random username that people are trying to login as. Valid or invalid.

@piskvorky
Copy link
Owner

piskvorky commented Jan 4, 2021

Thanks, that makes sense. How about a separate table (not DB) with valid usernames? …As long as you can keep it in sync with the corresponding user tables (atomic operations).

.commit to raise an exception on a read-only db but it should be OK to open a SqliteDict without problem even if the table does not exist.

Isn't that what happens now? I don't follow.

@hholst80
Copy link
Contributor Author

hholst80 commented Jan 4, 2021

Isn't that what happens now? I don't follow.

Per my example it does not look like .commit raises an exception on a read-only db. It does however raise an exception on assignment. I am not sure that I agree that the semantics are right. IMO it should raise an exception on commit and not on assignment. If autocommit is true it should raise an exception on assignment, as that should implicitly commit.

This is however unrelated to the issue here with the implicit table create.

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 a pull request may close this issue.

3 participants