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

Standardize table schema format #75

Merged
merged 7 commits into from
Sep 27, 2023
Merged

Standardize table schema format #75

merged 7 commits into from
Sep 27, 2023

Conversation

asutula
Copy link
Contributor

@asutula asutula commented Sep 27, 2023

Uses the Table.Schema type from the SDK. The schema builder component now uses that type to model its data, and when the table is saved to the store, a stringified version of that JSON object is stored.

Closes STU-116.

@linear
Copy link

linear bot commented Sep 27, 2023

STU-116 Standardize Table schema storage format

Currently, when a user creates a Table in Studio, we store the resulting create statement in a text column of the tables table. I started working on importing existing Tableland tables into Studio, and it seems clear that we will get a JSON representation of the columns and table constraints back from the validator. We can store that JSON in the text column of the tables table. The JSON format seems better because we can more easily use it to populate the UI for editing a Table schema, as well as for calculating migrations when editing a Table's schema.

carsonfarmer started on a PR that would give us access to the AST generated from a create statement, so we could easily implement the same logic the Go validator uses to construct the JSON representation of a Table:

tablelandnetwork/go-sqlparser#108

Here's the Go code that uses the create statement plus AST to create JSON:

https://github.com/tablelandnetwork/go-tableland/blob/main/internal/gateway/impl/gateway_store.go#L79

which creates JSON that looks like:

    {
      "columns": [
        {
          "name": "id",
          "type": "int",
          "constraints": [
            "primary key"
          ]
        },
        {
          "name": "name",
          "type": "text",
          "constraints": [
            "not null"
          ]
        },
        {
          "name": "age",
          "type": "int",
          "constraints": [
            "not null"
          ]
        },
        {
          "name": "city",
          "type": "text"
        }
      ],
      "tableConstraints": [
        "unique(name)"
      ]
    }

