fix: disallow AlterColumn method chaining

[soanvig]

Fixes: #467

It was slightly tricky, but I decided that "unwinding" looks the best in terms of clarity of what is happening.

This effectively produces query:

ALTER TABLE "x" ALTER COLUMN "y" SET DEFAULT true, ALTER COLUMN "z" DROP NOT NULL, [...]

as in Postgres docs. MySQL has exact syntax for chaining ALTER COLUMN, but supports only drop default and set default operations, so probably nobody will chain those two together, plus alter column usage for MySQL is not properly handled in whole Kysely anyway. I tested MySQL case manually.

---

EDIT: now it just disallows chaining

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
kysely	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 28, 2023 9:06am

[igalklebanov]

Hey 👋

Thank you! ❤️

I understand what you were going for with this and it does seem convenient, but it is not aligned with Kysely's WYSIWYG design principle. Not sure about these changes.

We should consider either:

1. each method returns a builder that cannot chain further and .alterColumn expects it as argument. So the original issue is blocked by more precise API.

2. make compiler just add a whitespace between each operation, and keep it a single alter column clause. This'll cause a runtime error obviously with PostgreSQL/MySQL - but consumer should know SQL. This might provide future-proofing in case some dialect supports this.

I'm leaning towards 1.

@koskimas WDYT?

[igalklebanov]

plus alter column usage for MySQL is not properly handled in whole Kysely anyway. I tested MySQL case manually.

Can you explain this? does it require an issue?

[soanvig]

@igalklebanov that's rather new to me that Kysely pushes WYSIWYG to such extent.
I can wait for other people decision on that topic, sure.

---

Can you explain this? does it require an issue?

No. I was just saying, that you can call AlterTableBuilder.dropNotNull even if it is not supported in MySQL (and you will get runtime error)

[igalklebanov]

No. I was just saying, that you can call AlterTableBuilder.dropNotNull even if it is not supported in MySQL (and you will get runtime error)

Oh, gotcha. The API is dialect agnostic by design.

[koskimas]

each method returns a builder that cannot chain further and .alterColumn expects it as argument. So the original issue is blocked by more precise API.

@igalklebanov Yes, I agree. This is the way to proceed.

[soanvig]

@koskimas @igalklebanov
So for fun I'll implement that part, and we'll see. I really like this library, and we heavily use it at work, so I'm glad to contribute something.

[soanvig]

[soanvig]

@igalklebanov

I updated the PR. Above you can see intelisense suggestion (type compilation result).
Besides restricting the result, I also narrowed API of AlterColumnNode, so it cannot contain more than one operation at once (since it won't be handled properly anyway): I removed cloneWith, because it was not needed, and creating the node now requires passing the property to be set.

[igalklebanov]

Looks good! 💪

Left a few minor suggestions..

[soanvig]

@igalklebanov thanks for the suggestions. I've implemented them all in one commit.

[igalklebanov]

LGTM! 🚀