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

API support for "Link Table" dialog #972

Closed
Tracked by #451
seancolsen opened this issue Jan 12, 2022 · 8 comments · Fixed by #1374
Closed
Tracked by #451

API support for "Link Table" dialog #972

seancolsen opened this issue Jan 12, 2022 · 8 comments · Fixed by #1374
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Jan 12, 2022

Problem

  • In the "Link Table" dialog, the front end needs to create a new foreign key reference between two tables.

    • For the one-to-one case, we need to add a column, add a foreign key constraint, and add a unique constraint.
    • For the many-to-one and one-to-many cases, we need to add a column and add a foreign key constraint.
    • For the many-to-many case, we need to add a mapping table, add columns to the table, and add foreign key constraints.
  • The front end can already perform all of those operations through the API. However, at the DB level, we'de like for those operations to occur within the same transaction, which necessitates some additional API features.

Solution

The implementer should write a proposed API spec and get feedback from other members of the engineering team before implementing this issue.

Ideas

This section written by @kgodey.

  • An /api/v0/links/ endpoint that lists all links and their types and allows creation and deletion of links. Creating and delete links should encapsulate all operations involved (creation and deletion of columns, constraints, and/or tables) in a single transaction.
  • Links can be one of three types:
    • one-to-one: this involves any single-column FK with a unique constraint applied.
    • many-to-one: this is any single-column FK
    • many-to-many: this is any table with two FKs
  • Links should be populated entirely by reflection and shouldn't need to be created within Mathesar

Details about how to define many-to-many and many-to-one tables need to be worked out as part of the spec. Please also consider the case of tables with FKs to themselves while doing this.

Additional context

@seancolsen seancolsen added status: draft type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL labels Jan 12, 2022
@seancolsen seancolsen added this to the [08] Working with Views milestone Jan 12, 2022
@kgodey kgodey self-assigned this Jan 12, 2022
@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

Assigned to myself to review.

@kgodey kgodey removed their assignment Jan 12, 2022
@kgodey kgodey added ready Ready for implementation and removed status: draft labels Jan 12, 2022
@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

Added some ideas about how to design the API.

@kgodey kgodey modified the milestones: [09] Working with Views, [08] Links Between Tables Jan 18, 2022
@silentninja silentninja self-assigned this Apr 27, 2022
@silentninja silentninja added status: started and removed ready Ready for implementation labels Apr 27, 2022
@silentninja
Copy link
Contributor

silentninja commented May 5, 2022

@seancolsen I have described a rough API schema below, please do suggest changes if any.