@@ -44,7 +50,7 @@ export function initTables(
.orderBy(tables.name)
.all();
const mapped = res.map((r) => r.tables);
return mapped;
return mapped as unknown as schema.Table[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird, see my comment in packages/store/src/schema/index.ts for an explanation.

Comment on lines 154 to 159
export type Schema = TablelandTable["schema"];

export type Table = Omit<InferSelectModel<typeof tables>, "schema"> & {
schema: Schema;
};
export type NewTable = InferInsertModel<typeof tables>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an interesting/quirky behavior when reading data from a table that has a text column, but JSON strings are stored in that column: The validator marshals the data as JSON within the containing JSON response payload, not as a string property of the response payload. This is pretty convenient if you're simply querying the validator and using the returned JSON data because you don't have to do any further parsing of your JSON strings to convert them to objects. It's not so convenient in the context of using an ORM like Drizzle where types are inferred from your table schema definitions. Drizzle says "you defined this column as text, so when you query it, it will come back as text". So to get around that, I hacked this inferred drizzle type which represents the data returned from the tables table. Not ideal, but not sure what else to do short of adding support of an option to the validator to leave text as text no matter what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What really isn't nice is the asymmetry between the table getting written to with string but returning a Schema

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't ideal, but seems like a worthy compromise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually looking into Drizzle's support for custom column types. Have a feeling it will work.

Comment on lines +112 to +114

const stmt = generateCreateTableStatement(table.name, table.schema);
const res = await tbl.exec(stmt);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convert the Schema object to a create table statement and execute it.

values.name,
values.description,
cleanSchema(schema),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleanSchema just removes any column definitions that don't have a name (this is possible in the UI to create an effectively empty column. We could change the ux to error out on those columns, but this seems easy enough)

<SchemaBuilder />
<pre>{createTableStatementFromObject(createTable, name)}</pre>
<SchemaBuilder schema={schema} setSchema={setSchema} />
<pre>{generateCreateTableStatement(name, schema)}</pre>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display the equivalent create statement in real time.

Copy link
Contributor Author

@asutula asutula Sep 27, 2023

Choose a reason for hiding this comment

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

Nothing too crazy in here, just converting it to use the Schema type from the SDK as the backing state type. @carsonfarmer your readonly Table (and therefore readonly Schema type) forced me to get all functional in here. Lots of destructuring ahead! Actually feels way better then relying on shared references to objects.

Comment on lines 3 to 24
export type Constraint = "not null" | "primary key" | "unique";

export function hasConstraint(
column: schema.Schema["columns"][number],
constraint: Constraint,
) {
return column.constraints ? column.constraints.includes(constraint) : false;
}

export function setConstraint(
column: schema.Schema["columns"][number],
constraint: Constraint,
value: boolean,
) {
const { constraints, ...rest } = column;
if (value) {
return { ...rest, constraints: [...(constraints || []), constraint] };
} else {
const res = [...(constraints || [])].filter((c) => c !== constraint);
return { ...rest, constraints: res.length ? res : undefined };
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote this little lib to make it easy to check and set constraints on a Column. All functional readonly style as well.

Comment on lines 26 to 51
export function generateCreateTableStatement(
tableName: string,
schema: schema.Schema,
) {
if (!tableName.length) {
return "";
}
const cleaned = cleanSchema(schema);
const columnDefinitions = cleaned.columns
.map((column) => {
const definition = `${column.name} ${column.type}`;
const columnConstraints = !!column.constraints?.length
? " " + column.constraints.join(" ")
: "";
return `${definition}${columnConstraints.toLowerCase()}`;
})
.join(", ");

const tableConstraints = cleaned.table_constraints
? cleaned.table_constraints.join(",")
: "";

return `create table ${tableName}(${columnDefinitions}${
tableConstraints ? `, ${tableConstraints}` : ""
});`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly taken from the tests that @carsonfarmer wrote in his PR, cleaned up a little to return and empty string if a valid create statement can't be created.

Comment on lines -240 to -253
<TableCell>
<Input
className="w-20"
type="text"
name="default"
placeholder="null"
defaultValue={column.default}
onChange={(e) => {
setCreateTable((prev) => {
prev.columns[columnIndex].default = e.target.value;
return {
...prev,
};
});
Copy link
Contributor Author

@asutula asutula Sep 27, 2023

Choose a reason for hiding this comment

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

I removed the option to set a column default constraint. While seemingly simple, default constraints are actually quite complex and translating them between input values and constraint string is tricky because of quoting strings, but not other things, supporting any expression, etc. Plus, if/when we do roll out a more sophisticated data model like in @carsonfarmer's PR, it seems like default constraints may not be supported, at least at first because they are tricky, so this will make us a little more future proof.

@asutula asutula self-assigned this Sep 27, 2023
joewagner
joewagner previously approved these changes Sep 27, 2023
Copy link
Contributor

@joewagner joewagner left a comment

Choose a reason for hiding this comment

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

This looks like some excellent improvements. I'll signoff, but might be good to have @carsonfarmer look it over too.

Signed-off-by: Aaron Sutula <[email protected]>
@asutula
Copy link
Contributor Author

asutula commented Sep 27, 2023

Ok @joewagner, I think I have it working with a custom Drizzle column type, and it is super clean now. It depends on this SDK pr being merged/released tablelandnetwork/tableland-js#39.

I'm having trouble running the web app locally to test it out at runtime. Something about npm linking to my local sdk branch is causing an error ../../node_modules/@tableland/sdk/dist/esm/helpers/chains.js:1:0 Module not found: Can't resolve '@tableland/evm/network.js'. Not sure what's up with that, but would rather not dig into that and just wait til we release a hotfix sdk with those changes.

@joewagner
Copy link
Contributor

Ok @joewagner, I think I have it working with a custom Drizzle column type, and it is super clean now. It depends on this SDK pr being merged/released tablelandnetwork/tableland-js#39.

Awesome!

I'm having trouble running the web app locally to test it out at runtime. Something about npm linking to my local sdk branch is causing an error ../../node_modules/@tableland/sdk/dist/esm/helpers/chains.js:1:0 Module not found: Can't resolve '@tableland/evm/network.js'. Not sure what's up with that, but would rather not dig into that and just wait til we release a hotfix sdk with those changes.

Works for me. Without spending much (any) time on it, you might have to also link @tableland/evm?

Comment on lines +7 to +17
const schemaSchema: z.ZodType<Schema> = z.object({
columns: z.array(
z.object({
name: z.string().nonempty(),
type: z.string().nonempty(),
constraints: z.array(z.string().nonempty()).optional(),
}),
),
table_constraints: z.array(z.string().nonempty()).optional(),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the best way I could find to create an api input definition that conforms to the Schema type. At least if the Schema type changes, this will cause a TS error.

const table = await store.tables.createTable(
input.projectId,
input.name,
input.description,
JSON.stringify(tablelandTable.schema),
tablelandTable.schema,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty cool, just send the Schema type into the db layer.

Comment on lines +4 to +18
export type Schema = SDKSchema;

export const schema = customType<{
data: Schema;
}>({
dataType() {
return "text";
},
fromDriver(value: unknown): Schema {
return value as Schema;
},
toDriver(value: Schema): string {
return JSON.stringify(value);
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our custom drizzle column type. I had to use unknown in the fromDriver implementation because we have an asymmetric mapping where we go from Schema -> string on the way into the db and Schema -> Schema on the way out of the db. This generic customType function for drizzle works better when it's a symmetric mapping like Schema <-> string.

@@ -80,7 +81,7 @@ export const tables = sqliteTable("tables", {
slug: text("slug").notNull(),
name: text("name").notNull(),
description: text("description").notNull(),
schema: text("schema").notNull(),
schema: schema("schema").notNull(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the custom type in the table definition. I verified this doesn't effect the generated migration (create table statement) because it still just creates a text column to back this (according to our custom type definition).

Signed-off-by: Aaron Sutula <[email protected]>
@asutula
Copy link
Contributor Author

asutula commented Sep 27, 2023

Ok @joewagner and @carsonfarmer this is ready for review again.

Copy link
Contributor

@joewagner joewagner left a comment

Choose a reason for hiding this comment

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

This is great, definitely looks better with the update to the sdk

@asutula asutula merged commit 7c3c4ce into main Sep 27, 2023
1 check passed
@asutula asutula deleted the table-schema branch September 27, 2023 23:01
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.

2 participants