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

Add run configuration for sqlite #2718

Merged

Conversation

aperfilyev
Copy link
Collaborator

Would you be interested in something like this?

Screen.Recording.2021-12-03.at.23.04.40.mov

@JakeWharton
Copy link
Member

@AlecKazakova
Copy link
Collaborator

What the fuck yes that’s amazing

@AlecKazakova
Copy link
Collaborator

AlecKazakova commented Dec 4, 2021

if you’ll allow some nerd sniping this would be amazing if you could tell it to connect to a live db, like something on disk or running android app

@aperfilyev
Copy link
Collaborator Author

Yes I wanted to add a dialog to pick a database file. As for Android, what's the right way to do it? I might peek into database inspector in AS.

@AlecKazakova
Copy link
Collaborator

AlecKazakova commented Dec 4, 2021 via email

@aperfilyev
Copy link
Collaborator Author

Untitled.mov

I've made integration with AS database inspector but it requires IJ 2021.3. Is there a way to depend both on Android Studio and Intellij?

@AlecKazakova
Copy link
Collaborator

WOW that is amazing. Is the problem that you need to compile against android SDKs for this to work?

@aperfilyev
Copy link
Collaborator Author

Yes, I have to specify either intellij.localPath=/path/to/Android Studio.app or intellij.version=2021.3 for it to compile

@AlecKazakova
Copy link
Collaborator

if you dont specify those which classes cant be found?

The thing I'm thinking is specifying a compileOnly dependency on the maven artifacts for 2021.3 APIs, and then wrapping the code in a if (ApplicationInfo.getInstance().getVersion() > 2021.3) { register everything }

The other alternatives are to ship separate plugins, which I think is what most folks do but sounds like a lot of work, or just wait until Android Studio stable is on 2021.3, it looks like the bumblebee beta is on 2021.1 so presumably the one after that will be 2021.3?

@aperfilyev
Copy link
Collaborator Author

if you dont specify those which classes cant be found?

it can't find new database inspector classes that are not present in ij-203.

The thing I'm thinking is specifying a compileOnly dependency on the maven artifacts for 2021.3 APIs, and then wrapping the code in a if (ApplicationInfo.getInstance().getVersion() > 2021.3) { register everything }

I'm not sure it will work because it throws a bunch of ClassNotFoundException if I specify android studio in runIde config. :/

it looks like the bumblebee beta is on 2021.1 so presumably the one after that will be 2021.3?

Chipmunk is on 2021.2, unfortunately

if you wanna play with it, you can check this commit aperfilyev@9a2e0ad

@AlecKazakova
Copy link
Collaborator

coolcool, i'll give that a shot

@AlecKazakova
Copy link
Collaborator

dolphin is 2021.3, I think we just wait for that to release and then merge this as is bumping our minimum intellij up, probably the easiest thing to do

@aperfilyev
Copy link
Collaborator Author

Yes, and I hope it doesn't take another year before it becomes stable. Should I send a pr?

@AlecKazakova
Copy link
Collaborator

id wait to update this until we're getting close to the next sqldelight release and depending on what stage dolphin is at we can merge it

@JakeWharton
Copy link
Member

It's supposed to be 4 month cycles so June-ish.

Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an initial skim through of the high level stuff. I think these abstractions are the right direction, and theres probably small stuff we could do to extend them to other dialects+connection types.

throw SQLException("The file path is empty")
}

return DriverManager.getConnection("jdbc:sqlite:$filePath")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should reuse this class for DatabaseFileViewProvider too, and steal its logic to make sure its loading from the correct plugin classloader #3080

}
}

