Feat 3179 schema omit pick #3229
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Move and consolidate development guidelines into `AGENTS.md` at the repository root. - Simplify structure and commands, making `AGENTS.md` the single source of truth for workflows and processes. - Remove outdated `.junie/guidelines.md
…ers for JsonSchema composition - Introduce `.pick()` and `.omit()` methods to streamline property selection and exclusion in schemas. - Add `.merge()` to combine schema fragments, including union of required fields. - Extend TypeScript inference (`s.infer()`) to support new helpers for accurate transformed shapes. - Update documentation in Functional API to include usage examples for `.pick`, `.omit`, and `.merge`. - Add runtime and typing tests to validate behavior and ensure schema immutability.
…chema - Add `.pick()` and `.omit()` methods for schema property selection and exclusion. - Implement `.merge()` to combine schema definitions with union of required fields. - Enhance TypeScript inference (`s.infer()`) to support new schema transformation helpers. - Extend runtime and typing tests to validate correct behavior and schema immutability. - Update Functional API documentation with examples for the new methods. Closes: #3179
Contributor
WalkthroughAdds JsonSchema object-transforms (pick, omit, partial, merge) with runtime tests and TypeScript typing utilities, and replaces a guidelines stub with a new AGENTS.md that centralizes development workflows and tooling. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.junie/guidelines.mdAGENTS.mddocs/docs/model.mdopenspec/changes/add-jsonschema-object-transforms/design.mdopenspec/changes/add-jsonschema-object-transforms/proposal.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdopenspec/changes/add-jsonschema-object-transforms/tasks.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tspackages/specs/schema/src/fn/typing.spec.ts
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/{models,controllers}/**/*.ts : Keep serialization/validation consistent using Json Mapper and class annotations
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/nextMapper.ts:35-42
Timestamp: 2025-07-21T11:58:48.206Z
Learning: In the JsonSchema mapper context within `packages/specs/schema/src/components/default/nextMapper.ts`, when `schema1.canRef` is true, `options.components.schemas` is guaranteed to be defined, making non-null assertions safe to use on `options.components!.schemas`.
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/domain/JsonSchema.ts:1045-1054
Timestamp: 2024-11-16T08:52:24.617Z
Learning: When constructing discriminator mappings in `packages/specs/schema/src/domain/JsonSchema.ts`, it's necessary to assign the schema object to the mapping (i.e., `acc[key] = schema;`) instead of a schema reference, because the schema is needed to resolve the correct reference later based on given group options to the `getSchema` function.
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/domain/JsonSchema.ts:37-37
Timestamp: 2025-07-21T09:00:00.253Z
Learning: In Ts.ED's JsonSchema implementation, the JsonSchemaObject interface intentionally uses wrapper constructor classes (String, Number, Boolean, Object) rather than primitive types (string, number, boolean, object) in type definitions. This is because the schema system needs to reference the actual constructor classes for proper type handling and generics support, not just primitive types.
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Use the Ts.ED CLI (tsed/cli) to scaffold controllers, services, DTOs, middlewares, interceptors, and tests to ensure files, names, and boilerplate match current best practices
Applied to files:
.junie/guidelines.mdAGENTS.md
📚 Learning: 2025-03-22T17:11:05.964Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3003
File: packages/platform/platform-multer/src/common/decorators/multerOptions.ts:40-40
Timestamp: 2025-03-22T17:11:05.964Z
Learning: In the Ts.ED project, documentation links in JSDoc comments that start with `/docs/` are correct as is because vitepress resolves links relatively from the `/docs` directory where the package.json is located, not from the repository root.
Applied to files:
.junie/guidelines.mdAGENTS.md
📚 Learning: 2024-11-16T08:52:24.617Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/domain/JsonSchema.ts:1045-1054
Timestamp: 2024-11-16T08:52:24.617Z
Learning: When constructing discriminator mappings in `packages/specs/schema/src/domain/JsonSchema.ts`, it's necessary to assign the schema object to the mapping (i.e., `acc[key] = schema;`) instead of a schema reference, because the schema is needed to resolve the correct reference later based on given group options to the `getSchema` function.
Applied to files:
openspec/changes/add-jsonschema-object-transforms/proposal.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/design.mdpackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-07-21T11:58:48.206Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/nextMapper.ts:35-42
Timestamp: 2025-07-21T11:58:48.206Z
Learning: In the JsonSchema mapper context within `packages/specs/schema/src/components/default/nextMapper.ts`, when `schema1.canRef` is true, `options.components.schemas` is guaranteed to be defined, making non-null assertions safe to use on `options.components!.schemas`.
Applied to files:
openspec/changes/add-jsonschema-object-transforms/proposal.mdpackages/specs/schema/src/domain/JsonSchema.spec.tsdocs/docs/model.mdpackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdpackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/**/*.spec.ts : Update or add unit/integration tests near changed files (test/ or src/**/**.spec.ts co-location)
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tsAGENTS.mdopenspec/changes/add-jsonschema-object-transforms/tasks.mdpackages/specs/schema/src/fn/typing.spec.ts
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/{models,controllers}/**/*.ts : Keep serialization/validation consistent using Json Mapper and class annotations
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tsdocs/docs/model.mdpackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.mdpackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2024-11-16T09:07:53.545Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/types.tspackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2024-11-16T08:52:26.301Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/utils/ref.ts:59-61
Timestamp: 2024-11-16T08:52:26.301Z
Learning: In the `toRef` function of `packages/specs/schema/src/utils/ref.ts`, the `schema` parameter is always an object when it's not nullish.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/types.tspackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-07-21T09:00:00.253Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/domain/JsonSchema.ts:37-37
Timestamp: 2025-07-21T09:00:00.253Z
Learning: In Ts.ED's JsonSchema implementation, the JsonSchemaObject interface intentionally uses wrapper constructor classes (String, Number, Boolean, Object) rather than primitive types (string, number, boolean, object) in type definitions. This is because the schema system needs to reference the actual constructor classes for proper type handling and generics support, not just primitive types.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/types.tspackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/**/*.ts : Always use Ts.ED decorators and DI patterns (Controller, Injectable, Inject, UseBefore, UseAfter, PathParams, BodyParams, QueryParams)
Applied to files:
AGENTS.md
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/controllers/**/*.ts : When adding routes, include types, status codes, and example responses; update OpenAPI annotations when applicable
Applied to files:
AGENTS.mddocs/docs/model.md
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/models/**/*.ts : Ensure DTOs are classes with explicit types; avoid loose 'any' types
Applied to files:
docs/docs/model.mdpackages/specs/schema/src/domain/types.ts
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/models/**/*.ts : Create DTOs with validation decorators (Required, MinLength, Email, etc.)
Applied to files:
docs/docs/model.md
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/models/**/*.ts : Use Model/DTOs and validation decorators to protect your routes
Applied to files:
docs/docs/model.md
📚 Learning: 2025-07-21T08:57:11.179Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/domain/JsonMedia.ts:35-41
Timestamp: 2025-07-21T08:57:11.179Z
Learning: In Ts.ED's JsonSchema implementation, the `itemSchema()` method is designed to create references to nested schemas for multiple scenarios: arrays, additionalProperties, and single models/classes. When used with class types in `JsonMedia.type()`, it correctly creates the appropriate schema reference rather than being limited to just array items.
Applied to files:
docs/docs/model.mdpackages/specs/schema/src/domain/types.tspackages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-07-21T08:45:38.058Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/genericsMapper.ts:39-48
Timestamp: 2025-07-21T08:45:38.058Z
Learning: In the JsonSchema implementation within `packages/specs/schema/`, when `.has(key)` returns true, it guarantees that `.get(key)` will return a non-null value. This makes the non-null assertion operator (!) safe to use after a `.has()` check, as it's part of the designed contract between these methods.
Applied to files:
openspec/changes/add-jsonschema-object-transforms/tasks.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.md
📚 Learning: 2025-01-24T08:24:51.539Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2971
File: packages/orm/mongoose/src/utils/createSchema.ts:168-168
Timestamp: 2025-01-24T08:24:51.539Z
Learning: In Ts.ED's JsonSchema implementation, when the `enum` property is truthy, it is guaranteed to be a non-empty array of values, following the JSON Schema specification.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-05-12T11:30:58.388Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3076
File: packages/specs/schema/src/domain/JsonParameterStore.ts:113-121
Timestamp: 2025-05-12T11:30:58.388Z
Learning: In the JsonParameter class, the `itemSchema()` method intelligently handles both collection and primitive types by checking `$schema.isCollection`. For collections, it sets/gets the item schema; for primitives, it delegates to the `schema()` method. This avoids setting unnecessary `items` properties on non-collection schemas.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
🧬 Code graph analysis (2)
packages/specs/schema/src/fn/typing.spec.ts (3)
docs/docs/snippets/model/fn-api-infer.ts (1)
UserSchema(16-23)packages/specs/schema/src/fn/index.ts (2)
s(24-47)infer(52-52)packages/specs/schema/src/domain/JsonSchema.ts (1)
partial(1146-1167)
packages/specs/schema/src/domain/JsonSchema.ts (1)
packages/specs/schema/src/domain/types.ts (6)
SchemaKey(93-93)SchemaPick(65-69)SchemaOmit(71-75)SchemaPartial(77-81)SchemaMerge(83-89)Infer(20-20)
🪛 LanguageTool
openspec/changes/add-jsonschema-object-transforms/proposal.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rence reflecting the partial shape). 3. Provide a .merge() helper that combines multi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/add-jsonschema-object-transforms/tasks.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: CodeQL-Build
- GitHub Check: test-third-parties (20.19.4)
- GitHub Check: test-specs (20.19.4)
- GitHub Check: test-platform (20.19.4)
- GitHub Check: test-security (20.19.4)
- GitHub Check: lint (20.19.4)
- GitHub Check: test-orm (20.19.4)
- GitHub Check: test-core (20.19.4)
- GitHub Check: test-graphql (20.19.4)
🔇 Additional comments (15)
.junie/guidelines.md (1)
3-3: LGTM! Clear consolidation strategy.The consolidation of guidelines into a single source of truth (AGENTS.md) is a good practice that reduces maintenance overhead and prevents documentation drift.
docs/docs/model.md (1)
1882-1919: Excellent documentation for the new composition helpers.The new "Composing object schemas" section clearly demonstrates the fluent API for derived object modeling. The TypeScript examples effectively show both usage patterns and resulting inferred types, which will help users understand the type-safety guarantees.
AGENTS.md (1)
1-110: Well-structured development guidelines document.This comprehensive document successfully consolidates development workflows, commands, and expectations into a single authoritative reference. The organization is clear, commands are practical, and the coverage of testing, linting, documentation, and release processes provides excellent guidance for contributors.
openspec/changes/add-jsonschema-object-transforms/proposal.md (1)
1-45: Well-structured proposal with clear goals and scope.The proposal effectively outlines the problem statement, goals, approach, and success criteria for the object transform helpers. The risks section appropriately identifies key concerns around regex keys and required field management.
packages/specs/schema/src/fn/typing.spec.ts (1)
178-255: Comprehensive type inference tests for object transform helpers.The new tests effectively validate that
s.infer()produces the correct TypeScript types forpick,omit,partial, andmergeoperations. The tests cover important edge cases including:
- Required/optional field preservation in pick/omit
- Nullable field handling
- Full optionality after partial()
- Property and required field merging
This ensures type safety guarantees align with runtime behavior.
openspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md (1)
1-26: Clear specification for merge behavior.The spec clearly defines the merge() method's behavior, including the union of property maps, handling of key conflicts, required set merging, and type inference guarantee. The BDD-style scenarios make the requirements easy to understand and test.
openspec/changes/add-jsonschema-object-transforms/design.md (1)
1-72: Thorough design document with clear implementation guidance.The design effectively addresses:
- Immutability guarantees through cloning semantics
- Step-by-step implementation details for each helper
- Type operation mappings to TypeScript utility types
- Comprehensive testing strategy covering runtime and type inference
- Open questions acknowledging future considerations
The design provides clear guidance for implementation while maintaining alignment with TypeScript's type system.
packages/specs/schema/src/domain/JsonSchema.spec.ts (1)
205-364: Comprehensive runtime tests for object transform helpers.The new test suite thoroughly validates the object transform API:
- Immutability: Verifies that transformations return new instances without mutating originals (line 220-221)
- pick(): Tests property subset selection with correct required field propagation
- omit(): Tests property exclusion
- partial(): Verifies all properties become optional and required array is cleared
- merge(): Tests schema combination including nested objects and required field merging
The use of inline snapshots makes expected JSON Schema outputs explicit and maintainable.
openspec/changes/add-jsonschema-object-transforms/tasks.md (1)
1-8: LGTM! All tasks completed.The implementation is complete with all items checked off, including runtime helpers, type utilities, tests, and documentation.
packages/specs/schema/src/domain/JsonSchema.ts (4)
35-35: LGTM! Import statement properly updated.The new type utilities are correctly imported to support type inference for the object transformation methods.
1103-1122: LGTM! The pick() implementation is correct.The method properly:
- Creates a clone to ensure immutability
- Filters properties to only include picked keys
- Clones each property value to prevent shared references
- Updates the required set to only include picked keys
1146-1167: LGTM! The partial() implementation is correct.The method properly:
- Clones the schema to maintain immutability
- Makes all required properties optional using
required(false)(which itself creates clones)- Clears the required set
1606-1654: LGTM! Deep cloning logic properly implemented.The enhanced
assign()method now performs deep cloning when merging from another JsonSchema instance:
- Properties are cloned to prevent shared references
- Items are cloned when not nullable
- AdditionalProperties are cloned
- Local schema's itemSchema is cloned
This ensures immutability for the new object transformation methods.
packages/specs/schema/src/domain/types.ts (2)
49-63: LGTM! Private utility types are well-designed.The utility types correctly:
- Extract object and non-object portions from union types
- Apply transformations only to the object portion
- Handle merge logic with proper intersection types
These provide a solid foundation for the public type aliases.
65-93: LGTM! Public type aliases are correctly implemented.The exported types properly handle:
- Union types containing both object and non-object portions
- Edge cases where the type is purely object or non-object
- Key extraction with fallback to string for flexible APIs
- Merge semantics with intersection for objects and union for non-objects
These will ensure accurate type inference for the new JsonSchema methods.
| @@ -0,0 +1,45 @@ | |||
| ## ADDED Requirements | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a top-level heading.
The file should begin with a level-1 heading to follow markdown best practices and improve document structure.
📝 Suggested addition
+# JsonSchema Object Transformations Specification
+
## ADDED RequirementsBased on static analysis hints.
📝 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.
Suggested change
| ## ADDED Requirements | |
| # JsonSchema Object Transformations Specification | |
| ## ADDED Requirements |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
In
openspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.md
around lines 1 to 1, the file lacks a top-level (level-1) heading; add a concise
H1 at the very top (e.g., "# Object Projections" or an appropriate title for
this spec) so the document starts with a level-1 heading to follow markdown best
practices and improve structure.
Comment on lines
+1124
to
+1144
| omit<K extends SchemaKey<T>>(...keys: K[]): JsonSchema<SchemaOmit<T, K>> { | ||
| const keySet = new Set(keys.map((key) => toJsonRegex(key))); | ||
| const schema = this.clone() as JsonSchema<SchemaOmit<T, K>>; | ||
|
|
||
| const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => { | ||
| if (!keySet.has(key)) { | ||
| return { | ||
| ...properties, | ||
| [key]: value | ||
| }; | ||
| } | ||
|
|
||
| return properties; | ||
| }, {}); | ||
|
|
||
| schema.set("properties", properties); | ||
|
|
||
| keySet.forEach((key) => schema.#required.delete(key)); | ||
|
|
||
| return schema; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider cloning property values for consistency.
The omit() method doesn't clone property values (line 1132), while pick() does (line 1111). This inconsistency could lead to shared references between the original schema and the result of omit().
🔎 Suggested fix for consistency
const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => {
if (!keySet.has(key)) {
return {
...properties,
- [key]: value
+ [key]: value.clone()
};
}
return properties;
}, {});🤖 Prompt for AI Agents
In packages/specs/schema/src/domain/JsonSchema.ts around lines 1124 to 1144, the
omit() implementation copies property references instead of cloning them (unlike
pick()), which can produce shared mutable state; when building the new
properties object, call the property's clone method (e.g., use value.clone())
before assigning it to the new properties map (and guard for undefined/null if
necessary) so the returned schema contains cloned property values consistent
with pick().
Collaborator
Author
There was a problem hiding this comment.
this.clone() clone recursively the values. we don't need to clone again.
Contributor
There was a problem hiding this comment.
@Romakita, you're absolutely right! Looking at the assign() method (lines 1826-1834), when this.clone() is called, it already performs deep cloning of all properties through the assign() logic. The properties returned by schema.get("properties") in omit() are already cloned instances, so there's no need to clone them again.
In fact, the inconsistency is that pick() is performing redundant cloning by calling value.clone() again on line 1111, not that omit() is missing a clone. Both methods are functionally correct, though pick() does unnecessary work. Thank you for the clarification!
🧠 Learnings used
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/genericsMapper.ts:39-48
Timestamp: 2025-07-21T08:45:38.058Z
Learning: In the JsonSchema implementation within `packages/specs/schema/`, when `.has(key)` returns true, it guarantees that `.get(key)` will return a non-null value. This makes the non-null assertion operator (!) safe to use after a `.has()` check, as it's part of the designed contract between these methods.
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/domain/JsonSchema.ts:1045-1054
Timestamp: 2024-11-16T08:52:24.617Z
Learning: When constructing discriminator mappings in `packages/specs/schema/src/domain/JsonSchema.ts`, it's necessary to assign the schema object to the mapping (i.e., `acc[key] = schema;`) instead of a schema reference, because the schema is needed to resolve the correct reference later based on given group options to the `getSchema` function.
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/nextMapper.ts:35-42
Timestamp: 2025-07-21T11:58:48.206Z
Learning: In the JsonSchema mapper context within `packages/specs/schema/src/components/default/nextMapper.ts`, when `schema1.canRef` is true, `options.components.schemas` is guaranteed to be defined, making non-null assertions safe to use on `options.components!.schemas`.
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/{models,controllers}/**/*.ts : Keep serialization/validation consistent using Json Mapper and class annotations
- Implement `.partial()` to mark all properties in a schema as optional without modifying the original schema. - Add `SchemaPartial<T>` typing to align `s.infer()` results with the partial shape. - Update Functional API documentation with examples illustrating `.partial()` usage. - Extend runtime and typing tests to validate `.partial()` behavior and maintain schema immutability. Closes: #3179
Romakita
force-pushed
the
feat-3179-schema-omit-pick
branch
from
3432578 to
aff1fbf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
openspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.md (1)
1-1: Consider adding a top-level heading.The file should begin with a level-1 heading to follow markdown best practices and improve document structure.
openspec/changes/add-jsonschema-object-transforms/proposal.md (1)
15-17: Minor style improvement: vary sentence beginnings.Three successive sentences begin with "Provide". Consider varying the sentence structure for better readability.
openspec/changes/add-jsonschema-object-transforms/tasks.md (1)
1-1: Consider adding a top-level heading.Following markdown conventions, the file should start with a level-1 heading.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/docs/model.mdopenspec/changes/add-jsonschema-object-transforms/proposal.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdopenspec/changes/add-jsonschema-object-transforms/tasks.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tspackages/specs/schema/src/fn/typing.spec.ts
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/{models,controllers}/**/*.ts : Keep serialization/validation consistent using Json Mapper and class annotations
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/nextMapper.ts:35-42
Timestamp: 2025-07-21T11:58:48.206Z
Learning: In the JsonSchema mapper context within `packages/specs/schema/src/components/default/nextMapper.ts`, when `schema1.canRef` is true, `options.components.schemas` is guaranteed to be defined, making non-null assertions safe to use on `options.components!.schemas`.
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/models/**/*.ts : Ensure DTOs are classes with explicit types; avoid loose 'any' types
Applied to files:
docs/docs/model.mdpackages/specs/schema/src/domain/types.ts
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/{models,controllers}/**/*.ts : Keep serialization/validation consistent using Json Mapper and class annotations
Applied to files:
docs/docs/model.mdopenspec/changes/add-jsonschema-object-transforms/proposal.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.md
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/controllers/**/*.ts : When adding routes, include types, status codes, and example responses; update OpenAPI annotations when applicable
Applied to files:
docs/docs/model.md
📚 Learning: 2025-07-21T08:57:11.179Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/domain/JsonMedia.ts:35-41
Timestamp: 2025-07-21T08:57:11.179Z
Learning: In Ts.ED's JsonSchema implementation, the `itemSchema()` method is designed to create references to nested schemas for multiple scenarios: arrays, additionalProperties, and single models/classes. When used with class types in `JsonMedia.type()`, it correctly creates the appropriate schema reference rather than being limited to just array items.
Applied to files:
docs/docs/model.mdpackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.ts
📚 Learning: 2025-07-21T11:58:48.206Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/nextMapper.ts:35-42
Timestamp: 2025-07-21T11:58:48.206Z
Learning: In the JsonSchema mapper context within `packages/specs/schema/src/components/default/nextMapper.ts`, when `schema1.canRef` is true, `options.components.schemas` is guaranteed to be defined, making non-null assertions safe to use on `options.components!.schemas`.
Applied to files:
docs/docs/model.mdopenspec/changes/add-jsonschema-object-transforms/proposal.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md
📚 Learning: 2024-11-16T08:52:24.617Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/domain/JsonSchema.ts:1045-1054
Timestamp: 2024-11-16T08:52:24.617Z
Learning: When constructing discriminator mappings in `packages/specs/schema/src/domain/JsonSchema.ts`, it's necessary to assign the schema object to the mapping (i.e., `acc[key] = schema;`) instead of a schema reference, because the schema is needed to resolve the correct reference later based on given group options to the `getSchema` function.
Applied to files:
openspec/changes/add-jsonschema-object-transforms/proposal.mdpackages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.mdopenspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md
📚 Learning: 2025-12-01T15:26:21.293Z
Learnt from: CR
Repo: tsedio/tsed PR: 0
File: docs/public/ai/AGENTS.md:0-0
Timestamp: 2025-12-01T15:26:21.293Z
Learning: Applies to docs/public/ai/src/**/*.spec.ts : Update or add unit/integration tests near changed files (test/ or src/**/**.spec.ts co-location)
Applied to files:
packages/specs/schema/src/fn/typing.spec.tsopenspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdpackages/specs/schema/src/domain/JsonSchema.spec.tsopenspec/changes/add-jsonschema-object-transforms/tasks.md
📚 Learning: 2025-01-29T23:19:32.209Z
Learnt from: LonguCodes
Repo: tsedio/tsed PR: 2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:86-111
Timestamp: 2025-01-29T23:19:32.209Z
Learning: The test suite for `BelongsToOne` decorator in `belongsToOne.spec.ts` focuses on positive cases of relationship mapping configuration. Error cases are not applicable to this test suite as they are likely handled at TypeScript compilation time or in separate validation layers.
Applied to files:
packages/specs/schema/src/fn/typing.spec.ts
📚 Learning: 2024-11-17T08:54:37.569Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2890
File: packages/platform/platform-router/test/routers-injection.integration.spec.ts:36-38
Timestamp: 2024-11-17T08:54:37.569Z
Learning: In TypeScript unit test files in the Ts.ED framework, it is acceptable to use non-null assertions (`!`), so we should not suggest replacing them with explicit checks.
Applied to files:
packages/specs/schema/src/fn/typing.spec.ts
📚 Learning: 2025-07-21T08:45:38.058Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/components/default/genericsMapper.ts:39-48
Timestamp: 2025-07-21T08:45:38.058Z
Learning: In the JsonSchema implementation within `packages/specs/schema/`, when `.has(key)` returns true, it guarantees that `.get(key)` will return a non-null value. This makes the non-null assertion operator (!) safe to use after a `.has()` check, as it's part of the designed contract between these methods.
Applied to files:
openspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.mdopenspec/changes/add-jsonschema-object-transforms/tasks.md
📚 Learning: 2024-11-16T09:07:53.545Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.tsopenspec/changes/add-jsonschema-object-transforms/tasks.md
📚 Learning: 2024-11-16T08:52:26.301Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/specs/schema/src/utils/ref.ts:59-61
Timestamp: 2024-11-16T08:52:26.301Z
Learning: In the `toRef` function of `packages/specs/schema/src/utils/ref.ts`, the `schema` parameter is always an object when it's not nullish.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.ts
📚 Learning: 2025-07-21T09:00:00.253Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/domain/JsonSchema.ts:37-37
Timestamp: 2025-07-21T09:00:00.253Z
Learning: In Ts.ED's JsonSchema implementation, the JsonSchemaObject interface intentionally uses wrapper constructor classes (String, Number, Boolean, Object) rather than primitive types (string, number, boolean, object) in type definitions. This is because the schema system needs to reference the actual constructor classes for proper type handling and generics support, not just primitive types.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.spec.tspackages/specs/schema/src/domain/JsonSchema.tspackages/specs/schema/src/domain/types.ts
📚 Learning: 2024-11-16T15:43:27.145Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/core/src/utils/objects/cleanObject.ts:9-9
Timestamp: 2024-11-16T15:43:27.145Z
Learning: In the `cleanObject` function in `packages/core/src/utils/objects/cleanObject.ts`, `null` is considered a valid value (like `0` or `false`) and should not be filtered out.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2024-11-16T17:53:10.160Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2891
File: packages/core/src/utils/objects/deepMerge.ts:67-68
Timestamp: 2024-11-16T17:53:10.160Z
Learning: In `packages/core/src/utils/objects/deepMerge.ts`, the `createInstance(source)` function returns a fresh instance and does not mutate `source` or `obj` in the `deepMerge` function. Therefore, using `createInstance(source)` does not introduce mutability.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-07-21T08:55:04.910Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3124
File: packages/specs/schema/src/decorators/common/groups.ts:44-53
Timestamp: 2025-07-21T08:55:04.910Z
Learning: In TypeScript files within the Ts.ED framework (tsed/tsed repository), Romakita prefers to reassign function parameters when needed for logic flow, even when static analysis tools like Biome flag it with lint/style/noParameterAssign. This is an accepted coding style choice in the codebase.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-01-24T08:24:51.539Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 2971
File: packages/orm/mongoose/src/utils/createSchema.ts:168-168
Timestamp: 2025-01-24T08:24:51.539Z
Learning: In Ts.ED's JsonSchema implementation, when the `enum` property is truthy, it is guaranteed to be a non-empty array of values, following the JSON Schema specification.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
📚 Learning: 2025-05-12T11:30:58.388Z
Learnt from: Romakita
Repo: tsedio/tsed PR: 3076
File: packages/specs/schema/src/domain/JsonParameterStore.ts:113-121
Timestamp: 2025-05-12T11:30:58.388Z
Learning: In the JsonParameter class, the `itemSchema()` method intelligently handles both collection and primitive types by checking `$schema.isCollection`. For collections, it sets/gets the item schema; for primitives, it delegates to the `schema()` method. This avoids setting unnecessary `items` properties on non-collection schemas.
Applied to files:
packages/specs/schema/src/domain/JsonSchema.ts
🧬 Code graph analysis (2)
packages/specs/schema/src/domain/JsonSchema.spec.ts (1)
packages/specs/schema/src/domain/JsonSchema.ts (2)
JsonSchema(129-1890)partial(1146-1167)
packages/specs/schema/src/domain/JsonSchema.ts (1)
packages/specs/schema/src/domain/types.ts (6)
SchemaKey(93-93)SchemaPick(65-69)SchemaOmit(71-75)SchemaPartial(77-81)SchemaMerge(83-89)Infer(20-20)
🪛 LanguageTool
openspec/changes/add-jsonschema-object-transforms/proposal.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rence reflecting the partial shape). 3. Provide a .merge() helper that combines multi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/add-jsonschema-object-transforms/specs/object-projections/spec.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/add-jsonschema-object-transforms/tasks.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: lint (20.19.4)
- GitHub Check: test-security (20.19.4)
- GitHub Check: test-third-parties (20.19.4)
- GitHub Check: test-core (20.19.4)
- GitHub Check: test-platform (20.19.4)
- GitHub Check: test-orm (20.19.4)
- GitHub Check: test-graphql (20.19.4)
- GitHub Check: test-specs (20.19.4)
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
docs/docs/model.md (1)
1882-1919: LGTM! Clear and comprehensive documentation.The new "Composing object schemas" section provides excellent coverage of the
pick(),omit(),partial(), andmerge()helpers with accurate type inference examples. The code sample effectively demonstrates all four helpers and the corresponding inferred shapes.packages/specs/schema/src/fn/typing.spec.ts (3)
178-207: LGTM! Comprehensive type inference tests for pick/omit.The tests effectively validate that
pick()andomit()produce correct TypeScript types vias.infer<>, with proper handling of required vs optional fields.
208-224: LGTM! Correct partial helper type inference.The test verifies that
partial()makes all properties optional with| undefined, matching the expectedSchemaPartial<T>behavior.
225-255: LGTM! Thorough merge helper type tests.The tests validate merged schema type inference including nested objects, correctly checking that merged properties from both schemas are present with appropriate optionality.
openspec/changes/add-jsonschema-object-transforms/proposal.md (1)
1-45: LGTM! Well-structured proposal document.The proposal clearly outlines the problem, goals, approach, risks, and success measures for the new object transform helpers. The documented scope is appropriate and aligns with the implementation.
packages/specs/schema/src/domain/types.ts (1)
49-93: LGTM! Well-designed type utilities.The conditional type definitions correctly handle edge cases including
nevertypes and non-object portions. The[X] extends [never]pattern is properly used to distinguish empty types, and the merge logic appropriately combines object and non-object portions.packages/specs/schema/src/domain/JsonSchema.ts (4)
35-35: LGTM! Import updated correctly.The import statement now includes all necessary type utilities for the new object transform methods.
1146-1167: LGTM! Defensive handling of properties.The
partial()implementation correctly guards against missing properties with theif (properties)check, and properly clears the required set while marking each property as optional.
1169-1180: LGTM! Merge implementation is correct.The merge correctly combines properties with override semantics (later definitions win) and unions the required sets from both schemas.
1606-1654: LGTM! Enhanced deep cloning in assign().The improved cloning logic ensures properties, items, and additionalProperties are properly deep-cloned when assigning from another JsonSchema, maintaining immutability guarantees for the new object transform methods.
openspec/changes/add-jsonschema-object-transforms/specs/object-merging/spec.md
Show resolved
Hide resolved
Comment on lines
+205
to
+364
| describe("object transforms", () => { | ||
| const buildUserSchema = () => { | ||
| return JsonSchema.from({type: Object}).properties({ | ||
| id: JsonSchema.from({type: String}).required(true), | ||
| email: JsonSchema.from({type: String}), | ||
| admin: JsonSchema.from({type: Boolean}) | ||
| }); | ||
| }; | ||
|
|
||
| it("should pick a subset of properties", () => { | ||
| const schema = buildUserSchema(); | ||
|
|
||
| const picked = schema.pick("id", "email"); | ||
| const output = picked.toJSON(); | ||
|
|
||
| expect(picked).not.toBe(schema); | ||
| expect(schema.get("properties")["id"]).not.toBe(picked.get("properties")["id"]); | ||
| expect(output).toMatchInlineSnapshot(` | ||
| { | ||
| "properties": { | ||
| "email": { | ||
| "type": "string", | ||
| }, | ||
| "id": { | ||
| "minLength": 1, | ||
| "type": "string", | ||
| }, | ||
| }, | ||
| "required": [ | ||
| "id", | ||
| ], | ||
| "type": "object", | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| it("should omit properties", () => { | ||
| const schema = buildUserSchema(); | ||
|
|
||
| const omitted = schema.omit("id", "email"); | ||
| const output = omitted.toJSON(); | ||
|
|
||
| expect(output).toMatchInlineSnapshot(` | ||
| { | ||
| "properties": { | ||
| "admin": { | ||
| "type": "boolean", | ||
| }, | ||
| }, | ||
| "type": "object", | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| it("should mark all properties optional via partial()", () => { | ||
| const schema = buildUserSchema(); | ||
|
|
||
| const partial = schema.partial(); | ||
| const output = partial.toJSON(); | ||
|
|
||
| expect(partial).not.toBe(schema); | ||
| expect(partial.get("properties")["id"]).not.toBe(schema.get("properties")["id"]); | ||
| expect(schema.toJSON().required).toEqual(["id"]); | ||
| expect(partial.getRequiredFields()).toEqual([]); | ||
| expect(output.required).toBeUndefined(); | ||
| expect(output).toMatchInlineSnapshot(` | ||
| { | ||
| "properties": { | ||
| "admin": { | ||
| "type": "boolean", | ||
| }, | ||
| "email": { | ||
| "type": "string", | ||
| }, | ||
| "id": { | ||
| "minLength": 1, | ||
| "type": "string", | ||
| }, | ||
| }, | ||
| "type": "object", | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| it("should merge schemas", () => { | ||
| const user = buildUserSchema(); | ||
| const audit = JsonSchema.from({type: Object}).properties({ | ||
| createdAt: JsonSchema.from({type: Date}).required(), | ||
| updatedAt: JsonSchema.from({type: Date}) | ||
| }); | ||
|
|
||
| const mergedSchema = user.merge(audit); | ||
|
|
||
| expect(mergedSchema.toJSON()).toMatchInlineSnapshot(` | ||
| { | ||
| "properties": { | ||
| "admin": { | ||
| "type": "boolean", | ||
| }, | ||
| "createdAt": { | ||
| "minLength": 1, | ||
| "type": "string", | ||
| }, | ||
| "email": { | ||
| "type": "string", | ||
| }, | ||
| "id": { | ||
| "minLength": 1, | ||
| "type": "string", | ||
| }, | ||
| "updatedAt": { | ||
| "type": "string", | ||
| }, | ||
| }, | ||
| "required": [ | ||
| "id", | ||
| "createdAt", | ||
| ], | ||
| "type": "object", | ||
| } | ||
| `); | ||
|
|
||
| const flags = JsonSchema.from({type: Object}).properties({ | ||
| flags: JsonSchema.from({type: Object}).properties({ | ||
| active: JsonSchema.from({type: Boolean}) | ||
| }) | ||
| }); | ||
|
|
||
| const withFlags = user.merge(flags).toJSON(); | ||
|
|
||
| expect(withFlags).toMatchInlineSnapshot(` | ||
| { | ||
| "properties": { | ||
| "admin": { | ||
| "type": "boolean", | ||
| }, | ||
| "email": { | ||
| "type": "string", | ||
| }, | ||
| "flags": { | ||
| "properties": { | ||
| "active": { | ||
| "type": "boolean", | ||
| }, | ||
| }, | ||
| "type": "object", | ||
| }, | ||
| "id": { | ||
| "minLength": 1, | ||
| "type": "string", | ||
| }, | ||
| }, | ||
| "required": [ | ||
| "id", | ||
| ], | ||
| "type": "object", | ||
| } | ||
| `); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strong test coverage for object transformation methods.
The new test suite effectively validates the core functionality of pick(), omit(), partial(), and merge() operations:
- ✓ Immutability checks confirm new instances and cloned properties (lines 220-221, 265-266)
- ✓ Required field semantics are properly verified across operations
- ✓ Nested property merging is tested
- ✓ Inline snapshots provide detailed regression protection
- ✓ Helper function promotes clean, DRY test code
💡 Consider adding edge case coverage
The following scenarios would strengthen the test suite:
describe("object transforms edge cases", () => {
it("should handle picking non-existent properties", () => {
const schema = buildUserSchema();
const picked = schema.pick("id", "nonExistent" as any);
// Should only include existing properties
});
it("should handle empty pick/omit lists", () => {
const schema = buildUserSchema();
expect(schema.pick().toJSON().properties).toEqual({});
expect(schema.omit().toJSON().properties).toEqual(schema.toJSON().properties);
});
it("should handle merging conflicting property types", () => {
const schema1 = JsonSchema.from({type: Object}).properties({
id: JsonSchema.from({type: String})
});
const schema2 = JsonSchema.from({type: Object}).properties({
id: JsonSchema.from({type: Number})
});
// Verify last-wins or error behavior
});
});🤖 Prompt for AI Agents
In packages/specs/schema/src/domain/JsonSchema.spec.ts around lines 205-364, add
an "object transforms edge cases" describe block that includes three focused
tests: 1) "should handle picking non-existent properties" — call schema.pick
with an existing and a non-existent key and assert the result only contains
existing properties; 2) "should handle empty pick/omit lists" — assert
schema.pick() yields an empty properties map and schema.omit() with no args
returns the original properties; 3) "should handle merging conflicting property
types" — create two schemas with the same property name but different primitive
types, merge them and assert the repository's defined behavior (e.g., last-wins
or error) by checking the merged property's type accordingly; ensure tests use
the existing buildUserSchema helper and include expectations that make the
intended behavior explicit.
Comment on lines
+1103
to
+1122
| pick<K extends SchemaKey<T>>(...keys: K[]): JsonSchema<SchemaPick<T, K>> { | ||
| const keySet = new Set(keys.map((key) => toJsonRegex(key))); | ||
| const schema = this.clone() as JsonSchema<SchemaPick<T, K>>; | ||
|
|
||
| const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => { | ||
| if (keySet.has(key)) { | ||
| return { | ||
| ...properties, | ||
| [key]: value | ||
| }; | ||
| } | ||
| return properties; | ||
| }, {}); | ||
|
|
||
| schema.set("properties", properties); | ||
|
|
||
| schema.#required = new Set([...schema.#required].filter((key) => keySet.has(key))); | ||
|
|
||
| return schema; | ||
| } |
Contributor
There was a problem hiding this comment.
Add a guard for missing properties.
If pick() is called on a schema without a properties map (e.g., a primitive schema), Object.entries(schema.get("properties")) will throw because Object.entries(undefined) is invalid.
The partial() method already includes a guard (if (properties)) for this case.
🔎 Suggested fix
pick<K extends SchemaKey<T>>(...keys: K[]): JsonSchema<SchemaPick<T, K>> {
const keySet = new Set(keys.map((key) => toJsonRegex(key)));
const schema = this.clone() as JsonSchema<SchemaPick<T, K>>;
+ const currentProperties = schema.get("properties") || {};
- const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => {
+ const properties = Object.entries(currentProperties).reduce((properties: any, [key, value]: [string, any]) => {
if (keySet.has(key)) {
return {
...properties,
[key]: value
};
}
return properties;
}, {});
schema.set("properties", properties);
schema.#required = new Set([...schema.#required].filter((key) => keySet.has(key)));
return schema;
}🤖 Prompt for AI Agents
In packages/specs/schema/src/domain/JsonSchema.ts around lines 1103 to 1122, the
pick() implementation calls Object.entries(schema.get("properties")) without
guarding for a missing properties map which will throw for primitive schemas;
add a guard like in partial() that first reads const properties =
schema.get("properties"); if properties is falsy, return schema immediately (no
changes), otherwise proceed to build the filtered properties object and set it
and update #required; this prevents Object.entries(undefined) and preserves
current behavior for non-object schemas.
Comment on lines
+1124
to
+1144
| omit<K extends SchemaKey<T>>(...keys: K[]): JsonSchema<SchemaOmit<T, K>> { | ||
| const keySet = new Set(keys.map((key) => toJsonRegex(key))); | ||
| const schema = this.clone() as JsonSchema<SchemaOmit<T, K>>; | ||
|
|
||
| const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => { | ||
| if (!keySet.has(key)) { | ||
| return { | ||
| ...properties, | ||
| [key]: value | ||
| }; | ||
| } | ||
|
|
||
| return properties; | ||
| }, {}); | ||
|
|
||
| schema.set("properties", properties); | ||
|
|
||
| keySet.forEach((key) => schema.#required.delete(key)); | ||
|
|
||
| return schema; | ||
| } |
Contributor
There was a problem hiding this comment.
Add a guard for missing properties (consistent with pick).
Same issue as pick() — calling omit() on a schema without properties will throw.
🔎 Suggested fix
omit<K extends SchemaKey<T>>(...keys: K[]): JsonSchema<SchemaOmit<T, K>> {
const keySet = new Set(keys.map((key) => toJsonRegex(key)));
const schema = this.clone() as JsonSchema<SchemaOmit<T, K>>;
+ const currentProperties = schema.get("properties") || {};
- const properties = Object.entries(schema.get("properties")).reduce((properties: any, [key, value]: [string, any]) => {
+ const properties = Object.entries(currentProperties).reduce((properties: any, [key, value]: [string, any]) => {
if (!keySet.has(key)) {
return {
...properties,
[key]: value
};
}
return properties;
}, {});
schema.set("properties", properties);
keySet.forEach((key) => schema.#required.delete(key));
return schema;
}🤖 Prompt for AI Agents
In packages/specs/schema/src/domain/JsonSchema.ts around lines 1124-1144,
calling Object.entries(schema.get("properties")) can throw when properties is
missing; add the same guard used in pick(): read const props =
schema.get("properties"); if (!props) return schema; then operate on
Object.entries(props) to build the new properties object and keep the rest of
the logic (setting properties and removing required keys) unchanged.
Collaborator
Author
|
🎉 This PR is included in version 8.21.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.