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

feat(core): Add endpoint GET /projects/:projectId/folders (no-changelog) #13519

Open
wants to merge 82 commits into
base: master
Choose a base branch
from

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Feb 26, 2025

Based on PR #13516

Chain of upstream PRs as of 2025-02-25

Summary

This endpoint retrieves a list of available folders where the user can move the contents of a deleted folder.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/ADO-3270/[be]-add-endpoint-to-list-folders

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@RicardoE105 RicardoE105 force-pushed the ado-3270-be-add-endpoint-to-list-folders branch from ae7b4ca to 1c6bb55 Compare February 27, 2025 16:27
@RicardoE105 RicardoE105 force-pushed the ado-3270-be-add-endpoint-to-list-folders branch from 1c6bb55 to 754a8d2 Compare February 27, 2025 16:29
Base automatically changed from ado-3167-be-add-endpoint-to-delete-folder to master February 27, 2025 21:47
@RicardoE105 RicardoE105 requested a review from a team as a code owner February 27, 2025 21:47
@RicardoE105 RicardoE105 changed the title feat(core): Add endpoint GET /projects/:projectId/folders (no-changelog) feat(core): Add endpoint GET /projects/:projectId/folders (no-changelog) Feb 27, 2025
@RicardoE105 RicardoE105 removed the request for review from a team February 27, 2025 23:52
@RicardoE105 RicardoE105 changed the title feat(core): Add endpoint GET /projects/:projectId/folders (no-changelog) feat(core): Add endpoint GET /projects/:projectId/folders Feb 27, 2025
@RicardoE105 RicardoE105 changed the title feat(core): Add endpoint GET /projects/:projectId/folders feat(core): Add endpoint GET /projects/:projectId/folders (no-changelog) Feb 27, 2025
Copy link
Contributor

@Cadiac Cadiac left a comment

Choose a reason for hiding this comment

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

Some suggestions on how to simplify the schema validation on DTOs :D

});

// SortBy parameter validation
const sortByValidator = z
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use z.enum instead (which also has a nice default message that could be used if the exact message isn't important)

Suggested change
const sortByValidator = z
const sortByValidator = z
.enum(VALID_SORT_OPTIONS, { message: `sortBy must be one of: ${VALID_SORT_OPTIONS.join(', ')}` })
.optional();

// ---------------------

// Filter parameter validation
const filterValidator = z
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using superRefine and transform what if we just did this on a single transform, it also can set issues via ctx?

Suggested change
const filterValidator = z
const filterValidator = z
.string()
.optional()
.transform((val, ctx) => {
if (!val) return undefined;
try {
const parsed: unknown = JSON.parse(val);
try {
return filterSchema.parse(parsed);
} catch (e) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Invalid filter fields',
path: ['filter'],
});
return z.NEVER;
}
} catch (e) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Invalid filter format',
path: ['filter'],
});
return z.NEVER;
}
});

Comment on lines +88 to +90
.refine((val) => val === undefined || !isNaN(val), {
message: 'Skip must be a valid number',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

refine doesn't need to check for undefined here

Suggested change
.refine((val) => val === undefined || !isNaN(val), {
message: 'Skip must be a valid number',
});
.refine((val) => !isNaN(val), {
message: 'Skip must be a valid number',
});

.string()
.optional()
.transform((val) => (val ? parseInt(val, 10) : 10))
.refine((val) => val === undefined || !isNaN(val), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Suggested change
.refine((val) => val === undefined || !isNaN(val), {
.refine((val) => !isNaN(val), {
message: 'Take must be a valid number',
});

@@ -0,0 +1,181 @@
import { ApplicationError, jsonParse } from 'n8n-workflow';
Copy link
Contributor

Choose a reason for hiding this comment

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

How necessary is using jsonParse here, as we're using it without the extra features. Now using it brings extra production dependency.

.strict();

// Common transformers
const parseJsonArray = (val: string): unknown => {
Copy link
Contributor

Choose a reason for hiding this comment

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

With the suggestions this and isArrayOfStrings become unnecessary

});

// Select parameter validation
const selectValidator = z
Copy link
Contributor

Choose a reason for hiding this comment

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

This could written utilizing zod more as

Suggested change
const selectValidator = z
// Select parameter validation
const selectFieldsValidator = z.array(z.enum(VALID_SELECT_FIELDS));
const selectValidator = z
.string()
.optional()
.transform((val, ctx) => {
if (!val) return undefined;
try {
const parsed: unknown = JSON.parse(val);
try {
const selectFields = selectFieldsValidator.parse(parsed);
if (selectFields.length === 0) return undefined;
type SelectField = (typeof VALID_SELECT_FIELDS)[number];
return selectFields.reduce<Record<SelectField, true>>(
(acc, field) => ({ ...acc, [field]: true }),
{} as Record<SelectField, true>,
);
} catch (e) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: `Invalid select fields. Valid fields are: ${VALID_SELECT_FIELDS.join(', ')}`,
path: ['select'],
});
return z.NEVER;
}
} catch (e) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Invalid select format',
path: ['select'],
});
return z.NEVER;
}
});

Note that this suggestion slightly changes the error messages, if they are important and shown on UI or something please adjust

},
])('should validate $name', ({ request }) => {
const result = ListFolderQueryDto.safeParse(request);
expect(result.success).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have any tests here testing that what these DTOs produce are what we expect, and as there's some amount of logic in there I feel like we maybe should have some.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants