-
Notifications
You must be signed in to change notification settings - Fork 5
fix(nextly): create the table on every field-group create transport #596
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
fe6dda5
refactor(nextly): move field-group table provisioning beside its sche…
mobeenabdullah 0604a56
feat(nextly): give field groups a service that owns the table and the…
mobeenabdullah be3117e
fix(nextly): create the table on every field-group create transport
mobeenabdullah 27369a7
fix(nextly): refuse an owned field-group table in the service, and bo…
mobeenabdullah 2855850
fix(nextly): bind a field group's runtime schema only once its row is…
mobeenabdullah ab8b2d4
fix(nextly): bound a field-group slug by the longest identifier it ge…
mobeenabdullah 6b2a655
fix(nextly): refuse names the database would not store, in every tran…
mobeenabdullah 4896d8b
fix(nextly): check the identifiers the statements actually contain
mobeenabdullah File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| "nextly": patch | ||
| "create-nextly-app": patch | ||
| "@nextlyhq/admin": patch | ||
| "@nextlyhq/admin-css": patch | ||
| "@nextlyhq/blocks-engine": patch | ||
| "@nextlyhq/blocks-react": patch | ||
| "@nextlyhq/ui": patch | ||
| "@nextlyhq/adapter-drizzle": patch | ||
| "@nextlyhq/adapter-postgres": patch | ||
| "@nextlyhq/adapter-mysql": patch | ||
| "@nextlyhq/adapter-sqlite": patch | ||
| "@nextlyhq/storage-s3": patch | ||
| "@nextlyhq/storage-uploadthing": patch | ||
| "@nextlyhq/storage-vercel-blob": patch | ||
| "@nextlyhq/plugin-form-builder": patch | ||
| "@nextlyhq/plugin-page-builder": patch | ||
| "@nextlyhq/plugin-seo": patch | ||
| "@nextlyhq/plugin-sdk": patch | ||
| "@nextlyhq/eslint-config": patch | ||
| "@nextlyhq/prettier-config": patch | ||
| "@nextlyhq/telemetry": patch | ||
| "@nextlyhq/tsconfig": patch | ||
| --- | ||
|
|
||
| Creating a field group through `nextly.fieldGroups.create()` or the mounted `POST /api/field-groups` route now creates its table. Both previously answered success while writing only a registry row, leaving a field group whose storage did not exist and every read and write to it failing. | ||
|
|
||
| Those two routes now also refuse a create whose table another field group already owns, which only the admin path checked before. Because a slug is normalised on its way to a table name, two slugs that differ only by hyphens and underscores name one table; such a request used to reach the schema change and rebind the existing field group's storage to the new field list. The mounted route additionally rejects a slug over 50 characters, the bound the rest of the product already validates against, instead of accepting it and provisioning a table under a name the database truncates or refuses. |
155 changes: 155 additions & 0 deletions
155
packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| // The REST create route reaches the service that owns the table, rather than writing the row itself. | ||
| // | ||
| // This route used to call `registerComponent` directly. That wrote a registry row and no table, so | ||
| // it answered `201 Field group created.` for a field group whose `comp_<slug>` did not exist, and | ||
| // every later read and write to it failed against the database. | ||
| // | ||
| // The half this file proves is the DELEGATION. The other half — that the service actually | ||
| // provisions the table on a real engine — is proved against live databases in | ||
| // `domains/field-groups/__tests__/field-group-create-transports.integration.test.ts`, through the | ||
| // two transports the harness can reach without a session. This route is permission-gated and the | ||
| // harness cannot authenticate a request, so proving it there would mean faking a session and | ||
| // testing the fake. Split deliberately, and neither half stands alone: delegation without | ||
| // provisioning proves nothing, and provisioning without delegation would leave this route still | ||
| // writing its own row. | ||
|
|
||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| const createFieldGroup = vi.fn(); | ||
| const registerComponent = vi.fn(); | ||
|
|
||
| vi.mock("../../di", () => ({ | ||
| getService: vi.fn((name: string) => | ||
| name === "fieldGroupMetadataService" | ||
| ? { createFieldGroup } | ||
| : { registerComponent } | ||
| ), | ||
| })); | ||
|
|
||
| vi.mock("../../init", () => ({ | ||
| getCachedNextly: vi.fn(async () => undefined), | ||
| })); | ||
|
|
||
| vi.mock("../route-auth", () => ({ | ||
| requireRouteAnyPermission: vi.fn(async () => undefined), | ||
| })); | ||
|
|
||
| describe("the field-group create route", () => { | ||
| beforeEach(() => { | ||
| createFieldGroup.mockReset(); | ||
| registerComponent.mockReset(); | ||
| createFieldGroup.mockResolvedValue({ | ||
| record: { | ||
| id: "fg-1", | ||
| slug: "hero", | ||
| label: "Hero", | ||
| tableName: "comp_hero", | ||
| fields: [], | ||
| migrationStatus: "applied", | ||
| }, | ||
| migrationStatus: "applied", | ||
| }); | ||
| }); | ||
|
|
||
| it("creates through the service that owns the table, not the registry", async () => { | ||
| const { POST } = await import("../field-groups"); | ||
|
|
||
| await POST( | ||
| new Request("http://localhost/api/field-groups", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| slug: "hero", | ||
| label: "Hero", | ||
| fields: [{ name: "heading", type: "text" }], | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| expect(createFieldGroup).toHaveBeenCalledTimes(1); | ||
| // Stated as well as the positive, because the defect was not that the row went unwritten: it | ||
| // was that the row was written ALONE. A route that called both would look correct here. | ||
| expect(registerComponent).not.toHaveBeenCalled(); | ||
| expect(createFieldGroup.mock.calls[0]?.[0]?.tableName).toBe("comp_hero"); | ||
| }); | ||
|
|
||
| it("says so when the table could not be provisioned", async () => { | ||
| createFieldGroup.mockResolvedValue({ | ||
| record: { | ||
| id: "fg-2", | ||
| slug: "hero", | ||
| label: "Hero", | ||
| tableName: "comp_hero", | ||
| fields: [], | ||
| migrationStatus: "failed", | ||
| }, | ||
| migrationStatus: "failed", | ||
| }); | ||
| const { POST } = await import("../field-groups"); | ||
|
|
||
| const response = await POST( | ||
| new Request("http://localhost/api/field-groups", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| slug: "hero", | ||
| label: "Hero", | ||
| fields: [{ name: "heading", type: "text" }], | ||
| }), | ||
| }) | ||
| ); | ||
| const body = (await response.json()) as { message?: string }; | ||
|
|
||
| // An unqualified success for a create whose table was never made is what let this ship unnoticed | ||
| // for as long as it did. | ||
| expect(body.message).not.toBe("Field group created."); | ||
| expect(body.message).toContain("could not be provisioned"); | ||
| }); | ||
|
|
||
| it("refuses a slug too long to survive becoming a table name", async () => { | ||
| // 48 characters: one past the bound. The bound is not the slug's own length limit, it is | ||
| // whatever leaves the LONGEST generated identifier legal — `idx_comp_<slug>_parent`, sixteen | ||
| // characters longer than this. At 48 that index name is 64: rejected by MySQL, and past the 63 | ||
| // where PostgreSQL stops rejecting and starts silently truncating. | ||
| const { POST } = await import("../field-groups"); | ||
|
|
||
| const response = await POST( | ||
| new Request("http://localhost/api/field-groups", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| slug: `a${"b".repeat(47)}`, | ||
| label: "Too long", | ||
| fields: [{ name: "heading", type: "text" }], | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| // The point is WHERE it is refused. Accepted here, it reaches the DDL, provisions a table the | ||
| // verification then cannot find under the name it asked for, records the field group as failed, | ||
| // and still answers 201 for input the route declared valid. | ||
| expect(createFieldGroup).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("accepts a slug at the bound", async () => { | ||
| // The positive control. Without it, a rule that rejected everything would satisfy the case | ||
| // above while breaking every real create. | ||
| const { POST } = await import("../field-groups"); | ||
|
|
||
| const response = await POST( | ||
| new Request("http://localhost/api/field-groups", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| slug: `a${"b".repeat(46)}`, | ||
| label: "At the bound", | ||
| fields: [{ name: "heading", type: "text" }], | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| expect(response.status).toBe(201); | ||
| expect(createFieldGroup).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.