internal class ConnectionManagerImpl(private val project: Project) : ConnectionManager {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is actually the only thing thats dialect-specific. I don't know if its worth promoting into the dialect api but at least for testing we could try bundling like the postgres jdbc artifact and prompting the user for the connection url. Doesnt have to be in this PR, just spitballing

private val dialogFactory: ArgumentsInputDialog.Factory = ArgumentsInputDialogFactoryImpl()
) : AnAction() {

override fun actionPerformed(e: AnActionEvent) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this would be visible along with the android studio action that we just merged in - the implementations are actually so similar it seems like this AnAction stuff might be able to just detect if android is around and use that to connect vs the custom jdbc stuff

var connectionType: ConnectionType by connectionType(project)
}

internal class SelectConnectionTypeDialog(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay and then this is the thing where it would be sweet if we could somehow find localhost ports that have a valid db running to connect to. If thats even possible we can probably just skim IntelliJ source code to find something that already does it, but again not for this PR

@AlecKazakova AlecKazakova changed the base branch from master to sql-psi-dev April 20, 2022 13:41
@AlecKazakova
Copy link
Collaborator

FYI I'm gonna play around on this branch and might push some stuff, but I'll do it as separate commits instead of force pushing so we don't step on each others toes

@olme04
Copy link

olme04 commented Apr 20, 2022

This looks really awesome!
Will it be possible to somehow later integrate it with IDEA database plugin (https://www.jetbrains.com/help/idea/relational-databases.html, unfortunately it's available only in Ultimate) in similar way, as with android studio database inspector?

@AlecKazakova
Copy link
Collaborator

ideally yes, but because those APIs are likely closed source it might be impossible.

@aperfilyev 's trick for the AS Database inspector might also work here though, creating a fake .sql file that just delegates to the gutter action ultimate would create for that file. Worth trying at some point but no guarantees it will work

@AlecKazakova
Copy link
Collaborator

Okay here's where its at

Screen.Recording.2022-04-20.at.10.18.13.PM.mov

I'm pretty happy with this as a starting point. Some follow ups possible after this:

  • Implement the connection manager for Postgres/MySQL
  • Ability to delete connection
  • Ability to select no connection so the gutter icon disappears
  • Show the SQL that you ran in the run tab
  • Make it possible to just run raw sql against a connection

Will open this up for review then, I'll do a self review before merging but @aperfilyev might want to give things a look over too to make sure my changes make sense

@AlecKazakova AlecKazakova marked this pull request as ready for review April 21, 2022 02:21
Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the best I could

}

private val _result = mutableListOf<SqlParameter>()
override val result: List<SqlParameter> = _result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit:

Suggested change
override val result: List<SqlParameter> = _result
override val result: List<SqlParameter> get() = _result

avoid redundant backing property

@aperfilyev
Copy link
Collaborator Author

aperfilyev commented Apr 21, 2022

The code looks good but when I execute :runIde: I get this weird exception java.lang.AbstractMethodError: Receiver class app.cash.sqldelight.dialects.sqlite_3_18.SqliteDialect does not define or inherit an implementation of the resolved method 'abstract app.cash.sqldelight.dialect.api.ConnectionManager connectionManager()' of interface app.cash.sqldelight.dialect.api.SqlDelightDialect.
Maybe it's just me

upd: nevermind, it seems related to the specified dialect version

@AlecKazakova AlecKazakova merged commit 80e2799 into cashapp:sql-psi-dev Apr 21, 2022
@aperfilyev aperfilyev deleted the aperfilyev/run-configuration branch April 21, 2022 11:17
github-actions bot pushed a commit that referenced this pull request Apr 22, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Apr 23, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Apr 23, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Apr 23, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Apr 24, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Apr 25, 2022
* Add run configuration for sqlite

* Add some tests

* Run spotless

* Fix up code so it works with master

* Remove need for an identifier

* Reuse ConnectionManager in compiler module

* Move the connection manager to the dialect API

* Remove tests and run spotless

* Simplify RunSqlAction

* Update sqldelight-idea-plugin/src/main/resources/META-INF/plugin.xml

* Address feedback

Co-authored-by: Alec Strong <astrong@squareup.com>
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
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.

None yet

4 participants