Skip to content

Commit 6a8c831

Browse files
cliffhallcliffhall
authored
Merge pull request #480 from olaservo/fix-required
Fix fundamental schema issue with required fields defined incorrectly on JsonSchemaType
2 parents c24d2a9 + 95975c5 commit 6a8c831

File tree

6 files changed

+520
-81
lines changed

6 files changed

+520
-81
lines changed

‎client/src/components/DynamicJsonForm.tsx

Lines changed: 175 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,39 @@ interface DynamicJsonFormProps {
1616
const isSimpleObject = (schema: JsonSchemaType): boolean => {
1717
const supportedTypes = ["string", "number", "integer", "boolean", "null"];
1818
if (supportedTypes.includes(schema.type)) return true;
19-
if (schema.type !== "object") return false;
20-
return Object.values(schema.properties ?? {}).every((prop) =>
21-
supportedTypes.includes(prop.type),
22-
);
19+
if (schema.type === "object") {
20+
return Object.values(schema.properties ?? {}).every((prop) =>
21+
supportedTypes.includes(prop.type),
22+
);
23+
}
24+
if (schema.type === "array") {
25+
return !!schema.items && isSimpleObject(schema.items);
26+
}
27+
return false;
28+
};
29+
30+
const getArrayItemDefault = (schema: JsonSchemaType): JsonValue => {
31+
if ("default" in schema && schema.default !== undefined) {
32+
return schema.default;
33+
}
34+
35+
switch (schema.type) {
36+
case "string":
37+
return "";
38+
case "number":
39+
case "integer":
40+
return 0;
41+
case "boolean":
42+
return false;
43+
case "array":
44+
return [];
45+
case "object":
46+
return {};
47+
case "null":
48+
return null;
49+
default:
50+
return null;
51+
}
2352
};
2453

2554
const DynamicJsonForm = ({
@@ -113,6 +142,8 @@ const DynamicJsonForm = ({
113142
currentValue: JsonValue,
114143
path: string[] = [],
115144
depth: number = 0,
145+
parentSchema?: JsonSchemaType,
146+
propertyName?: string,
116147
) => {
117148
if (
118149
depth >= maxDepth &&
@@ -122,7 +153,8 @@ const DynamicJsonForm = ({
122153
return (
123154
<JsonEditor
124155
value={JSON.stringify(
125-
currentValue ?? generateDefaultValue(propSchema),
156+
currentValue ??
157+
generateDefaultValue(propSchema, propertyName, parentSchema),
126158
null,
127159
2,
128160
)}
@@ -140,6 +172,10 @@ const DynamicJsonForm = ({
140172
);
141173
}
142174

175+
// Check if this property is required in the parent schema
176+
const isRequired =
177+
parentSchema?.required?.includes(propertyName || "") ?? false;
178+
143179
switch (propSchema.type) {
144180
case "string":
145181
return (
@@ -148,16 +184,11 @@ const DynamicJsonForm = ({
148184
value={(currentValue as string) ?? ""}
149185
onChange={(e) => {
150186
const val = e.target.value;
151-
// Allow clearing non-required fields by setting undefined
152-
// This preserves the distinction between empty string and unset
153-
if (!val && !propSchema.required) {
154-
handleFieldChange(path, undefined);
155-
} else {
156-
handleFieldChange(path, val);
157-
}
187+
// Always allow setting string values, including empty strings
188+
handleFieldChange(path, val);
158189
}}
159190
placeholder={propSchema.description}
160-
required={propSchema.required}
191+
required={isRequired}
161192
/>
162193
);
163194
case "number":
@@ -167,9 +198,7 @@ const DynamicJsonForm = ({
167198
value={(currentValue as number)?.toString() ?? ""}
168199
onChange={(e) => {
169200
const val = e.target.value;
170-
// Allow clearing non-required number fields
171-
// This preserves the distinction between 0 and unset
172-
if (!val && !propSchema.required) {
201+
if (!val && !isRequired) {
173202
handleFieldChange(path, undefined);
174203
} else {
175204
const num = Number(val);
@@ -179,7 +208,7 @@ const DynamicJsonForm = ({
179208
}
180209
}}
181210
placeholder={propSchema.description}
182-
required={propSchema.required}
211+
required={isRequired}
183212
/>
184213
);
185214
case "integer":
@@ -190,9 +219,7 @@ const DynamicJsonForm = ({
190219
value={(currentValue as number)?.toString() ?? ""}
191220
onChange={(e) => {
192221
const val = e.target.value;
193-
// Allow clearing non-required integer fields
194-
// This preserves the distinction between 0 and unset
195-
if (!val && !propSchema.required) {
222+
if (!val && !isRequired) {
196223
handleFieldChange(path, undefined);
197224
} else {
198225
const num = Number(val);
@@ -203,7 +230,7 @@ const DynamicJsonForm = ({
203230
}
204231
}}
205232
placeholder={propSchema.description}
206-
required={propSchema.required}
233+
required={isRequired}
207234
/>
208235
);
209236
case "boolean":
@@ -213,9 +240,135 @@ const DynamicJsonForm = ({
213240
checked={(currentValue as boolean) ?? false}
214241
onChange={(e) => handleFieldChange(path, e.target.checked)}
215242
className="w-4 h-4"
216-
required={propSchema.required}
243+
required={isRequired}
244+
/>
245+
);
246+
case "object":
247+
if (!propSchema.properties) {
248+
return (
249+
<JsonEditor
250+
value={JSON.stringify(currentValue ?? {}, null, 2)}
251+
onChange={(newValue) => {
252+
try {
253+
const parsed = JSON.parse(newValue);
254+
handleFieldChange(path, parsed);
255+
setJsonError(undefined);
256+
} catch (err) {
257+
setJsonError(
258+
err instanceof Error ? err.message : "Invalid JSON",
259+
);
260+
}
261+
}}
262+
error={jsonError}
263+
/>
264+
);
265+
}
266+
267+
return (
268+
<div className="space-y-2 border rounded p-3">
269+
{Object.entries(propSchema.properties).map(([key, subSchema]) => (
270+
<div key={key}>
271+
<label className="block text-sm font-medium mb-1">
272+
{key}
273+
{propSchema.required?.includes(key) && (
274+
<span className="text-red-500 ml-1">*</span>
275+
)}
276+
</label>
277+
{renderFormFields(
278+
subSchema as JsonSchemaType,
279+
(currentValue as Record<string, JsonValue>)?.[key],
280+
[...path, key],
281+
depth + 1,
282+
propSchema,
283+
key,
284+
)}
285+
</div>
286+
))}
287+
</div>
288+
);
289+
case "array": {
290+
const arrayValue = Array.isArray(currentValue) ? currentValue : [];
291+
if (!propSchema.items) return null;
292+
293+
// If the array items are simple, render as form fields, otherwise use JSON editor
294+
if (isSimpleObject(propSchema.items)) {
295+
return (
296+
<div className="space-y-4">
297+
{propSchema.description && (
298+
<p className="text-sm text-gray-600">
299+
{propSchema.description}
300+
</p>
301+
)}
302+
303+
{propSchema.items?.description && (
304+
<p className="text-sm text-gray-500">
305+
Items: {propSchema.items.description}
306+
</p>
307+
)}
308+
309+
<div className="space-y-2">
310+
{arrayValue.map((item, index) => (
311+
<div key={index} className="flex items-center gap-2">
312+
{renderFormFields(
313+
propSchema.items as JsonSchemaType,
314+
item,
315+
[...path, index.toString()],
316+
depth + 1,
317+
)}
318+
<Button
319+
variant="outline"
320+
size="sm"
321+
onClick={() => {
322+
const newArray = [...arrayValue];
323+
newArray.splice(index, 1);
324+
handleFieldChange(path, newArray);
325+
}}
326+
>
327+
Remove
328+
</Button>
329+
</div>
330+
))}
331+
<Button
332+
variant="outline"
333+
size="sm"
334+
onClick={() => {
335+
const defaultValue = getArrayItemDefault(
336+
propSchema.items as JsonSchemaType,
337+
);
338+
handleFieldChange(path, [...arrayValue, defaultValue]);
339+
}}
340+
title={
341+
propSchema.items?.description
342+
? `Add new ${propSchema.items.description}`
343+
: "Add new item"
344+
}
345+
>
346+
Add Item
347+
</Button>
348+
</div>
349+
</div>
350+
);
351+
}
352+
353+
// For complex arrays, fall back to JSON editor
354+
return (
355+
<JsonEditor
356+
value={JSON.stringify(currentValue ?? [], null, 2)}
357+
onChange={(newValue) => {
358+
try {
359+
const parsed = JSON.parse(newValue);
360+
handleFieldChange(path, parsed);
361+
setJsonError(undefined);
362+
} catch (err) {
363+
setJsonError(
364+
err instanceof Error ? err.message : "Invalid JSON",
365+
);
366+
}
367+
}}
368+
error={jsonError}
217369
/>
218370
);
371+
}
219372
default:
220373
return null;
221374
}

‎client/src/components/ToolsTab.tsx

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { TabsContent } from "@/components/ui/tabs";
77
import { Textarea } from "@/components/ui/textarea";
88
import DynamicJsonForm from "./DynamicJsonForm";
99
import type { JsonValue, JsonSchemaType } from "@/utils/jsonUtils";
10-
import { generateDefaultValue } from "@/utils/schemaUtils";
10+
import { generateDefaultValue, isPropertyRequired } from "@/utils/schemaUtils";
1111
import {
1212
CompatibilityCallToolResult,
1313
ListToolsResult,
@@ -48,7 +48,11 @@ const ToolsTab = ({
4848
selectedTool?.inputSchema.properties ?? [],
4949
).map(([key, value]) => [
5050
key,
51-
generateDefaultValue(value as JsonSchemaType),
51+
generateDefaultValue(
52+
value as JsonSchemaType,
53+
key,
54+
selectedTool?.inputSchema as JsonSchemaType,
55+
),
5256
]);
5357
setParams(Object.fromEntries(params));
5458
}, [selectedTool]);
@@ -92,13 +96,19 @@ const ToolsTab = ({
9296
{Object.entries(selectedTool.inputSchema.properties ?? []).map(
9397
([key, value]) => {
9498
const prop = value as JsonSchemaType;
99+
const inputSchema =
100+
selectedTool.inputSchema as JsonSchemaType;
101+
const required = isPropertyRequired(key, inputSchema);
95102
return (
96103
<div key={key}>
97104
<Label
98105
htmlFor={key}
99106
className="block text-sm font-medium text-gray-700 dark:text-gray-300"
100107
>
101108
{key}
109+
{required && (
110+
<span className="text-red-500 ml-1">*</span>
111+
)}
102112
</Label>
103113
{prop.type === "boolean" ? (
104114
<div className="flex items-center space-x-2 mt-2">

0 commit comments

Comments
 (0)