Skip to content

Fix passthrough additionalProperties fallback for models with typed properties#1560

Open
lohanidamodar wants to merge 3 commits into
masterfrom
fix/passthrough-additional-properties-fallback
Open

Fix passthrough additionalProperties fallback for models with typed properties#1560
lohanidamodar wants to merge 3 commits into
masterfrom
fix/passthrough-additional-properties-fallback

Conversation

@lohanidamodar
Copy link
Copy Markdown
Member

@lohanidamodar lohanidamodar commented May 27, 2026

What changed

Two related fixes for the model passthrough/additionalProperties handling. Sister PR on the Appwrite side that emits the spec: appwrite/appwrite#12403.

1. Fix the regression in fromMap fallback (commit 1)

Commit 171408b (PR #1543) added a definition.properties | length > 0 branch around the ?? fallback in the dart/kotlin/android Model templates. Models that have BOTH typed properties AND outer additionalProperties: true (Document, Row, Presence, etc.) ended up emitting an empty-map fallback (?? {} / ?: emptyMap()), which silently drops user payloads when the response is inline (e.g. Row's user columns live at the top level next to $id, not nested under a data key).

This restores the unconditional whole-map fallback so the same code path that worked before #1543 keeps working. Swift template is unaffected — it uses Codable and never had the branch.

2. Drop the x-additional-properties-key vendor extension (commit 2)

The extension let an Appwrite response model declare a custom name (e.g. metadata) for the passthrough field in the generated SDK, on top of marking the outer schema as additionalProperties: true. This conflated two distinct OpenAPI 3 concepts:

  • Outer-level additionalProperties: true — extras are inline siblings of typed properties. Standard.
  • Nested free-form object — express as a regular property of type object with additionalProperties: true on that property. Standard.

The vendor extension was being used to model the second case via the first one, which required generator-side special handling and made sdk-generator non-portable for anyone consuming a vanilla OpenAPI 3 spec.

This PR removes:

  • the x-additional-properties-key reading from src/Spec/Swagger2.php,
  • the definition.additionalPropertiesKey | default('data') lookup from every template that used it (dart, kotlin, android, swift, php, dotnet, ruby, rust, python — model and test templates).

All templates now emit the literal 'data' field name for inline-passthrough models — same default as before. The sister Appwrite PR (#12403) migrates the one consumer (Presence) to a normal typed metadata property and removes the emission side of the extension.

Why now

After #12403 lands, no Appwrite spec will emit x-additional-properties-key anymore. The reading was already dead in CI: mock-server/ (the spec source for sdk-generator's integration tests) has no models that use the extension.

Verification

  • composer lint-twig: 539 files, 0 errors.
  • Regenerated dart/kotlin/android example SDKs spot-checked: Row, Document, Presence-equivalent models all emit ?? map (whole-map fallback) in fromMap again.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 27, 2026

Greptile Summary

This PR delivers two coordinated fixes: it restores the unconditional whole-map fallback in fromMap/from(map:) for mixed models (those with both typed properties and additionalProperties: true) across Dart, Kotlin, Android, and Swift, reversing a regression from #1543; and it removes the x-additional-properties-key vendor extension from the parser and all 10 language templates, hardcoding the field name to 'data' in preparation for the companion Appwrite spec PR (#12403) that eliminates the only consumer of that extension.

  • Regression fix: The {% if definition.properties | length > 0 %} branch introduced in Presence api #1543 has been removed from Dart, Kotlin, Android, and Swift templates. All four now use a single unconditional path (?? map / ?: map.jsonCast(…) / ?? map) so models like Row and Document, whose user columns live at the top level of the API response rather than nested under a "data" key, deserialize correctly again.
  • Vendor extension removal: additionalPropertiesKey is dropped from Swagger2::getDefinitions() and getRequestModels(), and every template that referenced definition.additionalPropertiesKey | default('data') now uses the literal 'data'. PHP, Python, Ruby, Rust, and .NET are purely mechanical substitutions; their deserialization logic was never affected by the regression.

Confidence Score: 5/5

Safe to merge; all changed paths have been cross-checked and the whole-map fallback is correctly restored for every affected language.

The regression fix is straightforward and consistent across all four affected templates. The vendor-extension removal is mechanical — every template that read additionalPropertiesKey now emits the literal 'data', which was the default value in every case. The companion Appwrite spec PR removes the only emitter of x-additional-properties-key, so no in-flight spec will silently produce the wrong field name. The previously flagged Swift from(map:) issue is also resolved in this PR.

No files require special attention.

Important Files Changed

Filename Overview
src/Spec/Swagger2.php Removes additionalPropertiesKey from both getDefinitions() and getRequestModels() — straightforward, no fallback left behind.
templates/dart/lib/src/models/model.dart.twig Removes the `properties
templates/kotlin/src/main/kotlin/io/appwrite/models/Model.kt.twig Identical regression fix to Dart — removes `properties
templates/android/library/src/main/java/io/package/models/Model.kt.twig Same regression fix as Kotlin — collapses the two-branch fromMap to a single unconditional whole-map fallback.
templates/swift/Sources/Models/Model.swift.twig Removes the `properties
templates/php/src/Models/Model.php.twig Only replaces the additionalPropertiesKey variable with the literal 'data'; the PHP deserialization path (extractAdditionalPropertiesFromFields) was never affected by the regression.
templates/dotnet/Package/Models/Model.cs.twig Replaces additionalPropertiesKey with literal 'data'; the TryGetValue(…) : map whole-map fallback was already present and unaffected by the regression.
templates/python/package/models/model.py.twig Replaces additionalPropertiesKey with 'data' in 10 spots across the property declaration, getter/setter, and to_dict; no logic changes.
templates/ruby/lib/container/models/model.rb.twig Replaces additionalPropertiesKey with 'data'; the `map["data"]
templates/rust/src/models/model.rs.twig Replaces additionalPropertiesKey with 'data' in the struct field, accessor, and get helper; no logic changes.

Reviews (5): Last reviewed commit: "Fix Swift Model template passthrough fal..." | Re-trigger Greptile

…roperties

Commit 171408b ("refactor: remove expiry handling and improve additional
properties handling in models", PR #1543) added a property-count branch in
the Dart, Kotlin and Android model templates that picked the wrong fallback
when a model defines BOTH typed properties AND additionalProperties: true.

For affected models (Row, Document, Presence) the generated fromMap used an
empty-map fallback (Map<String, dynamic>.from(map["data"] ?? {}) in Dart,
emptyMap<String, Any>().jsonCast(to = nestedType) in Kotlin/Android) instead
of falling back to the whole input map. This silently dropped user-defined
columns/fields when the server response omitted the additionalProperties key.

Collapse the inner property-count branch so the whole-map fallback
(?? map / ?: map.jsonCast(to = nestedType)) is always emitted whenever
additionalProperties is true. The additionalPropertiesKey extension (used by
e.g. Presence's "metadata") is preserved.

Co-Authored-By: Damodar Lohani <dlohani48@gmail.com>
Removes the vendor-extension-driven indirection that allowed Appwrite
response models to redirect a passthrough additionalProperties payload
into a custom-named field (e.g. metadata) in generated SDKs. With the
upstream Appwrite spec dropping the x-additional-properties-key emission
in favour of expressing such fields as ordinary typed properties of
type object with additionalProperties: true, the sdk-generator side of
the mechanism is dead code and a non-standard barrier for any downstream
adopter generating SDKs from a vanilla OpenAPI 3 spec.

- src/Spec/Swagger2.php: stop reading x-additional-properties-key.
- templates/{dart,kotlin,android,swift,php,dotnet,ruby,rust,python}:
  replace definition.additionalPropertiesKey lookups with the literal
  'data' field name (the previous default).
Comment thread templates/swift/Sources/Models/Model.swift.twig Outdated
The Swift template still carried the property-count branch that the
dart, kotlin, and android templates lost in the parent commit:
mixed models (typed properties + additionalProperties: true) fell
through to JSONSerialization of an empty [:] dict when map["data"]
was absent. For Row and Document — whose user data lives inline at
the top level of the response, not nested under a "data" key — this
silently discarded the entire payload, matching the regression we
fixed in the other three languages.

Collapse to the whole-map fallback (?? map) the else branch already
used for pure-passthrough models. Generated examples/swift/Sources/
AppwriteModels/Row.swift now emits map["data"] ?? map alongside the
other SDKs.

Catch by greptile-apps[bot] inline review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant