Add Date type support with string format date-time #24
Conversation
Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🔍 PR Complexity Assessment🟢 Risk Score: 2/10
📝 SummaryThis PR adds support for TypeScript Date type in schema-forge, converting Date properties to JSON Schema string type with format "date-time". It includes new helper functions, type interface updates, and comprehensive tests across multiple LLM provider formats. 📊 Lines Analysis
💡 RecommendationLow-risk feature addition. Review the isDateType() helper function and verify the format: "date-time" is correctly stripped in OpenAI structured output mode as noted in the checklist. The changes are additive and well-tested. This assessment is automated and should be used as a guide. Please use your judgment when reviewing. |
📝 WalkthroughWalkthroughAdds class-validator metadata inference and Date type handling to schema generation: Date properties emit JSON Schema strings with "date-time" format; class-validator decorators are mapped to JSON Schema constraints and merged into property schemas unless explicitly provided in ToolProp options. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Arrays of Date types are not converted to string format date-time
When a property is an array of Date types (e.g., @ToolProp({ items: { type: Date } }) dates: Date[];), the Date type is not properly converted to { type: 'string', format: 'date-time' }.
Click to expand
Root Cause
In src/lib/decorators.ts lines 223-242, when handling array properties, the code checks:
options.enum- handles enum arrays!options.items- throws error if no items specifiedisCustomClass(options.items.type)- handles custom class arrays- else - passes through
options.itemsas-is
Since Date was added to the exclusion list in isCustomClass() at src/lib/utils.ts:162, isCustomClass(Date) now returns false. This means when options.items.type is Date, the code falls through to the else branch (lines 239-241) which just passes options.items directly:
} else {
finalOptions.type = 'array';
finalOptions.items = options.items; // { type: Date } - the constructor, not 'string'
}Actual vs Expected
- Actual:
{ type: 'array', items: { type: Date } }whereDateis the constructor function - Expected:
{ type: 'array', items: { type: 'string', format: 'date-time' } }
Impact
This results in an invalid JSON schema since Date (the constructor function) is not a valid JSON schema type. LLM APIs would reject this schema or produce unexpected behavior.
(Refers to lines 239-241)
Recommendation: Add a check for isDateType(options.items.type) before the else branch:
} else if (isDateType(options.items.type)) {
finalOptions.type = 'array';
finalOptions.items = { type: 'string', format: 'date-time' };
} else {
finalOptions.type = 'array';
finalOptions.items = options.items;
}
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/types.ts`:
- Around line 38-47: addSchemaProperty is not propagating the new
PropertyOptions.format into the schema, causing date-time formatting to be lost
for dynamically added properties; update the addSchemaProperty implementation to
accept and pass through the format field from PropertyOptions into the created
JSONSchemaProperty (ensure the format value from PropertyOptions is assigned to
the JSONSchemaProperty.format when building the property entry) so that
JSONSchemaProperty.format is preserved for functions/classes that call
addSchemaProperty.
Add support for inferring JSON Schema properties from class-validator decorators: - ArrayMaxSize -> maxItems - ArrayMinSize -> minItems - Max -> maximum (for numbers) - Min -> minimum (for numbers) - IsInt -> type: integer - MinLength -> minLength (for strings) - MaxLength -> maxLength (for strings) - IsUrl -> format: uri - IsPositive -> minimum: 1 Also adds support for manually specifying these properties via ToolProp options. Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
There was a problem hiding this comment.
🟡 Integer type constraints not stripped for OpenAI structured output
The prepareForOpenAIStructuredOutput function strips unsupported properties like minimum, maximum only for type: 'number' but not for type: 'integer'.
Click to expand
Problem
The PR adds support for type: 'integer' via class-validator's IsInt decorator and direct PropertyOptions. However, in src/lib/utils.ts:93-100, the code only checks for 'number' type when stripping unsupported constraints:
} else if (
newObj.type === 'number' ||
(Array.isArray(newObj.type) && newObj.type.includes('number'))
) {
// Remove unsupported number properties
['minimum', 'maximum', 'multipleOf'].forEach((prop) => {
if (prop in newObj) delete newObj[prop];
});
}Impact
When a property has type: 'integer' with minimum/maximum constraints (e.g., from @IsInt() and @IsPositive() decorators or direct specification), and the schema is prepared for OpenAI structured output, the constraints won't be stripped. OpenAI's structured output doesn't support these JSON Schema features, which could cause API errors or unexpected behavior.
Example
class TestDto {
@ToolProp({ type: 'integer', minimum: 1 })
score: number;
}
classToOpenAITool(TestDto, { forStructuredOutput: true });
// Result will incorrectly include minimum: 1 for integer type(Refers to lines 93-100)
Recommendation: Add handling for 'integer' type alongside 'number':
} else if (
newObj.type === 'number' ||
newObj.type === 'integer' ||
(Array.isArray(newObj.type) && (newObj.type.includes('number') || newObj.type.includes('integer')))
) {
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/class-validator-integration.ts`:
- Around line 76-93: The call to storage.getTargetValidationMetadatas in
src/lib/class-validator-integration.ts is passing undefined as the second
parameter but the API requires a string targetSchema; update the call used in
the function (the storage.getTargetValidationMetadatas(...) invocation) to pass
an appropriate string (e.g., "" or the actual schema name) instead of undefined
so the second parameter is a string and the signature matches the
class-validator 0.14.3 API.
In `@src/lib/decorators.ts`:
- Around line 268-289: The class-validator inferred constraints are currently
applied after spreading finalOptions and thus overwrite explicit ToolProp
settings; update the merge logic in decorators.ts (around propertySchema,
finalOptions, and classValidatorProps) so that you only assign each
classValidatorProps field if the corresponding propertySchema key is undefined
(i.e., do not set classValidatorProps.minimum/maxLength/etc. when propertySchema
already has that key from finalOptions), preserving explicit ToolProp options'
precedence.
In `@src/schema-forge.spec.ts`:
- Around line 314-317: The test for classToOpenAITool(EventDto, {
forStructuredOutput: true }) currently only asserts startDate.type is
'string'—add an explicit assertion that the format field is absent to verify
that OpenAI structured output strips formats; e.g., assert that
structuredOutput.function.parameters.properties.startDate.format is undefined,
keeping other expectations (like the snapshot) intact and referencing the same
structuredOutput/startDate symbols.
- Around line 320-325: The test currently named "11 class-validator decorator
inference" is actually asserting that ToolProp options (e.g., minimum, maximum)
are passed through to the schema rather than exercising
inferClassValidatorProperties reading class-validator metadata; rename the test
to something like "ToolProp validation constraint options" and update the
comments to state it verifies manual ToolProp constraint passthrough, or
alternatively add a new test that imports class-validator decorators (`@Min`,
`@Max`, `@IsInt`) and verifies inferClassValidatorProperties extracts those into the
schema; update references to the test description string and comments in the
test block and, if adding a new test, create a separate it(...) that uses the
actual decorators and calls inferClassValidatorProperties to assert expected
schema constraints.
- Add assertion that format is undefined in structured output test - Rename test 11 to 'ToolProp validation constraint options' for clarity - Fix class-validator integration to not override explicit ToolProp options - Fix getTargetValidationMetadatas call to pass empty string instead of undefined - Bump version from 1.0.3 to 1.1.0 Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/class-validator-integration.ts`:
- Around line 156-158: The mapping for VALIDATION_TYPES.IS_POSITIVE incorrectly
sets inferred.minimum = 1, excluding valid positive decimals; change the schema
to use exclusiveMinimum: 0 instead of minimum: 1 (update the place where case
VALIDATION_TYPES.IS_POSITIVE is handled), add exclusiveMinimum to the
InferredSchemaProperties type/interface, and ensure decorators.ts reads and
emits exclusiveMinimum when building the final schema so values > 0 (e.g., 0.5)
are allowed.
- Around line 51-61: Replace the globalThis access in
getClassValidatorMetadataStorage with class-validator's public API: import
getMetadataStorage from class-validator (i.e. import { getMetadataStorage } from
"class-validator") and return getMetadataStorage() inside the try block in the
getClassValidatorMetadataStorage function, keeping the catch branch to return
null; update any references to global.classValidatorMetadataStorage accordingly.
In `@src/schema-forge.spec.ts`:
- Around line 321-325: Add an integration test that defines a sample class
decorated with class-validator decorators (e.g., class TestDto { `@Min`(1)
`@Max`(10) `@IsInt`() count: number; `@MinLength`(3) `@MaxLength`(8) name: string }),
then call inferClassValidatorProperties on that class (or trigger the code path
that uses inferClassValidatorProperties from ToolProp) and assert the resulting
schema contains the inferred constraints
(minimum/maximum/type/minLength/maxLength, etc.); place this new assertions into
the existing test case labeled "11 ToolProp validation constraint options" and
reference inferClassValidatorProperties and ToolProp to ensure the test
exercises the metadata storage path at runtime.
| function getClassValidatorMetadataStorage(): any | null { | ||
| try { | ||
| // Try to access the global class-validator metadata storage | ||
| const global = globalThis as any; | ||
| if (global.classValidatorMetadataStorage) { | ||
| return global.classValidatorMetadataStorage; | ||
| } | ||
| return null; | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file
cat -n src/lib/class-validator-integration.ts | head -70Repository: firefliesai/schema-forge
Length of output: 2146
🌐 Web query:
class-validator 0.14.x getMetadataStorage API metadata access
💡 Result:
In class-validator 0.14.x, the public entry point for reading decorator metadata is:
getMetadataStorage(): MetadataStorage(a global singleton storage) [1]
From that storage you can query what was registered by decorators via:
getTargetValidationMetadatas(targetConstructor, targetSchema, always, strictGroups, groups?) => ValidationMetadata[][2]groupByPropertyName(metadata) => Record<string, ValidationMetadata[]>[2]getTargetValidatorConstraints(target) => ConstraintMetadata[][2]
Example (read validations for a class):
import { getMetadataStorage } from "class-validator";
const storage = getMetadataStorage();
const metas = storage.getTargetValidationMetadatas(
User, // targetConstructor
"", // targetSchema (usually "" unless using schema name)
false, // always
false, // strictGroups
undefined // groups?: string[]
);
const byProp = storage.groupByPropertyName(metas);
console.log(byProp); // { email: [...], age: [...], ... }Sources: [1] [2]
🏁 Script executed:
grep -n "getMetadataStorage\|import.*class-validator" src/lib/class-validator-integration.tsRepository: firefliesai/schema-forge
Length of output: 50
🏁 Script executed:
cat -n src/lib/class-validator-integration.tsRepository: firefliesai/schema-forge
Length of output: 5358
Use the standard getMetadataStorage() import from class-validator instead of accessing globalThis.
The function attempts to access globalThis.classValidatorMetadataStorage, but class-validator 0.14.x does not expose metadata through global state. The correct approach is to import and call the public API:
import { getMetadataStorage } from "class-validator";
function getClassValidatorMetadataStorage(): any | null {
try {
return getMetadataStorage();
} catch {
return null;
}
}The current implementation will return null in all environments, causing the decorator inference to fail silently.
🤖 Prompt for AI Agents
In `@src/lib/class-validator-integration.ts` around lines 51 - 61, Replace the
globalThis access in getClassValidatorMetadataStorage with class-validator's
public API: import getMetadataStorage from class-validator (i.e. import {
getMetadataStorage } from "class-validator") and return getMetadataStorage()
inside the try block in the getClassValidatorMetadataStorage function, keeping
the catch branch to return null; update any references to
global.classValidatorMetadataStorage accordingly.
| case VALIDATION_TYPES.IS_POSITIVE: | ||
| inferred.minimum = 1; | ||
| break; |
There was a problem hiding this comment.
IsPositive maps to minimum: 1, but positive numbers include decimals > 0.
The @IsPositive() decorator validates that a number is greater than zero (i.e., value > 0), which includes values like 0.5. Mapping to minimum: 1 excludes valid positive decimals in the (0, 1) range.
Consider using exclusiveMinimum: 0 for semantic correctness, or document this limitation.
🔧 Suggested fix using exclusiveMinimum
case VALIDATION_TYPES.IS_POSITIVE:
- inferred.minimum = 1;
+ inferred.exclusiveMinimum = 0;
break;Note: This requires adding exclusiveMinimum to the InferredSchemaProperties interface and handling it in decorators.ts.
🤖 Prompt for AI Agents
In `@src/lib/class-validator-integration.ts` around lines 156 - 158, The mapping
for VALIDATION_TYPES.IS_POSITIVE incorrectly sets inferred.minimum = 1,
excluding valid positive decimals; change the schema to use exclusiveMinimum: 0
instead of minimum: 1 (update the place where case VALIDATION_TYPES.IS_POSITIVE
is handled), add exclusiveMinimum to the InferredSchemaProperties
type/interface, and ensure decorators.ts reads and emits exclusiveMinimum when
building the final schema so values > 0 (e.g., 0.5) are allowed.
| it('11 ToolProp validation constraint options', () => { | ||
| // Note: This test verifies that ToolProp correctly passes through | ||
| // validation constraint options (minimum, maximum, minLength, etc.) to the schema. | ||
| // These options can be set manually or inferred from class-validator decorators. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test with actual class-validator decorators.
While the current test validates ToolProp option passthrough, there's no integration test exercising inferClassValidatorProperties with real class-validator decorators (@Min(), @Max(), @IsInt(), etc.). This would provide confidence that the metadata storage access works correctly at runtime.
Would you like me to generate a test case that uses actual class-validator decorators to verify the inference integration?
🤖 Prompt for AI Agents
In `@src/schema-forge.spec.ts` around lines 321 - 325, Add an integration test
that defines a sample class decorated with class-validator decorators (e.g.,
class TestDto { `@Min`(1) `@Max`(10) `@IsInt`() count: number; `@MinLength`(3)
`@MaxLength`(8) name: string }), then call inferClassValidatorProperties on that
class (or trigger the code path that uses inferClassValidatorProperties from
ToolProp) and assert the resulting schema contains the inferred constraints
(minimum/maximum/type/minLength/maxLength, etc.); place this new assertions into
the existing test case labeled "11 ToolProp validation constraint options" and
reference inferClassValidatorProperties and ToolProp to ensure the test
exercises the metadata storage path at runtime.
Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
| case VALIDATION_TYPES.IS_POSITIVE: | ||
| inferred.minimum = 1; | ||
| break; |
There was a problem hiding this comment.
🟡 IsPositive decorator infers incorrect minimum constraint for floating-point numbers
The IsPositive class-validator decorator inference sets minimum: 1 but IsPositive semantically means value > 0 (strictly greater than zero).
Click to expand
Issue Details
In class-validator-integration.ts:157, when IsPositive is detected, the code sets:
case VALIDATION_TYPES.IS_POSITIVE:
inferred.minimum = 1;
break;However, looking at class-validator's implementation, IsPositive validates that value > 0 (from node_modules/class-validator/cjs/decorator/number/IsPositive.js):
function isPositive(value) {
return typeof value === 'number' && value > 0;
}Actual vs Expected
- Actual: JSON Schema uses
minimum: 1which meansvalue >= 1 - Expected: Should use
exclusiveMinimum: 0to correctly representvalue > 0
Impact
For floating-point properties decorated with @IsPositive(), valid values like 0.5, 0.001, or 0.99 would pass class-validator validation but would be rejected by the generated JSON Schema. This could cause data that passes server-side validation to be rejected by LLM function calling schemas.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Add Class-Validator Integration section to README - Document supported decorators and their JSON Schema mappings - Add examples for using class-validator with ToolProp - Bump version to 2.0.0 Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 507-517: The table in README.md is triggering MD060 due to
inconsistent column separator spacing; update the separator row (the line of
dashes under the header) so each column's dash count and surrounding spaces
align with the header and data rows (e.g., make the separator cells match the
width of "Decorator" and "JSON Schema Property" and ensure a single space
after/before each pipe), ensuring consistent pipe + space formatting across the
header, separator, and data rows to satisfy the linter.
| | Decorator | JSON Schema Property | | ||
| |-----------|---------------------| | ||
| | `@ArrayMaxSize(n)` | `maxItems: n` | | ||
| | `@ArrayMinSize(n)` | `minItems: n` | | ||
| | `@Max(n)` | `maximum: n` | | ||
| | `@Min(n)` | `minimum: n` | | ||
| | `@IsInt()` | `type: 'integer'` | | ||
| | `@MinLength(n)` | `minLength: n` | | ||
| | `@MaxLength(n)` | `maxLength: n` | | ||
| | `@IsUrl()` | `format: 'uri'` | | ||
| | `@IsPositive()` | `minimum: 1` | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor table formatting inconsistency flagged by linter.
The static analysis tool reports MD060 (table-column-style) warnings on the separator row. This is a very minor formatting issue that doesn't affect readability, but can be fixed for consistency.
📝 Optional fix for table spacing consistency
-| Decorator | JSON Schema Property |
-|-----------|---------------------|
-| `@ArrayMaxSize(n)` | `maxItems: n` |
-| `@ArrayMinSize(n)` | `minItems: n` |
-| `@Max(n)` | `maximum: n` |
-| `@Min(n)` | `minimum: n` |
-| `@IsInt()` | `type: 'integer'` |
-| `@MinLength(n)` | `minLength: n` |
-| `@MaxLength(n)` | `maxLength: n` |
-| `@IsUrl()` | `format: 'uri'` |
-| `@IsPositive()` | `minimum: 1` |
+| Decorator | JSON Schema Property |
+| ------------------ | -------------------- |
+| `@ArrayMaxSize(n)` | `maxItems: n` |
+| `@ArrayMinSize(n)` | `minItems: n` |
+| `@Max(n)` | `maximum: n` |
+| `@Min(n)` | `minimum: n` |
+| `@IsInt()` | `type: 'integer'` |
+| `@MinLength(n)` | `minLength: n` |
+| `@MaxLength(n)` | `maxLength: n` |
+| `@IsUrl()` | `format: 'uri'` |
+| `@IsPositive()` | `minimum: 1` |📝 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.
| | Decorator | JSON Schema Property | | |
| |-----------|---------------------| | |
| | `@ArrayMaxSize(n)` | `maxItems: n` | | |
| | `@ArrayMinSize(n)` | `minItems: n` | | |
| | `@Max(n)` | `maximum: n` | | |
| | `@Min(n)` | `minimum: n` | | |
| | `@IsInt()` | `type: 'integer'` | | |
| | `@MinLength(n)` | `minLength: n` | | |
| | `@MaxLength(n)` | `maxLength: n` | | |
| | `@IsUrl()` | `format: 'uri'` | | |
| | `@IsPositive()` | `minimum: 1` | | |
| | Decorator | JSON Schema Property | | |
| | ------------------ | -------------------- | | |
| | `@ArrayMaxSize(n)` | `maxItems: n` | | |
| | `@ArrayMinSize(n)` | `minItems: n` | | |
| | `@Max(n)` | `maximum: n` | | |
| | `@Min(n)` | `minimum: n` | | |
| | `@IsInt()` | `type: 'integer'` | | |
| | `@MinLength(n)` | `minLength: n` | | |
| | `@MaxLength(n)` | `maxLength: n` | | |
| | `@IsUrl()` | `format: 'uri'` | | |
| | `@IsPositive()` | `minimum: 1` | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
508-508: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
508-508: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
508-508: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
508-508: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@README.md` around lines 507 - 517, The table in README.md is triggering MD060
due to inconsistent column separator spacing; update the separator row (the line
of dashes under the header) so each column's dash count and surrounding spaces
align with the header and data rows (e.g., make the separator cells match the
width of "Decorator" and "JSON Schema Property" and ensure a single space
after/before each pipe), ensuring consistent pipe + space formatting across the
header, separator, and data rows to satisfy the linter.
- ArrayUnique -> uniqueItems: true - ArrayNotEmpty -> minItems: 1 - IsEmail -> format: 'email' Updated README with new decorators documentation. Co-Authored-By: greg@fireflies.ai <greg.teixeira123@gmail.com>
| case VALIDATION_TYPES.IS_POSITIVE: | ||
| inferred.minimum = 1; | ||
| break; |
There was a problem hiding this comment.
🟡 @ispositive() unconditionally overwrites @min() constraint value
When both @IsPositive() and @Min(n) decorators are applied to the same property, @IsPositive() always sets minimum: 1, which can overwrite a more restrictive @Min(n) value.
Click to expand
How it happens
The inferClassValidatorProperties function processes class-validator metadata in order, and each decorator simply assigns its value without checking if a more restrictive constraint already exists:
case VALIDATION_TYPES.MIN:
if (meta.constraints && meta.constraints[0] !== undefined) {
inferred.minimum = meta.constraints[0];
}
break;
// ...
case VALIDATION_TYPES.IS_POSITIVE:
inferred.minimum = 1;
break;If a user applies @Min(5) and @IsPositive() to the same property, and isPositive is processed after min, the minimum becomes 1 instead of 5.
Actual vs Expected
- Actual:
@Min(5) @IsPositive()produces{ minimum: 1 } - Expected: Should produce
{ minimum: 5 }(the more restrictive value)
Impact
The generated JSON Schema will have a less restrictive constraint than intended, potentially allowing invalid values that should be rejected.
The same issue exists for @ArrayNotEmpty() (sets minItems: 1) overwriting @ArrayMinSize(n).
Recommendation: Compare with existing value before overwriting: if (inferred.minimum === undefined || inferred.minimum < 1) { inferred.minimum = 1; }. Apply similar logic for @ArrayNotEmpty() at line 136.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
What does this PR do?
Type of change
This pull request is a
Which introduces
How should this be manually tested?
Any background context you want to provide beyond Shortcut?
Devin session link
Shortcut story
Requested by: Greg Teixeira (greg@fireflies.ai) @greguintow
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.