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

[rust-server] (Re-)Adding support for rust-server #290

Merged
merged 11 commits into from
Jun 13, 2018

Conversation

bjgill
Copy link
Contributor

@bjgill bjgill commented Jun 12, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This is an initial attempt at getting rust-server working in openapi-generator (and therefore fix #111).

Thanks to @wing328 and @mirw for their help with this.

Outstanding issues:

  • Responses with schema: {type: string}
  • Enum strings (and arrays of strings) in form data params
  • Byte strings in form data parameters
  • Binary string properties

@bjgill
Copy link
Contributor Author

bjgill commented Jun 12, 2018

@wing328 - opening as you suggested. The first issue above is the one currently causing the most problems.

@bjgill bjgill changed the title [WIP] Adding support for rust-server [WIP] [rust-server] Adding support for rust-server Jun 12, 2018

pub struct ArrayOfArrayOfNumberOnly(object);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Have been looking into the missing objects with a single property. I've added some logs that catch them as they pass through postProcessModels (printing out the datatype):

[main] WARN  o.o.c.languages.RustServerCodegen - Found a disappearing model: OuterNumber(OuterNumber), number
[main] WARN  o.o.c.languages.RustServerCodegen - Found a disappearing model: ArrayOfNumberOnly(ArrayOfNumberOnly), object
[main] WARN  o.o.c.languages.RustServerCodegen - Found a disappearing model: EnumClass(EnumClass), string
[main] WARN  o.o.c.languages.RustServerCodegen - Found a disappearing model: List(List), object
[main] WARN  o.o.c.languages.RustServerCodegen - Found a disappearing model: NumberOnly(NumberOnly), object

It looks as if those with datatype object turn up in models.rs with the datatype object, whilst those with other datatypes just disappear...

@bjgill
Copy link
Contributor Author

bjgill commented Jun 12, 2018

OK - models with variables now seem to be working correctly. Models without properties, though, are still troublesome. It looks as if they're going missing between postProcessModels and models.mustache. I wonder if there is a particular field in their CodegenModels that is set wrong? I'm going to continue investigating...

@bjgill
Copy link
Contributor Author

bjgill commented Jun 12, 2018

Ah-ha! It looks like the single-property models have isAlias set to true. Presumably, something somewhere is filtering them out.

I'm not sure what the correct solution is, but I think I can get something working by setting isAlias to false and then tidying up the dataType.

@bjgill
Copy link
Contributor Author

bjgill commented Jun 12, 2018

Currently not working:

  • Responses with schema: {type: string}
  • Enum strings (and arrays of strings) in form data params
  • Byte strings in form data parameters
  • Binary string properties

However, those aside, we now produce code that compiles correctly. I think I'm now out of time to resolve the gaps for the moment, however. I'll tidy this up so that this could be merged as a barely acceptable initial effort.

Benjamin Gill added 2 commits June 12, 2018 17:38
We've commented out a few bits that rust-server doesn't yet support
And finally get rid of the generation date in the sample
@bjgill bjgill changed the title [WIP] [rust-server] Adding support for rust-server [rust-server] Adding support for rust-server Jun 12, 2018
@bjgill
Copy link
Contributor Author

bjgill commented Jun 12, 2018

Right. I think this is now in an acceptable state to get merged in (assuming CI passes). This PR allows rust-server to produce valid Rust in openapi-generator. It's not comprehensive - #290 (comment) tracks the known areas that I have not been able to re-add support for.

I'm proposing to merge this PR in its current state, however, since it does work for most mainline cases, and so may be useful in itself, and also as a starting point for other people to work off. I'm also almost out of time to work on rust-server for the moment, and would like to leave things in a reasonable clean state.

Once merged, I'll create an issue detailing all the known outstanding work required.

Technical committee: @frol @farcaller

@bjgill bjgill changed the title [rust-server] Adding support for rust-server [rust-server] (Re-)Adding support for rust-server Jun 12, 2018
@wing328
Copy link
Member

wing328 commented Jun 13, 2018

FYI. The CI failures have nothing to do with the change in this PR.

@wing328
Copy link
Member

wing328 commented Jun 13, 2018

I've reviewed the change, which looks good to me.

I'll submit a PR later for enhancements, e.g. check for model/object in CodegenParameter, isFile check, etc

@wing328 wing328 merged commit 6f6a4a1 into OpenAPITools:master Jun 13, 2018
@bjgill bjgill deleted the openapi-generator-rust-server branch June 13, 2018 12:30
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.

[rust-server] non-valid Rust produced in switch to OpenAPI v3
2 participants