Skip to content
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

[dart-dio] fix issue with multi layered discriminators #16136

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

felixmede
Copy link

@felixmede felixmede commented Jul 19, 2023

fix for issue issue #15467

When using the dart-dio generator to generate an API with an specs file, which uses multilayered classes with a discriminator at the lowest layer with an discriminator, the generated code tries to map all the values in the middle layer class to all of the subclasses of the lowest layer class.

e.g. classes animal, bird, reptile, turtle, crocodile
The generator runs without an error but there are issues inside of the generated code.
The discriminator mapping inside of the animal class is working fine but the one at reptile uses the same as the one at the animal. This creates a bird and then return it as a bird. This is not allowed in this place because bird is not a subtype of reptile. Since this is a compile error, created by parts of code, which will never be executed, any app using the library is unable is unable to be started.

https://gist.github.com/felixmede/058272bdad39fa14ee30f03e65dbd1df

@jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08)

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@felixmede
Copy link
Author

Hi

is there anything I still need to do so that it can be merged?

@ahmednfwela
Copy link
Contributor

@felixmede hi, sorry for not noticing this until now, I will check it

@ahmednfwela
Copy link
Contributor

@felixmede is it possible to also fix this #15517
in this PR?

@ahmednfwela
Copy link
Contributor

@wing328 can you run ci please ?
also is changing the java version ok here?

@wing328
Copy link
Member

wing328 commented Aug 30, 2023

also is changing the java version ok here?

I guess so. Not sure how that file is used

@felixmede
Copy link
Author

@felixmede is it possible to also fix this #15517
in this PR?

This is also fixed now

Comment on lines +190 to +198

@Override
public String toString() {
final StringBuffer sb = new StringBuffer("MappedModel{");
sb.append("mappingName='").append(mappingName).append('\'');
sb.append(", modelName='").append(modelName).append('\'');
sb.append('}');
return sb.toString();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 is this change appropriate for the core generator ?

@felixmede
Copy link
Author

Is there anything left to do from my side?

@felixmede
Copy link
Author

@ahmednfwela @wing328 ?

@ahmednfwela
Copy link
Contributor

while I can't give a proper review due to not having proper tests, I think it looks ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants