Skip to content

feat(core): honor schema-level nullable:true in OpenAPI 3.0#83

Merged
mfabisiak merged 5 commits into
masterfrom
feat/41-schema-nullable
Jul 3, 2026
Merged

feat(core): honor schema-level nullable:true in OpenAPI 3.0#83
mfabisiak merged 5 commits into
masterfrom
feat/41-schema-nullable

Conversation

@halotukozak

Copy link
Copy Markdown
Contributor

What

OpenAPI 3.0 nullable: true was ignored — nullability came only from the parent's required set. A property that is required and nullable: true was generated as a non-nullable Kotlin type, which is incorrect.

  • SpecParser.propertyModels now sets nullable = propName !in required || propSchema.nullable == true.
  • The allOf merge preserves schema-level nullability when recomputing against the union of required sets (prop.name !in required || prop.nullable).

Tests

Covers the four combinations on a plain object schema:

  • required + nullable:true → nullable
  • required + no nullable → non-nullable
  • optional + nullable:true → nullable
  • optional + no nullable → nullable

Notes

OpenAPI 3.1 nullability (type arrays / null in type) is out of scope — this addresses the 3.0 nullable keyword. allOf+nullable ordering edge cases (a property declared in one member, marked required in a later member) are rare and handled best-effort via the OR.

Closes #41

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Coverage Report

Overall Project 96.54% -0.02% 🍏
Files changed 97.56% 🍏

File Coverage
SpecParser.kt 94.72% -0.06% 🍏

Property nullability was derived solely from the parent's required set,
so a required property marked nullable:true was generated as a
non-nullable Kotlin type. The parser now treats a property as nullable
when it is absent from required OR the schema sets nullable:true. The
allOf merge preserves schema-level nullability when recomputing against
the union of required sets.

Closes #41

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@halotukozak halotukozak force-pushed the feat/41-schema-nullable branch from 10c3893 to a4396eb Compare June 9, 2026 14:13
@halotukozak halotukozak added this to the 1.0.0 milestone Jun 9, 2026
@halotukozak halotukozak self-assigned this Jun 9, 2026
@halotukozak halotukozak added the bug Something isn't working label Jun 9, 2026
…able

# Conflicts:
#	core/src/main/kotlin/com/avsystem/justworks/core/parser/SpecParser.kt
@mfabisiak mfabisiak self-requested a review June 26, 2026 10:24
Copilot AI review requested due to automatic review settings July 3, 2026 11:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mfabisiak mfabisiak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bartek's PR introduced a regression in the allOf merge: a required property could come out nullable (String? instead of String), depending on the order the allOf members are listed. Not reachable on master.

When it manifested — a property defined in one member and marked required in a later member:

Base:                 # defines `foo` (no required, no nullable)
  properties: { foo: { type: string } }
RequiresFoo:          # only marks foo required
  required: [foo]
Combined:
  allOf: [ {$ref: Base}, {$ref: RequiresFoo} ]

Combined.foo is required and not marked nullable, so it must be foo: String — but it generated foo: String?. Swap the two members and it's correct: the result depended purely on member order.

Why — the merge folds over members, computing each property's nullability against the required set known so far. foo is built while processing Base (required still empty → nullable = true); the later RequiresFoo adds it to required but never re-computes it, so the stale true sticks. On master the final line was an authoritative reset (nullable = name !in required) that discarded this. The PR changed it to name !in required || prop.nullable to keep the nullable: true marker — but prop.nullable isn't just the marker, it's the stale not-in-partial-required || marker, so OR-ing it back in resurfaced the bug.

Fix — compute the full required set first, then derive properties from it in one pass; the separate recompute is then redundant and removed:

val subSchemas = schema.allOf.orEmpty().map { it.resolveSubSchema() }
val required = topRequired + subSchemas.flatMap { it.required.orEmpty() }.toSet()
val properties = subSchemas.fold(emptyMap<String, PropertyModel>()) { acc, s ->
    acc + s.propertyModels(required, contextCreator)
}
val finalProperties = properties.plus(schema.propertyModels(required, contextCreator)).values.toList()

Now allOf computes nullability like the non-allOf and inline paths — one pass against the complete required set, no order dependency. nullable: true still works. Added a regression test (allOf property required in a later member is not nullable).

@mfabisiak mfabisiak requested a review from MattK97 July 3, 2026 11:54

@MattK97 MattK97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mfabisiak mfabisiak merged commit f52d3ab into master Jul 3, 2026
1 check passed
@mfabisiak mfabisiak deleted the feat/41-schema-nullable branch July 3, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support schema-level nullable: true in OpenAPI 3.0

4 participants