explorerhq / sql-explorer

SQL reporting that Just Works. Fast, simple, and confusion-free. Write and share queries in a delightful SQL editor, with AI assistance.
https://www.sqlexplorer.io
Other
2.76k stars 366 forks source link

Use of Django connection name rather than alias as lookup #456

Open sdether opened 3 years ago

sdether commented 3 years ago

While looking through the connection logic for my attempt to address #455, I noticed that the name used to get a connection is the django connection name rather than the alias. The implications of this are:

It would be better if the stored connection was the explorer's name rather than the django name, so that remapping is possible and EXPLORER_CONNECTIONS can be used to gate access to the defined database connections.

If this change in behavior is acceptable, I could prepare a PR to implement it, including a migration that re-writes the stored Query.connection values.

marksweb commented 3 years ago

@sdether Thanks for raising. Could you link to the relevant parts of the code please? Makes it a little easier for anyone reading the issue to see what you're referring to. Especially for me, because although I maintain the package, I didn't write it.

I think this may tie in with #453 which talked about issues when using different connections.

What you're saying certainly makes sense. So happy for changes to happen.

sdether commented 3 years ago

Yeah, I believe that #453 is affected by this. There isn't a clear place to point at in code other than pointing out that the conncections dictionary is just a proxy to the django connections dictionary, i.e. implicitly requires django connection names, not the defined aliases:

https://github.com/groveco/django-sql-explorer/blob/499f18d0efee8b0d7e739b764a52b9ba758b6d89/explorer/connections.py#L21

I'll create a PR which will more clearly illustrate and fix this.

marksweb commented 3 years ago

Thanks @sdether I had a look around the use of connections as well & this looked like it'd come under this PR as it's taking that alias into account if the Query has that connection field utilised.

https://github.com/groveco/django-sql-explorer/blob/499f18d0efee8b0d7e739b764a52b9ba758b6d89/explorer/utils.py#L161