-
-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: type are not determined in cyclic models #1906
Merged
jonaslagoni
merged 18 commits into
asyncapi:next
from
jonaslagoni:fix_incorrect_self_reference_problem
May 6, 2024
Merged
fix: type are not determined in cyclic models #1906
jonaslagoni
merged 18 commits into
asyncapi:next
from
jonaslagoni:fix_incorrect_self_reference_problem
May 6, 2024
Conversation
This file contains 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
✅ Deploy Preview for modelina canceled.
|
jonaslagoni
force-pushed
the
fix_incorrect_self_reference_problem
branch
from
April 29, 2024 06:25
95c97e3
to
c6a8d72
Compare
jonaslagoni
changed the title
fix!: incorrect self reference problem
fix: type are not determined in cyclic models
Apr 29, 2024
Quality Gate passedIssues Measures |
jonaslagoni
requested review from
artur-ciocanu,
Ferror,
leigh-johnson,
Samridhi-98,
magicmatatjahu and
kennethaasan
as code owners
April 29, 2024 10:57
Ferror
approved these changes
May 1, 2024
🎉 This PR is included in version 4.0.0-next.36 🎉 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.
Description
As #1903 explains, the underlying problem occurs because of the way we constrain models and their types, basically doing both at the same time.
This means that when we have circular models and they depend on each other, and the type has not been set yet, but we expect it to, we end up with the type having the value
''
. I.e. why we end up with self._items: | List[] = input['items'] where the union model that items reference has not been sat when we set the item's type.With this change in approach, we first build the constrained model (with any value that makes sense at the factory time), and then we apply types based on "safe types" first, and once those have been resolved, if we detect a circular model, we open it up by replacing it with
AnyModel
, and apply the more complex types automatically.Lastly, we then apply constants and discriminatory values (because they depend on type).
I would have loved to find a better way to do this implementation, but this is the best my bandwidth has come up with for the past weeks...
The reason this is not a breaking change is because it fixes types just being generated with
''
values, and therefore this corrects that incorrect syntax. If a type would have been generated, this would have been a breaking change.Fixes #1903
Blocked by #1893