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

dolt sql and other commands should connect to running database #3922

Closed
zachmu opened this issue Jul 25, 2022 · 3 comments
Closed

dolt sql and other commands should connect to running database #3922

zachmu opened this issue Jul 25, 2022 · 3 comments
Assignees
Labels
cli enhancement New feature or request

Comments

@zachmu
Copy link
Member

zachmu commented Jul 25, 2022

It's not safe to issue CLI commands to update a database while a server is running. To prevent issues this might cause, we made any command that updates database state fail if a server is running. This makes the command line much less useful.

Instead of locking the database when a server is running, we should implement the following:

  1. Implement all dolt CLI commands as SQL commands
  2. Make dolt automatically connect to appropriate running server to issue its command
  3. If no server is running, the command behaves exactly as if the command started a server, issued the single command, and quit the server.
  4. The command line client is a single long-term session that persists between database invocations

This design necessarily also resolves odd differences in behavior between running a command and issuing its equivalent SQL statement.

We can make this change incrementally, but we shouldn't write new commands or make major changes to existing ones without considering this design.

@fulghum
Copy link
Contributor

fulghum commented Jul 26, 2022

Makes sense to me. I love unifying on the sql path and cleaning up the duplicated logic in the CLI commands.

The only downside I could brainstorm was that starting a server will slow down some of the CLI commands, but that seems negligible for a command line experience.

@timsehn timsehn added enhancement New feature or request cli labels Aug 24, 2022
@macneale4 macneale4 self-assigned this Apr 4, 2023
macneale4 added a commit that referenced this issue Apr 20, 2023
macneale4 added a commit that referenced this issue Apr 20, 2023
Add Server Runtime details to the sql-server.lock file

Step (n) of #3922
macneale4 added a commit that referenced this issue Apr 27, 2023
The complexity of creating global arguments is being broken into multiple pieces. This step includes the breaking up of the top level dolt command set to enable global argument parsing and initialization of the CliContext. At present, the sql subcommand is the only one getting this treatment, but it will be extended with additional commands in the future.

This change doesn't change much that is visible to the user, with the exception of the --data-dir flag being enabled before the sql subcommand.

#3922
macneale4 added a commit that referenced this issue May 3, 2023
Small step to untangling shell and query engine. #3922

Existing tests cover behavior. This is a fairly straight forward refactor.
macneale4 added a commit that referenced this issue May 8, 2023
SQL Command arguments which pertain to instantiating a local SqlEngine instance are now global arguments which come before the sql subcommand. This is a breaking change for users of the sql command who pass in the following arguments:

--data-dir
--user
--doltcfg-dir
--privilege-file
--branch-control-file
All of the test changes pertain to moving those arguments, and nothing else. This is by design to ensure we don't have broader impact to the interface.

With this abstraction in place, we can put a different Queryist implementation in place - specifically one which talks to a remotes server - which is the end goal.

See: #3922
macneale4 added a commit that referenced this issue May 22, 2023
Smart routing for sql command execution against running local servers.

#3922
macneale4 added a commit that referenced this issue Jun 27, 2023
Enable SQL migrated commands authentication in the following ways:

* New Global Flag --password
* DOLT_CLI_PASSWORD environment variable
* Ask for a password with a prompt.
* Automatic authentication to a server using the secret in the sql-server.lock file

One significant change in behavior is that previously if a user presented a non-sense username/password, we'd accept it as a super user identity. If a real user was specified, we would promote that user to a super user - regardless of if the password was correct.

Now, if a --user flag is presented, the user must present a password by flag, env var or prompt. If the user/pwd combination is not a known user, the command will fail. This applies to both local and remote mode.

Important to call out that this isn't about security. If you want to be a super user when using your local instance, you can just not provide a --user. This behavior is to enable consistent behavior of client applications where they need to test permissions.

Related: #3922
@timsehn
Copy link
Contributor

timsehn commented Sep 25, 2023

As you can see, this is heavily in progress.

@timsehn
Copy link
Contributor

timsehn commented Apr 15, 2024

Resoving this uber issue as we have sub issues for the commands that don't now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants