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 new RPC-style API to create connections #3293

Closed
Tracked by #3294
seancolsen opened this issue Nov 3, 2023 · 11 comments · Fixed by #3348
Closed
Tracked by #3294

Add new RPC-style API to create connections #3293

seancolsen opened this issue Nov 3, 2023 · 11 comments · Fixed by #3348
Assignees
Labels
ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Nov 3, 2023

This API is to allow the front end to submit the New Database Connection form, as described in these specs.

The endpoint should be /api/ui/v0/connections/

Note that this is under the ui namespace, not db.

Requests should be submitted via POST.

The POST request body, as described via TypeScript, is:

type SampleDataSchemaIdentifier = 'library_management' | 'movie_collection';

type ConnectionToInternalDatabase = {
  connection_type: 'internal_database';
};
type ConnectionToUserDatabase = {
  connection_type: 'user_database';
  /** The Django ID from the connection object */
  id: number;
};
type Connection = ConnectionToInternalDatabase | ConnectionToUserDatabase;

type CredentialsCopied = {
  strategy: 'copy_from_known_connection';
  connection: ConnectionReference;
};
type CredentialsFromScratch = {
  strategy: 'from_scratch';
  user: string;
  password: string;
  host: string;
  port: string;
};
type CredentialsFromScratch = {
  strategy: 'with_new_user';
  create_user_via: Connection;
  new_user: string;
  new_password: string;
  host: string;
  port: string;
};

type Credentials =
  | CredentialsCopied
  | CredentialsFromScratch
  | CredentialsWithNewUser;

type RequestBody = {
  credentials: Credentials;
  database_name: string;
  create_database: boolean;
  sample_data: SampleDataSchemaIdentifier[];
  nickname: string;
};

Examples:

  • Case A

    {
      "credentials": {
        "strategy": "copy_from_known_connection",
        "connection": {
          "connection_type": "internal_database"
        }
      },
      "database_name": "mathesar",
      "create_database": true,
      "sample_data": [],
      "nickname": "My new connection"
    }
  • Case B

    {
      "credentials": {
        "strategy": "from_scratch",
        "user": "banana",
        "password": "riYwZ1xQ81RW28aU75k",
        "host": "example.com",
        "port": "5432"
      },
      "database_name": "mathesar",
      "create_database": true,
      "sample_data": [ "library_management" ],
      "nickname": "My new connection"
    }
  • Case C

    {
      "credentials": {
        "strategy": "with_new_user",
        "create_user_via": {
          "connection_type": "user_database",
          "id": 81
        },
        "user": "banana",
        "password": "riYwZ1xQ81RW28aU75k",
        "host": "example.com",
        "port": "5432"
      },
      "database_name": "mathesar",
      "create_database": true,
      "sample_data": [ "library_management", "movie_collection" ],
      "nickname": "My new connection"
    }
@seancolsen seancolsen added restricted: maintainers Only maintainers can resolve this issue ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL labels Nov 3, 2023
@seancolsen seancolsen added this to the v0.1.4 milestone Nov 3, 2023
@kanugurajesh

This comment was marked as off-topic.

@seancolsen

This comment was marked as off-topic.

@kanugurajesh

This comment was marked as off-topic.

@seancolsen

This comment was marked as off-topic.

@kanugurajesh

This comment was marked as off-topic.

@mathemancer mathemancer self-assigned this Nov 8, 2023
@mathemancer
Copy link
Contributor

mathemancer commented Nov 27, 2023

@seancolsen As I get into implementation, I have a couple of questions about desired behavior:

  • What should happen if the user wants to add sample data to a preexisting DB that already has schemata of the prescribed names? E.g., a previous user could have already added sample data, and even worse could have worked with / modified the relevant schemata. I assume we should either skip it, or throw an error in that case.
  • What should happen if a user wants to install on the internal DB itself? I think we should throw an error in that case.

Another note. There are 3 cases when the user chooses "create DB if it doesn't exist"

  1. The DB exists, Mathesar is already installed.
    In this case, we just add a connection to the DB. No confusion ensues
  2. The DB does not exist.
    In this case, we create the DB, install Mathesar, and create the connection. No confusion here either
  3. The DB exists, but Mathesar isn't installed
    Here, we're not just creating a connection to the preexisting DB. We would create the connection, and also install Mathesar on the preexisting DB (using, as I think we discussed, the preexisting credentials). This could certainly confuse a user.

These three cases seem like almost too much to compress into a single check box choice (and thus into a single "create_database" boolean in the API). Food for thought. For now, I'll proceed, but I'll be leaving this bit easily modifiable just in case.

@seancolsen
Copy link
Contributor Author

@mathemancer that all makes sense and I'll defer to your opinions on those questions.

@mathemancer
Copy link
Contributor

mathemancer commented Dec 4, 2023

@seancolsen What would you think of splitting into 3 endpoints under api/ui/v0/connections/, one for each strategy?

  • We need at least one endpoint under connections in order to make things play nicely with DRF
  • Multiple dispatch is a pain while we're simulating RPC endpoints with Django Rest Framework
  • Multiple dispatch needs to be handled on the FE anyway, since it has to fill that field
    • Thus, we either handle multiple dispatch only on the front end, or in both the FE and BE (and keep the types aligned between the FE and BE moving forward)
  • Cleaner, when thinking of the endpoints as functions (subjective; I don't like the nested value against which we need to validate the whole submission)

I propose:

  • POST api/ui/v0/connections/create_from_known_connection
  • POST api/ui/v0/connections/create_from_scratch
  • POST api/ui/v0/connections/create_with_new_user

And then we'd remove those bits from the submitted JSON blobs.

NOTE: This implies a slight semantic change in the meaning of 'strategy'. I.e., now it's a connection creation strategy, rather than a "where are my credentials" strategy. For me, the former is clearer and more consistent. YMMV

@seancolsen
Copy link
Contributor Author

@mathemancer and I chatted about this on Matrix. We'll be moving forward with his "three endpoints" approach and the credentials.strategy property will be removed.

@mathemancer
Copy link
Contributor

mathemancer commented Dec 7, 2023

@seancolsen For Case C, I'm going to remove the "host" and "port" keys; they're (at best) redundant, since the user and DB must be created on the DB server accessed by the 'via' connection. In case they're not redundant, then creating that user and DB via the given connection would be impossible anyway.

It occurs to me that I should have caught this earlier in the process; perhaps I was tired by the time we were discussing case C; sorry.

@seancolsen
Copy link
Contributor Author

@mathemancer

For Case C, I'm going to remove the "host" and "port" keys

Right. That makes sense. I don't think this change will require any frontend adjustments. Go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants