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

Feature: --database-name should take a URL #61

Open
xkortex opened this issue Nov 9, 2021 · 2 comments
Open

Feature: --database-name should take a URL #61

xkortex opened this issue Nov 9, 2021 · 2 comments

Comments

@xkortex
Copy link

xkortex commented Nov 9, 2021

I'm looking to tinker with using SAPP with multiple users, so I'd like to be able to run with some client/server style database (I'm thinking postgres or possibly python-dqlite. ). However the DB class does not handle URLs directly. It is typically in SQLAlchemy to use a URL to define all the connection parameters of a database. The DBType enum is somewhat superfluous, since the driver can be inferred from a proper URL. It would be pretty straightforward to refactor it to pass in a URL and just pass that directly to sqlalchemy.engine.url.make_url, and fall back if it's a file path.

If you wanted to keep the same interface, you could have a helper function something like

class DBType(Enum):
    XDB = "xdb"  # not yet implemented
    INFER = "infer" 
    SQLITE = "sqlite"
    MEMORY = "memory"


def _make_url(name_or_url: Optional[Union[str, sqlalchemy.engine.url.URL]] = None, 
              dbtype: Union[DBType, str] = DBType.INFER,
              default_db_file: str = 'sapp.db') -> sqlalchemy.engine.url.URL:
    if dbtype is DBType.MEMORY or name_or_url == ':memory:':
        return sqlalchemy.engine.url.URL('sqlite', database=":memory:")
    if dbtype is DBType.SQLITE:
        return sqlalchemy.engine.url.URL('sqlite', database=name_or_url or default_db_file)
    if dbtype is DBType.INFER:
        return sqlalchemy.engine.url.make_url(name_or_url)

    raise errors.AIException(f'unsupported database type: {dbtype}')

This would keep the existing CLI behavior exactly as is, while allowing folks to pass in different URLs. Obviously plugging into postgres would require a bit more tooling (seems there is some graphene integration which relies on some particular functions/stored procedures - it looks tractable) but I think this would be a start in the right direction. It's a useful feature in its own right - URLs are a pretty standard way to deal with database connections. Caveat: I'm not sure what XDB is. Also is there any reason why DBType is a sqlalchemy.Enum as opposed to enum.Enum?

I could PR this if you want. Just sketched this idea out and tests pass.

@0xedward
Copy link
Contributor

Hey @xkortex, so sorry about the late reply! :(

Thanks for the suggestion! I think if we wanted to implement this, we probably want to keep the current interface the same.

cc @dkgi and @arthaud, what are your thoughts on this?

@arthaud
Copy link
Contributor

arthaud commented Nov 18, 2021

I would accept that PR personally.

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

No branches or pull requests

3 participants