// /api/v0/links/
[
  {
    "link_type": "o2o", // One to One
    "data": {
      "referent_column_id": number,
      "column": { // Data to create new column
        name
        //      ... other column data
      },
      "column_id": number // Use existing column object
    }
  },
  {
    "link_type": "m2m", // Many to Many
    "data": {
      "referent_columns": [number],
      "map_table": {
                //      ... other table data
      },
  }
]

@seancolsen
Copy link
Contributor Author

seancolsen commented May 5, 2022

Hmmm. Now that I'm reading this ticket more closely, I'm a bit confused about some of the details in the "Ideas" section that @kgodey wrote in the ticket description. I apologize that I didn't catch this sooner and raise concern with it or propose an API spec.

As far as I can tell, we don't have a need to use a special API like the "links" API to list or delete links. And those operations can be somewhat complex if we choose to do them through this sort of API. (I can provide examples of such complexity if needed.) I was originally envisioning this API as serving only the purpose of creating links. I'd like to understand if there's a use-case that I'm not seeing though.

With the scope of this API limited to only the use-case that I can currently envision the front end requiring, here's an alternate proposal for this API...

Proposal B

  • Endpoint /api/v0/links/ only accepts POST requests.

  • The POST request schema is described via the TypeScript interface LinkTablePostRequestParams below:

    interface ForeignKeyCreationParams {
      /** If missing, the backend will auto-generate */
      new_column_name?: string;
    
      /** If missing, the backend will auto-generate */
      new_fk_constraint_name?: string;
    
      /** Defaults to `false` if missing */
      is_unique?: boolean;
    }
    
    interface LinkTablePostRequestParams {
      base_table: {
        id: number;
        /** Only used when creating a 'one-to-one' or 'many-to-one' link. */
        fk_to_target_table?: ForeignKeyCreationParams;
      };
    
      target_table: {
        id: number;
        /** Only used when creating a 'one-to-many' link. */
        fk_to_base_table?: ForeignKeyCreationParams;
      };
    
      /** Only used when creating a 'many-to-many' link. */
      mapping_table?: {
        /** If missing, the backend will auto-generate */
        name?: string;
        fk_to_base_table?: ForeignKeyCreationParams;
        fk_to_target_table?: ForeignKeyCreationParams;
      };
    }
  • Note that there is no need for a link_type param, because the API is operating at somewhat of a lower-level, describing the operations that should take place within the transaction. This gives the front end more flexibility, which I think might be useful for additional scenarios we may design in Design better UX in the Link Table dialog for self-referential links #1345. It also means that some requests might not quite make sense. For example, if a request has base_table.fk_to_target_table and target_table.fk_to_base_table, that's weird. We could choose to add validation to respond with errors in such cases if we like.

  • On success, the POST response is empty with HTTP 204 status code.

@kgodey
Copy link
Contributor

kgodey commented May 5, 2022

A few points, not all related to each other:

  • @seancolsen's proposal is basically RPC API, not a REST API, which isn't consistent with the rest of our API, see API standards. A REST API would model a Link resource and allow CRUD operations on it.
  • The intent of my proposal was to establish a "Link" user-facing concept that could be mirrored in the API. I think having this concept instead of breaking it down into its constituent FKs and tables will make us think about it as its own idea.
    • Links are a central concept in the product, they're the main reason databases are better than spreadsheets for many use cases. I think we may want to build more features on top of links in the future.
  • Listing links will likely be required for the query builder (to find columns to join to).
  • I can't see an immediate use case for deleting links.

That being said, we can always refactor the API if needed. I've got a bunch of feedback about @silentninja's API proposal too, but I'm not sure if it would be useful to give it now.

@silentninja
Copy link
Contributor

silentninja commented May 5, 2022

Links are not actually a resource as they don't have an identifier that could point to a definite object rather they are a design pattern, so I don't think we should be trying to establish a strict restful behaviour.

@seancolsen @kgodey I have listed my proposed API schema below, would like to get a feedback on it.

interface OneToOneRequestParams {

    /** base table */
    referent_table_id: number;

    /** reference table */
    reference_table_id: number;
    reference_column: {
      name: string;
    }

}

interface ManyToManyParams {

    mapping_table: string;
    reference_objects: [{
        reference_table_id: number;
        reference_column: string;
    }]

}

interface LinkTablePostRequestParams {
  link_type: 'one-to-one' | 'one-to-many' | 'many-to-one' | 'many-to-many'; // Need this for validation


  data: OneToOneRequestParams | ManyToManyParams
}


    /** Response  */
    
interface OneToOneResponseParams {

    /** base table */
    referent_table_id: number;

    /** reference table */
    reference_table_id: number;
    reference_column_id: number;

}

interface ManyToManyResponseParams {

    mapping_table_id: number;
    reference_objects: [{
        reference_table_id: number;
        reference_column_id: number;
    }]

}

interface LinkTablePostResponseParams {
  link_type: 'one-to-one' | 'one-to-many' | 'many-to-one' | 'many-to-many'; 
  data: OneToOneResponseParams | ManyToManyResponseParams
}

@kgodey
Copy link
Contributor

kgodey commented May 5, 2022

@silentninja I'm not sure how to read this API schema, I'll leave it to you and @seancolsen to figure out. I think you should do whatever is fastest and we can figure out the best pattern later, during the cooldown. You can add it to the weekly meeting agenda for issues to figure out like @seancolsen has been doing.

@seancolsen
Copy link
Contributor Author

@silentninja Your proposal seems pretty similar to mine, so I'm glad to see that we seem to be close to working this out.

I don't envision ever needing to utilize the response from this API, so I'll continue my critique by focusing only on the request.

Critique:

  • (A) Nesting the properties within the data object seems unnecessary to me, but I don't have a specific concerns with it either. I'll stick with it for the sake of moving this conversation forward, but if you don't have a good reason for the nesting, then I'd suggest we flatten it for the sake of simplicity.

  • (B) In the one-to-many scenario, the API does not seem to have a clear way for me to indicate the name of the new column that will get created on the referent table.

With the above critique in-mind, here is my counter-proposal which builds on yours, offering some minor adjustments to the schema and fixes to the TS syntax.

Proposal D

interface OneToOneOrManyToOne {
  link_type: 'one-to-one' | 'many-to-one';
  data: {
    reference_table_id: number;
    reference_column: { name: string };
    referent_table_id: number;
  }
}
interface OneToMany {
  link_type: 'one-to-many';
  data: {
    reference_table_id: number;
    referent_table_id: number;
    referent_column: { name: string }; // <- See note (1)
  };
}
interface ManyToMany {
  link_type: 'many-to-many';
  data: {
    mapping_table_name: string; // <- Name adjusted for clarity
    referents: {                // <- Name adjusted for simplicity
      table_id: number;         // <- Name adjusted for simplicity
      column_name: string;      // <- Name adjusted for simplicity/clarity
    }[]                         // <- See note (2)
  };
}

type LinkTablePostRequest = OneToOneOrManyToOne | OneToMany | ManyToMany;

Notes:

  • (1) To satisfy critique (B), I am proposing to use data.referent_column.name instead of data.reference_column.name (as in your proposal).

  • (2) Minor note on TS syntax: the type [{ foo: number }] represents a fixed-length tuple which must contain one and only one object, whereas { foo: number }[] represents an array of objects. I'm assuming you intended for reference_objects to be an array, so I've fixed the syntax for clarity.

Special validation to consider:

  • When link_type is 'many-to-many', we should validate that data.referents contains exactly 2 objects. It's okay if those objects have the same table_id, but they can't have the same table_id and the same column_name.

@silentninja silentninja mentioned this issue May 10, 2022
7 tasks
@kgodey kgodey removed this from the [08] Links between Tables milestone Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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