Support map properties#22
Conversation
There was a problem hiding this comment.
Pull request overview
Adds parsing support for JSON Schema additionalProperties as Basketry mapProperties and extends union parsing to support anyOf (plus explicit disjunction metadata), addressing Issue #9.
Changes:
- Implement
mapPropertiesparsing fromadditionalProperties(boolean and schema forms), including handling inline enum/union naming for map values. - Implement
anyOfunions asSimpleUnionwithinclusivedisjunction and addexclusivedisjunction tooneOfunions. - Add a comprehensive spec test file for map properties and update the snapshot + ESLint glob quoting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/spec/4.1-structure/4.1.11-map-properties.spec.ts | Adds spec-level tests covering additionalProperties map parsing across boolean/ref/enum/oneOf/anyOf cases. |
| src/json-schema-parser.ts | Introduces parseMapProperties / parseMapValue, implements anyOf union parsing, and adds union disjunction metadata. |
| src/snapshot/snapshot.json | Updates snapshot output to include the new disjunction field for a SimpleUnion. |
| package.json | Quotes ESLint globs to avoid shell expansion issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private parseMapProperties( | ||
| schema: AST.AbstractSchemaNode, | ||
| typeName: StringLiteral, | ||
| ): MapProperties | undefined { | ||
| const ap = schema.additionalProperties; | ||
| if (!ap) return undefined; | ||
|
|
||
| // Handle boolean literal additionalProperties (true/false) | ||
| if (isLiteralNode(ap.node)) { | ||
| // false is handled by objectAdditionalPropertiesRule; true means untyped map | ||
| if (ap.node.value !== true) return undefined; | ||
| return { | ||
| kind: 'MapProperties', | ||
| key: { | ||
| kind: 'MapKey', | ||
| value: { | ||
| kind: 'PrimitiveValue', | ||
| typeName: { kind: 'PrimitiveLiteral', value: 'string' }, | ||
| rules: [], | ||
| }, | ||
| }, | ||
| requiredKeys: [], | ||
| value: { | ||
| kind: 'MapValue', | ||
| value: untyped(), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| kind: 'MapProperties', | ||
| key: { | ||
| kind: 'MapKey', | ||
| value: { | ||
| kind: 'PrimitiveValue', | ||
| typeName: { kind: 'PrimitiveLiteral', value: 'string' }, | ||
| rules: [], | ||
| }, | ||
| }, | ||
| requiredKeys: [], | ||
| value: this.parseMapValue(ap, typeName.value), | ||
| }; |
There was a problem hiding this comment.
parseMapProperties/parseMapValue add several behaviors (e.g., additionalProperties: { type: 'string' } without enum, and inline object schemas under additionalProperties) that aren’t exercised by the new spec tests. Adding at least one test for a primitive-typed map and one for an inline object map value would help prevent regressions in these branches.
| const exactSet = new Set<unknown>(); | ||
| function exact<T>(input: T): T { | ||
| exactSet.add(input); | ||
| return input; | ||
| } | ||
|
|
||
| function partial<T = any>(input: DeepPartial<T>): any { | ||
| if (exactSet.has(input)) return input; | ||
|
|
There was a problem hiding this comment.
The exactSet/exact() helper is declared but never used in this spec file, which adds noise and makes the assertions harder to scan. Consider removing it, or using it in at least one assertion so future readers know why it exists.
| const exactSet = new Set<unknown>(); | |
| function exact<T>(input: T): T { | |
| exactSet.add(input); | |
| return input; | |
| } | |
| function partial<T = any>(input: DeepPartial<T>): any { | |
| if (exactSet.has(input)) return input; | |
| function partial<T = any>(input: DeepPartial<T>): any { |
|
|
||
| // Handle boolean literal additionalProperties (true/false) | ||
| if (isLiteralNode(ap.node)) { | ||
| // false is handled by objectAdditionalPropertiesRule; true means untyped map |
There was a problem hiding this comment.
The comment says additionalProperties: false is handled by objectAdditionalPropertiesRule, but objectAdditionalPropertiesFactory currently returns undefined (TODO), so the parser won’t emit any rule/violation for additionalProperties: false. Either update this comment to reflect current behavior, or implement the missing additionalProperties rule so false is enforced.
| // false is handled by objectAdditionalPropertiesRule; true means untyped map | |
| // currently, only `true` produces an untyped map; `false` results in no MapProperties/rule |
| const desiredName = | ||
| schema.oneOf || schema.anyOf | ||
| ? `${parentName}MapValues` | ||
| : schema.enum | ||
| ? `${parentName}MapValue` | ||
| : `${parentName}MapValues`; |
There was a problem hiding this comment.
desiredName’s fallback branch duplicates the union case (... : ${parentName}MapValues``), which makes the ternary harder to reason about and may be unintended (e.g., inline non-union complex schemas would also be renamed to *MapValues). Consider simplifying the conditional and/or choosing distinct singular/plural names for the non-union case to make the intent explicit.
| const desiredName = | |
| schema.oneOf || schema.anyOf | |
| ? `${parentName}MapValues` | |
| : schema.enum | |
| ? `${parentName}MapValue` | |
| : `${parentName}MapValues`; | |
| const hasUnion = !!(schema.oneOf || schema.anyOf); | |
| const desiredName = | |
| schema.enum && !hasUnion | |
| ? `${parentName}MapValue` | |
| : `${parentName}MapValues`; |
skonves
force-pushed
the
additional-properties
branch
from
922bc7a to
83a5a96
Compare
Resolves #9