feat: generic importers support#22
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces testing infrastructure and schema definitions for Postman environment importing functionality. The changes add Vitest-based testing with configuration and multiple test scripts to package.json, define an abstract BaseImporter class providing parsing and conversion patterns, implement a concrete EnvironmentImporter that transforms Postman environments into internal structures, create Zod and JSON schemas for Postman environments and collections, include comprehensive test fixtures and helper utilities, and add a Node.js script for automating JSON-to-Zod schema conversion. All changes are additive, with new dependencies (Zod, Vitest, Prettier, json-schema-to-zod) and approximately 2,100 lines of code, tests, fixtures, and configuration. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/apiClient/importers/base-importer.ts`:
- Around line 14-24: The three methods canParseRecord, safeParse, and parse
currently accept z.infer<I> which is the post-parse type; change their input
parameter types to unknown so runtime validation can accept untrusted data
(e.g., canParseRecord(data: unknown), safeParse(data: unknown), parse(data:
unknown)), keep using this.schema.safeParse/parse inside each method and
preserve return types, and update any callers/tests to pass unknown or cast as
needed; reference the methods canParseRecord, safeParse, parse and the
this.schema usage to locate the changes.
- Around line 26-32: The import flow should accept raw unknown input, validate
it via parse, then pass the validated value to convert; change import's
signature to import(data: unknown): Promise<O>, keep/ensure the abstract convert
signature accepts the parsed type (abstract convert(data: z.infer<I>):
Promise<O>), call const parsedData = this.parse(data) and then await
this.convert(parsedData) so types line up (parse returns z.infer<I> and convert
consumes that). Ensure any callers are updated to pass raw unknown to import
rather than already-typed data.
🧹 Nitpick comments (1)
src/apiClient/importers/base-importer.ts (1)
7-10: Minor:O extends anyis redundant.The constraint
extends anyhas no effect since all types extendany. You can simplify to justO.Proposed fix
abstract class BaseImporter< I extends ZodType, - O extends any + O > {
| canParseRecord(data: z.infer<I>) { | ||
| return this.schema.safeParse(data).success; | ||
| } | ||
|
|
||
| safeParse(data: z.infer<I>) { | ||
| return this.schema.safeParse(data); | ||
| } | ||
|
|
||
| parse(data: z.infer<I>) { | ||
| return this.schema.parse(data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Parameter types should accept unknown for meaningful runtime validation.
The methods canParseRecord, safeParse, and parse use z.infer<I> as the input type, which represents the output type after successful parsing. This creates a type-level contradiction: if TypeScript already knows the data conforms to z.infer<I>, runtime validation is redundant.
For validation utilities, accept unknown to allow validating untrusted input at runtime while still benefiting from type narrowing:
Proposed fix
- canParseRecord(data: z.infer<I>) {
+ canParseRecord(data: unknown): data is z.infer<I> {
return this.schema.safeParse(data).success;
}
- safeParse(data: z.infer<I>) {
+ safeParse(data: unknown) {
return this.schema.safeParse(data);
}
- parse(data: z.infer<I>) {
+ parse(data: unknown): z.infer<I> {
return this.schema.parse(data);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canParseRecord(data: z.infer<I>) { | |
| return this.schema.safeParse(data).success; | |
| } | |
| safeParse(data: z.infer<I>) { | |
| return this.schema.safeParse(data); | |
| } | |
| parse(data: z.infer<I>) { | |
| return this.schema.parse(data); | |
| } | |
| canParseRecord(data: unknown): data is z.infer<I> { | |
| return this.schema.safeParse(data).success; | |
| } | |
| safeParse(data: unknown) { | |
| return this.schema.safeParse(data); | |
| } | |
| parse(data: unknown): z.infer<I> { | |
| return this.schema.parse(data); | |
| } |
🤖 Prompt for AI Agents
In `@src/apiClient/importers/base-importer.ts` around lines 14 - 24, The three
methods canParseRecord, safeParse, and parse currently accept z.infer<I> which
is the post-parse type; change their input parameter types to unknown so runtime
validation can accept untrusted data (e.g., canParseRecord(data: unknown),
safeParse(data: unknown), parse(data: unknown)), keep using
this.schema.safeParse/parse inside each method and preserve return types, and
update any callers/tests to pass unknown or cast as needed; reference the
methods canParseRecord, safeParse, parse and the this.schema usage to locate the
changes.
| abstract convert(data: z.infer<I>): Promise<O>; | ||
|
|
||
| async import(data: z.infer<I>): Promise<O> { | ||
| const parsedData = this.parse(data); | ||
| const convertedData = await this.convert(parsedData); | ||
| return convertedData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same typing issue applies to convert and import methods.
The import method is the primary entry point and should accept unknown data, validate it, then pass the validated result to convert.
Proposed fix
abstract convert(data: z.infer<I>): Promise<O>;
- async import(data: z.infer<I>): Promise<O> {
+ async import(data: unknown): Promise<O> {
const parsedData = this.parse(data);
const convertedData = await this.convert(parsedData);
return convertedData;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| abstract convert(data: z.infer<I>): Promise<O>; | |
| async import(data: z.infer<I>): Promise<O> { | |
| const parsedData = this.parse(data); | |
| const convertedData = await this.convert(parsedData); | |
| return convertedData; | |
| } | |
| abstract convert(data: z.infer<I>): Promise<O>; | |
| async import(data: unknown): Promise<O> { | |
| const parsedData = this.parse(data); | |
| const convertedData = await this.convert(parsedData); | |
| return convertedData; | |
| } |
🤖 Prompt for AI Agents
In `@src/apiClient/importers/base-importer.ts` around lines 26 - 32, The import
flow should accept raw unknown input, validate it via parse, then pass the
validated value to convert; change import's signature to import(data: unknown):
Promise<O>, keep/ensure the abstract convert signature accepts the parsed type
(abstract convert(data: z.infer<I>): Promise<O>), call const parsedData =
this.parse(data) and then await this.convert(parsedData) so types line up (parse
returns z.infer<I> and convert consumes that). Ensure any callers are updated to
pass raw unknown to import rather than already-typed data.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/apiClient/importers/postman/environment/schemas/environment.ts`:
- Around line 4-10: Update the PostmanEnvironmentValueSchema so it accepts
Postman's real-world exports where "value" can be null: change the value field
on PostmanEnvironmentValueSchema from its current strict z.string() to allow
nulls and missing keys (use z.string().nullable().optional()). This will let the
parser accept exported variables with value null so the fallback logic in
environment_importer (the variable.value ?? "" handling) can run as intended.
🧹 Nitpick comments (7)
src/apiClient/importers/postman/environment/__tests__/fixtures/invalid/invalid-types.json (1)
1-12: Consider separating type violations into individual fixtures for targeted assertions.This fixture bundles three distinct type violations in one entry (
valueas number,typeas invalid enum,enabledas string). This makes it harder to write isolated assertions for each validation error. Consider splitting into separate fixtures (one violation each) if the test suite needs to verify specific error messages per field.That said, if the test only checks that parsing fails generically, this is fine as-is.
src/apiClient/importers/postman/environment/__tests__/helpers.ts (2)
3-4:PostmanVariableTypeduplicates the schema enum — consider deriving it.This local type mirrors the
typeenum inPostmanEnvironmentValueSchema. If the schema's enum values change, this won't stay in sync. You could derive it from the schema or importPostmanEnvironmentValue["type"]instead.type PostmanVariableType = NonNullable<PostmanEnvironmentValue["type"]>;
29-35: Simplify theenableddefault.♻️ Proposed simplification
- enabled: options?.enabled !== undefined ? options.enabled : true, + enabled: options?.enabled ?? true,The
?? trueform is equivalent here sinceenabledis a boolean (nevernull).src/apiClient/importers/postman/environment/__tests__/environment_importer.test.ts (2)
20-29: Type assertions on JSON imports bypass schema validation.Casting raw JSON as
PostmanEnvironmentskips compile-time type checking. The tests forcanParseRecordandsafeParsepartly mitigate this for invalid fixtures, but the valid/edge-case fixtures are trusted without runtime validation in the test setup.Consider parsing the JSON through the schema in a setup step (or a helper) so fixture typos surface as test failures rather than silently passing with wrong data.
127-135: Test name says "efficiently" but doesn't measure performance.This test verifies correctness with 15 variables, not efficiency. Consider renaming to something like
"should handle large environments correctly"to avoid confusion.src/apiClient/importers/postman/environment/environment_importer.ts (2)
15-15: Markschemaasreadonlyto prevent accidental reassignment.♻️ Proposed fix
- schema = PostmanEnvironmentSchema; + readonly schema = PostmanEnvironmentSchema;
18-36: Skipped variables (empty keys) are silently dropped — consider adding a warning.When a variable has an empty key (line 20-22), it's filtered out without producing any feedback. Since the
warningsarray (line 47) is already part of the output contract, populating it with info about skipped entries would give callers visibility.♻️ Proposed approach
+ const warnings: string[] = []; const variables = data.values.reduce( (acc: Record<string, EnvironmentVariableData>, variable: PostmanEnvironmentValue, index: number) => { if (!variable.key) { + warnings.push(`Skipped variable at index ${index}: empty key`); return acc; } // ... }, {} ); // ... return { data: [environment], errors: [], - warnings: [], + warnings, notSupportedFeatures: [], };
| const PostmanEnvironmentValueSchema = z.object({ | ||
| key: z.string(), | ||
| value: z.string(), | ||
| type: z.enum(["default", "secret", "text", "number", "boolean"]).optional(), | ||
| enabled: z.boolean().optional(), | ||
| description: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/apiClient/importers/postman/environment/schemas/environment.tsRepository: requestly/alternative-importers
Length of output: 1119
🏁 Script executed:
cat -n src/apiClient/importers/postman/environment/environment_importer.ts | head -40Repository: requestly/alternative-importers
Length of output: 1636
🌐 Web query:
Postman environment export JSON null values variables
💡 Result:
When you export a Postman environment, seeing "value": null (or empty values) usually means the values you entered are local-only and therefore not included in the exported JSON.
- In newer Postman, variable values are local by default (not synced to the cloud). Only values you explicitly Share get synced and are usable by cloud features—and those are the ones that reliably survive sharing/export workflows. [1]
- In older Postman terminology, exports didn’t include Current Value (local); they only included the shared/synced value (formerly Initial Value). If your values were only in Current Value, exports would show names but no values (null/empty). [2]
- If you want the exported JSON to contain the values you currently have, you need to persist/copy current → initial/shared (e.g., use Persist All in the variable editor in versions that support it). [3]
What to do
- Open the environment → for each variable, click Share (or “Share all”) so it has a shared value. [1]
or - Use Persist All to copy current values into the exportable/shared slot. [3]
Why Postman does this: it’s a privacy/safety design—local/current values aren’t automatically persisted/shared/exported. [3]
Sources:
[1] Postman Docs – variables are local by default; you can share values; export environments via Export action. (learning.postman.com)
[2] Postman Community – if values are only in Current Value, they aren’t synced and aren’t part of the exported file. (community.postman.com)
[3] Postman Support – “persist variable values” replaces initial values with current values; auto-persist removed for privacy. (support.postman.com)
Citations:
- 1: https://learning.postman.com/docs/postman/environments_and_globals/variables/
- 2: https://community.postman.com/t/downloading-environment-variables-omits-variable-values/4247
- 3: https://support.postman.com/hc/en-us/articles/4409005403031-How-to-persist-variable-values
🏁 Script executed:
# Check if there are test fixtures that test null/empty value scenarios
find . -name "*.json" -path "*/postman*" | head -20Repository: requestly/alternative-importers
Length of output: 1509
🏁 Script executed:
# Search for test files related to environment import
fd -e test -e spec -path "*postman*environment*" . | head -20Repository: requestly/alternative-importers
Length of output: 243
🏁 Script executed:
# Look for any existing null value test cases
rg -i "null.*value\|value.*null" --type json . | grep -i postman | head -10Repository: requestly/alternative-importers
Length of output: 57
🏁 Script executed:
# Check the empty-values.json fixture to see if it has null values
cat -n src/apiClient/importers/postman/environment/__tests__/fixtures/valid/empty-values.jsonRepository: requestly/alternative-importers
Length of output: 183
🏁 Script executed:
# Check the missing-required-fields.json to understand error handling
cat -n src/apiClient/importers/postman/environment/__tests__/fixtures/invalid/missing-required-fields.jsonRepository: requestly/alternative-importers
Length of output: 145
🏁 Script executed:
# Check the test file to see what validation scenarios are tested
find . -path "*/postman/environment/__tests__*" -name "*.ts" -o -name "*.js" | head -5Repository: requestly/alternative-importers
Length of output: 245
🏁 Script executed:
# Examine the test file to understand what scenarios are tested
cat -n src/apiClient/importers/postman/environment/__tests__/environment_importer.test.tsRepository: requestly/alternative-importers
Length of output: 15212
🏁 Script executed:
# Check if there's any fixture with null value in the value field
rg "\"value\"\s*:\s*null" src/apiClient/importers/postman/environment/__tests__/fixtures/ -A 1 -B 1Repository: requestly/alternative-importers
Length of output: 57
🏁 Script executed:
# Check the missing-optional-values.json fixture
cat -n src/apiClient/importers/postman/environment/__tests__/fixtures/edge-cases/missing-optional-values.jsonRepository: requestly/alternative-importers
Length of output: 460
🏁 Script executed:
# Check all the other test fixtures to see if any have null values
cat -n src/apiClient/importers/postman/environment/__tests__/fixtures/valid/basic-environment.jsonRepository: requestly/alternative-importers
Length of output: 690
🏁 Script executed:
# Search all fixture files for null values
for f in src/apiClient/importers/postman/environment/__tests__/fixtures/**/*.json; do
if grep -q "null" "$f" 2>/dev/null; then
echo "=== $f ==="
cat -n "$f"
fi
doneRepository: requestly/alternative-importers
Length of output: 57
🏁 Script executed:
# Check if there are any real-world test cases or documentation about handling null values
cat -n src/apiClient/importers/postman/environment/__tests__/fixtures/invalid/invalid-types.jsonRepository: requestly/alternative-importers
Length of output: 360
🏁 Script executed:
# Verify the exact current schema definition one more time
sed -n '4,10p' src/apiClient/importers/postman/environment/schemas/environment.tsRepository: requestly/alternative-importers
Length of output: 319
value: z.string() will reject real Postman exports where value is null.
Postman exports environment variables with "value": null for local-only (non-shared) variables. This is a privacy-by-design feature where local values aren't synced or exported. The schema currently rejects these valid exports at parse time. The fallback at environment_importer.ts line 27 (variable.value ?? "") was written to handle this case but never receives null values due to schema validation rejecting them first.
Change value: z.string() to value: z.string().nullable().optional() to allow these real-world Postman exports to parse successfully.
🤖 Prompt for AI Agents
In `@src/apiClient/importers/postman/environment/schemas/environment.ts` around
lines 4 - 10, Update the PostmanEnvironmentValueSchema so it accepts Postman's
real-world exports where "value" can be null: change the value field on
PostmanEnvironmentValueSchema from its current strict z.string() to allow nulls
and missing keys (use z.string().nullable().optional()). This will let the
parser accept exported variables with value null so the fallback logic in
environment_importer (the variable.value ?? "" handling) can run as intended.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/apiClient/importers/postman/environment/__tests__/environment_importer.test.ts`:
- Around line 46-103: Add a unit test in environment_importer.test.ts that
covers Postman variables with a null "value" to verify the importer uses the
fallback (variable.value ?? ""): create or reuse a fixture (e.g.,
nullValueEnvironment) containing a variable with key "nullableVar" and value
null, call importer.convert(nullValueEnvironment) and assert the resulting
env.variables.nullableVar.syncValue === "" (and that type is
EnvironmentVariableType.String and isPersisted is true); this ensures the
convert() path that applies the variable.value ?? "" fallback handles nullable
Postman exports correctly.
🧹 Nitpick comments (2)
src/schemas/zod-schemas/postman_environment.ts (1)
1-10: Consider exportingPostmanEnvironmentValueSchema.The value schema is kept internal, but
types.tsderivesPostmanEnvironmentValueviaPostmanEnvironment["values"][number]. Exporting it directly would allow consumers to validate individual values and make the derivation more explicit.src/apiClient/importers/postman/environment/__tests__/environment_importer.test.ts (1)
20-29: Type assertions on edge-case fixtures bypass compile-time safety.Casting edge-case JSON fixtures with
as PostmanEnvironment(e.g.,emptyKeys,missingOptionalValues) silences the compiler even if the fixture shape drifts from the schema. Consider usingimporter.parse()or a runtime assertion in a setup step for edge-case fixtures so mismatches surface as test failures rather than silent casts.
| describe("convert()", () => { | ||
| describe("valid environments", () => { | ||
| it("should convert basic environment correctly", async () => { | ||
| const result = await importer.convert(basicEnvironment); | ||
| const env = result.data[0]; | ||
| const baseUrlVar = basicEnvironment.values.find(v => v.key === "baseUrl")!; | ||
|
|
||
| expect(env.name).toBe(basicEnvironment.name); | ||
| expect(env.scope).toBe(VariableScope.ENVIRONMENT); | ||
| expect(Object.keys(env.variables)).toHaveLength(basicEnvironment.values.length); | ||
| expect(env.variables.baseUrl).toMatchObject({ | ||
| isPersisted: true, | ||
| syncValue: baseUrlVar.value, | ||
| type: EnvironmentVariableType.String, | ||
| }); | ||
| }); | ||
|
|
||
| it("should handle environment with secrets", async () => { | ||
| const result = await importer.convert(environmentWithSecrets); | ||
| const env = result.data[0]; | ||
| const apiSecretVar = environmentWithSecrets.values.find(v => v.key === "apiSecret")!; | ||
|
|
||
| expect(env.name).toBe(environmentWithSecrets.name); | ||
| expect(env.variables.apiSecret).toMatchObject({ | ||
| isPersisted: true, | ||
| syncValue: apiSecretVar.value, | ||
| type: EnvironmentVariableType.Secret, | ||
| }); | ||
| expect(env.variables.apiKey).toMatchObject({ | ||
| type: EnvironmentVariableType.String, | ||
| }); | ||
| }); | ||
|
|
||
| it("should handle global scope correctly", async () => { | ||
| const result = await importer.convert(globalScope); | ||
| const env = result.data[0]; | ||
|
|
||
| expect(env.scope).toBe(VariableScope.GLOBAL); | ||
| expect(env.name).toBe(globalScope.name); | ||
| }); | ||
|
|
||
| it("should handle empty values array", async () => { | ||
| const result = await importer.convert(emptyValues); | ||
| const env = result.data[0]; | ||
|
|
||
| expect(env.name).toBe(emptyValues.name); | ||
| expect(Object.keys(env.variables)).toHaveLength(emptyValues.values.length); | ||
| }); | ||
|
|
||
| it("should convert all enabled variables regardless of disabled flag", async () => { | ||
| const result = await importer.convert(disabledVariables); | ||
| const env = result.data[0]; | ||
|
|
||
| expect(Object.keys(env.variables)).toHaveLength(2); | ||
| expect(env.variables.enabledVar).toBeDefined(); | ||
| expect(env.variables.disabledVar).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test for null variable values.
Related to the schema issue: real Postman exports can have "value": null. There's no test fixture or test case verifying the importer handles null values gracefully. Once the schema is updated to accept nullable values, add a test to confirm the variable.value ?? "" fallback produces the expected output.
🤖 Prompt for AI Agents
In
`@src/apiClient/importers/postman/environment/__tests__/environment_importer.test.ts`
around lines 46 - 103, Add a unit test in environment_importer.test.ts that
covers Postman variables with a null "value" to verify the importer uses the
fallback (variable.value ?? ""): create or reuse a fixture (e.g.,
nullValueEnvironment) containing a variable with key "nullableVar" and value
null, call importer.convert(nullValueEnvironment) and assert the resulting
env.variables.nullableVar.syncValue === "" (and that type is
EnvironmentVariableType.String and isPersisted is true); this ensures the
convert() path that applies the variable.value ?? "" fallback handles nullable
Postman exports correctly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@scripts/convert-schema.js`:
- Around line 43-70: customOneOfParser currently emits z.union([]) for an empty
schema.oneOf which produces invalid Zod; update customOneOfParser (referencing
schema.oneOf and parsedBranches) to early-handle an empty array by returning
undefined (to fall back to the default parser) or another safe Zod expression
(e.g., 'z.never()') before generating parsedBranches.join; implement the check
immediately after building parsedBranches (if parsedBranches.length === 0) so
you never emit z.union([]).
- Around line 103-112: The current use of execSync with a shell string and
temporary file (symbols: execSync, tempFile, outputPath, zodCodeWithHeader,
baseFilename) is vulnerable to command injection and can leave temp files or
truncated outputs on failure; instead, avoid shell interpolation by calling
Prettier via a child process without shell (e.g., spawnSync/execFile) and pipe
zodCodeWithHeader to the process's stdin, writing the formatted stdout to
outputPath atomically (write to a .tmp and rename) so filenames are not embedded
in a shell, and ensure any temporary file is removed in a finally/failure path
if you keep a temp file approach.
In `@src/schemas/json-schemas/postman_collection_2_1.json`:
- Around line 230-238: The "cookie-list" JSON Schema definition has an incorrect
title "Certificate List"; update the title value for the definition named
"cookie-list" to "Cookie List" so it matches the description and purpose (look
for the "cookie-list" definition block and change its "title" property
accordingly).
- Around line 156-163: The oauth1 schema block has an incorrect description
mentioning OAuth2; update the "description" property in the "oauth1" definition
to reference OAuth1 (e.g., "The attributes for [OAuth1](https://oauth.net/1/)")
so it accurately reflects the oauth1 array items referenced by "$ref":
"#/definitions/auth-attribute"; modify the description string in the oauth1
object accordingly.
🧹 Nitpick comments (1)
scripts/convert-schema.js (1)
72-123: Unhandled promise rejection on the entry-level call.
convertSchema()returns a promise (async function) but the call at line 125 doesn't attach a.catch(). If the promise rejects with an error not caught inside thetry/catch(e.g., an error thrown afterprocess.exit), Node.js will emit anUnhandledPromiseRejectionWarning. In practice, the internal try/catch covers most paths, so this is low risk.Belt-and-suspenders fix
-convertSchema(); +convertSchema().catch((err) => { + console.error('❌ Unexpected error:', err); + process.exit(1); +});
| function customOneOfParser(schema, refs) { | ||
| // Only handle schemas that have oneOf | ||
| if (!schema.oneOf || !Array.isArray(schema.oneOf)) { | ||
| return; // Return undefined to use default parser | ||
| } | ||
|
|
||
| // Convert each oneOf branch to Zod independently | ||
| const parsedBranches = schema.oneOf.map((subSchema) => { | ||
| // Generate Zod code for each branch, passing the same parser override | ||
| // so nested oneOf instances also get converted to z.union() | ||
| const branchCode = jsonSchemaToZod(subSchema, { | ||
| module: 'none', | ||
| noImport: true, | ||
| parserOverride: customOneOfParser, // Recursive: handle nested oneOf | ||
| }); | ||
|
|
||
| // Remove any trailing semicolons and whitespace | ||
| return branchCode.trim().replace(/;$/, ''); | ||
| }); | ||
|
|
||
| // Handle single branch case | ||
| if (parsedBranches.length === 1) { | ||
| return parsedBranches[0]; | ||
| } | ||
|
|
||
| // Create union with all branches | ||
| return `z.union([${parsedBranches.join(', ')}])`; | ||
| } |
There was a problem hiding this comment.
Edge case: empty oneOf array would produce invalid Zod code.
If schema.oneOf is [], parsedBranches will be empty and line 69 would emit z.union([]), which is invalid in Zod (requires at least 2 members). This is unlikely with valid Postman schemas but could surface with malformed input.
Also worth noting: this treats oneOf (exactly one match) as z.union() (first match / any match), which relaxes validation. The comment on line 41 acknowledges this, so flagging just for visibility.
🤖 Prompt for AI Agents
In `@scripts/convert-schema.js` around lines 43 - 70, customOneOfParser currently
emits z.union([]) for an empty schema.oneOf which produces invalid Zod; update
customOneOfParser (referencing schema.oneOf and parsedBranches) to early-handle
an empty array by returning undefined (to fall back to the default parser) or
another safe Zod expression (e.g., 'z.never()') before generating
parsedBranches.join; implement the check immediately after building
parsedBranches (if parsedBranches.length === 0) so you never emit z.union([]).
| const tempFile = outputPath + '.tmp'; | ||
| fs.writeFileSync(tempFile, zodCodeWithHeader); | ||
|
|
||
| execSync( | ||
| `prettier --parser typescript "${tempFile}" > "${outputPath}"`, | ||
| { stdio: 'inherit', shell: true } | ||
| ); | ||
|
|
||
| // Clean up temp file | ||
| fs.unlinkSync(tempFile); |
There was a problem hiding this comment.
Command injection via unsanitized filename in execSync.
baseFilename originates from process.argv[2] and is interpolated directly into a shell command with shell: true. A filename containing shell metacharacters (e.g., foo; rm -rf /) would execute arbitrary commands. Even for a dev-only script, this is a bad pattern that can cause accidental damage with special characters in filenames.
Additionally, if execSync (prettier) fails, the temp file at outputPath + '.tmp' is never cleaned up, and the shell > redirection may leave outputPath truncated/empty.
Proposed fix — use stdio piping instead of shell redirection
- const tempFile = outputPath + '.tmp';
- fs.writeFileSync(tempFile, zodCodeWithHeader);
-
- execSync(
- `prettier --parser typescript "${tempFile}" > "${outputPath}"`,
- { stdio: 'inherit', shell: true }
- );
-
- // Clean up temp file
- fs.unlinkSync(tempFile);
+ // Format with Prettier using stdin/stdout to avoid shell injection and temp files
+ const formatted = execSync(
+ 'prettier --parser typescript',
+ { input: zodCodeWithHeader, encoding: 'utf8' }
+ );
+ fs.writeFileSync(outputPath, formatted);This approach pipes the content via stdin, eliminating the need for a temp file, shell redirection, and filename interpolation in the command string entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tempFile = outputPath + '.tmp'; | |
| fs.writeFileSync(tempFile, zodCodeWithHeader); | |
| execSync( | |
| `prettier --parser typescript "${tempFile}" > "${outputPath}"`, | |
| { stdio: 'inherit', shell: true } | |
| ); | |
| // Clean up temp file | |
| fs.unlinkSync(tempFile); | |
| // Format with Prettier using stdin/stdout to avoid shell injection and temp files | |
| const formatted = execSync( | |
| 'prettier --parser typescript', | |
| { input: zodCodeWithHeader, encoding: 'utf8' } | |
| ); | |
| fs.writeFileSync(outputPath, formatted); |
🤖 Prompt for AI Agents
In `@scripts/convert-schema.js` around lines 103 - 112, The current use of
execSync with a shell string and temporary file (symbols: execSync, tempFile,
outputPath, zodCodeWithHeader, baseFilename) is vulnerable to command injection
and can leave temp files or truncated outputs on failure; instead, avoid shell
interpolation by calling Prettier via a child process without shell (e.g.,
spawnSync/execFile) and pipe zodCodeWithHeader to the process's stdin, writing
the formatted stdout to outputPath atomically (write to a .tmp and rename) so
filenames are not embedded in a shell, and ensure any temporary file is removed
in a finally/failure path if you keep a temp file approach.
| "oauth1": { | ||
| "type": "array", | ||
| "title": "OAuth1", | ||
| "description": "The attributes for [OAuth2](https://oauth.net/1/)", | ||
| "items": { | ||
| "$ref": "#/definitions/auth-attribute" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Minor: oauth1 description incorrectly references OAuth2.
Line 159 says "The attributes for [OAuth2]" but this is the oauth1 definition. This is a known issue in the upstream Postman schema, but worth fixing in your copy to avoid confusion during development.
Proposed fix
"oauth1": {
"type": "array",
"title": "OAuth1",
- "description": "The attributes for [OAuth2](https://oauth.net/1/)",
+ "description": "The attributes for [OAuth1](https://oauth.net/1/)",
"items": {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "oauth1": { | |
| "type": "array", | |
| "title": "OAuth1", | |
| "description": "The attributes for [OAuth2](https://oauth.net/1/)", | |
| "items": { | |
| "$ref": "#/definitions/auth-attribute" | |
| } | |
| }, | |
| "oauth1": { | |
| "type": "array", | |
| "title": "OAuth1", | |
| "description": "The attributes for [OAuth1](https://oauth.net/1/)", | |
| "items": { | |
| "$ref": "#/definitions/auth-attribute" | |
| } | |
| }, |
🤖 Prompt for AI Agents
In `@src/schemas/json-schemas/postman_collection_2_1.json` around lines 156 - 163,
The oauth1 schema block has an incorrect description mentioning OAuth2; update
the "description" property in the "oauth1" definition to reference OAuth1 (e.g.,
"The attributes for [OAuth1](https://oauth.net/1/)") so it accurately reflects
the oauth1 array items referenced by "$ref": "#/definitions/auth-attribute";
modify the description string in the oauth1 object accordingly.
| "cookie-list": { | ||
| "$schema": "http://json-schema.org/draft-04/schema#", | ||
| "id": "#/definitions/cookie-list", | ||
| "title": "Certificate List", | ||
| "description": "A representation of a list of cookies", | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/cookie" | ||
| } |
There was a problem hiding this comment.
Minor: cookie-list title says "Certificate List" instead of "Cookie List".
This is another copy-paste error inherited from the upstream Postman schema. The description is correct but the title is wrong.
Proposed fix
"cookie-list": {
"$schema": "http://json-schema.org/draft-04/schema#",
"id": "#/definitions/cookie-list",
- "title": "Certificate List",
+ "title": "Cookie List",
"description": "A representation of a list of cookies",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "cookie-list": { | |
| "$schema": "http://json-schema.org/draft-04/schema#", | |
| "id": "#/definitions/cookie-list", | |
| "title": "Certificate List", | |
| "description": "A representation of a list of cookies", | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/definitions/cookie" | |
| } | |
| "cookie-list": { | |
| "$schema": "http://json-schema.org/draft-04/schema#", | |
| "id": "#/definitions/cookie-list", | |
| "title": "Cookie List", | |
| "description": "A representation of a list of cookies", | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/definitions/cookie" | |
| } |
🤖 Prompt for AI Agents
In `@src/schemas/json-schemas/postman_collection_2_1.json` around lines 230 - 238,
The "cookie-list" JSON Schema definition has an incorrect title "Certificate
List"; update the title value for the definition named "cookie-list" to "Cookie
List" so it matches the description and purpose (look for the "cookie-list"
definition block and change its "title" property accordingly).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores