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

Improve errors for invalid IDs in content collections #12892

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-dragons-shout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'astro': patch
---

Added more verbose errors for content collections with invalid IDs.

Previously, when an entry in a content collection had an ID that wasn't a string, the error would omit the ID entirely. With this change, the error will now explicitly say that the ID has to be a string.
45 changes: 38 additions & 7 deletions packages/astro/src/content/content-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import {
getEntryConfigByExtMap,
getEntryDataAndImages,
globalContentConfigObserver,
loaderReturnSchema,
safeStringify,
} from './utils.js';
import type { z } from 'zod';

export interface ContentLayerOptions {
store: MutableDataStore;
Expand All @@ -30,6 +32,12 @@ export interface ContentLayerOptions {
watcher?: FSWatcher;
}

type CollectionLoader<TData> = () =>
| Array<TData>
| Promise<Array<TData>>
| Record<string, Record<string, unknown>>
| Promise<Record<string, Record<string, unknown>>>;

export class ContentLayer {
#logger: Logger;
#store: MutableDataStore;
Expand Down Expand Up @@ -266,7 +274,7 @@ export class ContentLayer {
});

if (typeof collection.loader === 'function') {
return simpleLoader(collection.loader, context);
return simpleLoader(collection.loader as CollectionLoader<{ id: string }>, context);
}

if (!collection.loader.load) {
Expand Down Expand Up @@ -325,15 +333,38 @@ export class ContentLayer {
}

export async function simpleLoader<TData extends { id: string }>(
handler: () =>
| Array<TData>
| Promise<Array<TData>>
| Record<string, Record<string, unknown>>
| Promise<Record<string, Record<string, unknown>>>,
handler: CollectionLoader<TData>,
context: LoaderContext,
) {
const data = await handler();
const unsafeData = await handler();
const parsedData = loaderReturnSchema.safeParse(unsafeData);

if (!parsedData.success) {
const issue = parsedData.error.issues[0] as z.ZodInvalidUnionIssue;

// Due to this being a union, zod will always throw an "Expected array, received object" error along with the other errors.
// This error is in the second position if the data is an array, and in the first position if the data is an object.
const parseIssue = Array.isArray(unsafeData)
? issue.unionErrors[0]
: issue.unionErrors[1];

const error = parseIssue.errors[0];
const firstPathItem = error.path[0];

const entry = Array.isArray(unsafeData)
? unsafeData[firstPathItem as number]
: unsafeData[firstPathItem as string];

throw new AstroError({
...AstroErrorData.ContentLoaderReturnsInvalidResult,
message: AstroErrorData.ContentLoaderReturnsInvalidResult.message(context.collection, entry, error.message),
});
}

const data = parsedData.data;

context.store.clear();

if (Array.isArray(data)) {
for (const raw of data) {
if (!raw.id) {
Expand Down
55 changes: 17 additions & 38 deletions packages/astro/src/content/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ import {
} from './consts.js';
import { glob } from './loaders/glob.js';
import { createImage } from './runtime-assets.js';

/**
* Amap from a collection + slug to the local file path.
* A map from a collection + slug to the local file path.
* This is used internally to resolve entry imports when using `getEntry()`.
* @see `templates/content/module.mjs`
*/
Expand All @@ -41,10 +42,20 @@ const entryTypeSchema = z
.string({
invalid_type_error: 'Content entry `id` must be a string',
// Default to empty string so we can validate properly in the loader
})
.catch(''),
})
.catchall(z.unknown());
}),
}).passthrough();

export const loaderReturnSchema = z.union([
z.array(entryTypeSchema),
z.record(
z.string(),
z.object({
id: z.string({
invalid_type_error: 'Content entry `id` must be a string'
}).optional()
}).passthrough()
),
]);

const collectionConfigParser = z.union([
z.object({
Expand All @@ -59,39 +70,7 @@ const collectionConfigParser = z.union([
type: z.literal(CONTENT_LAYER_TYPE),
schema: z.any().optional(),
loader: z.union([
z.function().returns(
z.union([
z.array(entryTypeSchema),
z.promise(z.array(entryTypeSchema)),
z.record(
z.string(),
z
.object({
id: z
.string({
invalid_type_error: 'Content entry `id` must be a string',
})
.optional(),
})
.catchall(z.unknown()),
),

z.promise(
z.record(
z.string(),
z
.object({
id: z
.string({
invalid_type_error: 'Content entry `id` must be a string',
})
.optional(),
})
.catchall(z.unknown()),
),
),
]),
),
z.function(),
z.object({
name: z.string(),
load: z.function(
Expand Down
28 changes: 28 additions & 0 deletions packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1563,6 +1563,34 @@ export const InvalidContentEntryDataError = {
hint: 'See https://docs.astro.build/en/guides/content-collections/ for more information on content schemas.',
} satisfies ErrorData;

/**
* @docs
* @message
* **Example error message:**<br/>
* The content loader for the collection **blog** returned an invalid result:<br/>
* Content entry `id` must be a string:<br/>
* {<br/>
* "id": 1,<br/>
* "title": "Hello, World!"<br/>
* }
* @description
* A content loader returned an invalid result.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A content loader returned an invalid result.
* A content loader returned an invalid `id`.

Like the title of your PR, if the reason this entry is invalid is only and exactly because of the ID, would it be more helpful to specify this here?

Copy link
Contributor Author

@louisescher louisescher Jan 6, 2025

Choose a reason for hiding this comment

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

I've tried keeping the error as agnostic as possible so it can be used for other validation that might be needed in the future. The actual error content does include the following line:
"Property id has to be a string"
And right below that it shows the entry that caused this error.

Edit: The actual error message is supplied by the validation schema itself, meaning that this error can easily be extended with further error messages for other properties if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Just checking on this because I'm totally not sure whether this is how we've handled error messages in the past! I'm not sure we've had one that "means different things" and gets a different message based on exactly what went wrong. Usually it's one particular error, but the dynamic part of the message is e.g. identifying the file name or something like that.

Would love for someone to confirm this! Maybe @ematipico would be in a good position to judge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah fair enough, didn't know that. Gonna wait for Ema's input then

Copy link
Member

@ematipico ematipico Jan 8, 2025

Choose a reason for hiding this comment

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

If possible, we usually want to create specific messages. These objects are also created in our documentation, so users who land on our docs want to know exactly what the error is and how to fix it.

I share the sentiment of reusing shared "code"/"logic", but in this case we need to bend our vision towards DX and provide specific messages to users.

So the main message should be hardcoded in the error itself.

* Make sure that the ID of the entry is a string.
* See the [Content collections documentation](https://docs.astro.build/en/guides/content-collections/) for more information.
*/
export const ContentLoaderReturnsInvalidResult = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const ContentLoaderReturnsInvalidResult = {
export const ContentLoaderReturnsInvalidId = {

Just an e.g. based on my earlier question. There are probably lots of ways an entry can be invalid, so does it make sense to entirely base this on invalid ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See reply on previous change request

name: 'ContentLoaderReturnsInvalidResult',
title: 'Content loader returns invalid result.',
message(collection: string, entry: any, message: string) {
return [
`The content loader for the collection **${String(collection)}** returned an invalid result:`,
`${message}:`,
JSON.stringify(entry, null, 2),
].join('\n');
},
hint: 'See https://docs.astro.build/en/guides/content-collections/ for more information on content schemas.',
} satisfies ErrorData;

/**
* @docs
* @message
Expand Down
15 changes: 15 additions & 0 deletions packages/astro/test/content-collections.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,21 @@ describe('Content Collections', () => {
});
});

describe('With numbers for IDs', () => {
it('Throws the right error', async () => {
const fixture = await loadFixture({
root: './fixtures/content-collections-number-id/',
});
let error;
try {
await fixture.build({ force: true });
} catch (e) {
error = e.message;
}
assert.match(error, /Content entry `id` must be a string:/);
});
});

describe('With empty collections directory', () => {
it('Handles the empty directory correctly', async () => {
const fixture = await loadFixture({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@test/content-collections-number-id",
"version": "0.0.0",
"private": true,
"type": "module",
"dependencies": {
"astro": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { defineCollection, z } from 'astro:content';

const data = defineCollection({
loader: async () => ([
{ id: 1, title: 'One!' },
{ id: 2, title: 'Two!' },
{ id: 3, title: 'Three!' },
]),
schema: z.object({
id: z.number(),
title: z.string(),
}),
});

export const collections = {
data,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
import { getEntry } from 'astro:content';
const data = await getEntry('blog', 1);
---

{JSON.stringify(data)}
7 changes: 6 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading