-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[feat][elixir] use ecto schemas and changesets in generated models #21208
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/ElixirClientCodegen.java
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/elixir/deserializer.ex.mustache
Outdated
Show resolved
Hide resolved
...tstore/elixir/lib/openapi_petstore/model/mixed_properties_and_additional_properties_class.ex
Outdated
Show resolved
Hide resolved
samples/client/petstore/elixir/lib/openapi_petstore/model/array_of_array_of_number_only.ex
Show resolved
Hide resolved
samples/client/petstore/elixir/lib/openapi_petstore/model/pet.ex
Outdated
Show resolved
Hide resolved
0a9c9df
to
7e30705
Compare
@@ -25,25 +24,28 @@ defmodule DeserializerTest do | |||
"name": "sea" | |||
} | |||
], | |||
"status": "foo" | |||
"status": "available" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version did not validate enum values. This is one of the nice goodies we get from switching to Ecto.
%FormatTest{ | ||
integer: 1, | ||
int32: 2, | ||
int64: 3, | ||
number: 4.1, | ||
float: 5.2, | ||
double: 6.3, | ||
decimal: "7.4", | ||
decimal: 7.4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version did not handle type-casting of decimal values. This is a nice goodie we get from switching to Ecto.
@@ -6,6 +6,7 @@ defmodule OuterEnumTest do | |||
|
|||
@valid_json """ | |||
{ | |||
"enum_string_required": "lower", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows that the previous implementation was "off", and shows one of the benefits of migrating to Ecto. Now, it's easier and more idiomatic to write proper type-casting and validations, which allows us to have a better Elixir generator.
d1684c7
to
21c8024
Compare
21c8024
to
6dce1e6
Compare
6dce1e6
to
fa24401
Compare
fa24401
to
6ad3d78
Compare
f8684dc
to
618e39b
Compare
618e39b
to
3f6e3f3
Compare
@wing328, @mrmstn - I feel this pull request is ready for a first review. Curious to see what you think 😊 It is a significant change, and I recognize it comes with some risk, but I believe it can move us closer to offering a better developer experience for anyone working with clients generated by the Elixir generator. Do you happen to know of a clean way to dynamically skip generating certain model files? With this PR, I improved enum support by inlining enum values directly into the models where they are used. The previous approach was a bit rough. Models like enum_class) were essentially placeholders with no real value. Ideally, I would like to avoid generating these kinds of empty or redundant files altogether. For now, I’ve updated the model.mustache template to emit empty modules in such cases, but that feels like a workaround. I'm curious if you're aware of a more elegant or supported way to handle this within the generator. I’m also a bit unsure about the way I handled schemas nested inside maps. While the current solution works, it's limited, and I wonder whether it's worth supporting this at all. These kinds of nested structures, ie. maps of embedded schemas, potentially mixed with arrays, are hard to support generically in a meaningful way. Given that the existing generator doesn’t handle this comprehensively either (unless I am mistaken), I’m leaning toward treating such schemas as plain maps and leaving the responsibility of validation and transformation to developers. It might be better to offer a clear escape hatch than to implement a partial solution that gives a false sense of completeness. Would love to hear your thoughts on both points. |
Description
This
PR
extends the Elixir OpenAPI code generator to use Ecto (embeded) schemas for the generated models.Rather than relying on plain structs or custom validation code, this approach leverages Ecto's built-in validation and casting capabilities to provide a more powerful and idiomatic solution for API client development in Elixir.
The motivation for this change is three-fold:
Ecto
is widely adopted within the Elixir ecosystem. By aligning the generated code with this de facto standard, we make the output more familiar and maintainable for Elixir developers. Arguably, working with Ecto.Schema and Ecto.Changeset is more idiomatic than easier to reason about that the custom logic currently used by the Elixir generator (eg. generated deserializer module).Improved validation support out of the box: Ecto's validation functions (e.g., validate_required/2, validate_length/3, etc.) enable robust runtime checks for incoming API data, helping catch mismatches early and making client code safer by default.
Extensibility: By using Ecto changesets, developers consuming the generated models have a clear and idiomatic extension point for adding custom validation logic or transformation steps, without having to rework the generated code structure. For example, adding validation for a
string
field withemail
format would be as simple as patching the generated model with something along the lines ofvalidate_format(:email, ~r/@/)
.As usual, curious to hear what you think about this change @wing328 and @mrmstn. I understand this pull-request is more controversial than previous pull-requests, but I believe it helps us elevate the quality of the clients generated by the Elixir generator, making generated clients more idiomatic (read as in accessible) to the wider Elixir community.
Whilst the public API of the generated client does not change, I understand this change could be considered a breaking change. This said and given that the Elixir generator is still to considered to be an alpha version, maybe we can afford targeting
master
since this change may get the Elixir generator one step closer to leaving its "alpha" status. What do you think?I am opening this as a draft because whilst the change is fully functional, I am still working on polishing a few rough edges.
What has changed?
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)