Clean up file output handling #1895

jimschubert · 2019-01-12T03:50:01Z

This attempts to normalize all generators to use OS agnostic
File.separator.

It also fixes a bug in some areas in code where we replace "." in full file
output path with File.separator. We should only be modifying directory
names we own, and should avoid modifying anything that can be provided
by a user.

It would probably be better to use Paths.get(…).toString() in all cases.

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.4.x, 4.0.x~~. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

This attempts to normalize all generators to use OS agnostic File.separator. It also cleans up some areas in code where we replace "." in full file output path with File.separator. We should only be modifying directory names we own, and should avoid modifying anything that can be provided by a user. It would probably be better to use Paths.get(…).toString() in all cases.

jimschubert · 2019-01-12T04:20:15Z

This should fix #1834.

wing328 · 2019-01-12T04:22:27Z

@jimschubert thanks for the PR. I'll review later today.

Have a nice weekend.

* master: Add links to Patreon account (#1887) Python AIOHTTP server generator (#1470) Fix a problem that points to a folder that doesn't exist (#1863) Python: Update api_doc_example for multiple auth (#1870) [gradle] Add 4 boolean properties supported by codegenConfigurator (#1881)

jimschubert · 2019-01-12T04:40:01Z

@wing328 no problem. I've fixed merge conflicts.

To repro #1834, you can use -o some.dir on master and supporting files will write to some.dir while model/apis will write to some/dir. This was all in AbstractJavaCodegen.java. Other files were normalization of File.separator and usually moving parens so we didn't modify the user-specified outputFolder value.

ackintosh · 2019-01-12T09:30:49Z

https://travis-ci.org/OpenAPITools/openapi-generator/builds/478650968?utm_source=github_status&utm_medium=notification

It seems the issue that fixed in #1863 occurs again. 👀

wing328 · 2019-01-12T10:20:54Z

and here are the errors reported by Travis (and other CIs):

[ERROR] Tests run: 520, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 11.875 s <<< FAILURE! - in TestSuite
[ERROR] apiFileFolder(org.openapitools.codegen.java.AbstractJavaCodegenTest)  Time elapsed: 0.009 s  <<< FAILURE!
java.lang.AssertionError: expected [/User/open.api.tools/source.folder/org/openapitools/codegen/api] but found [/User/open.api.tools/source/folder/org/openapitools/codegen/api]
	at org.openapitools.codegen.java.AbstractJavaCodegenTest.apiFileFolder(AbstractJavaCodegenTest.java:167)
[ERROR] apiTestFileFolder(org.openapitools.codegen.java.AbstractJavaCodegenTest)  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError: expected [/User/open.api.tools/test.folder/org/openapitools/codegen/api] but found [/User/open.api.tools/test/folder/org/openapitools/codegen/api]
	at org.openapitools.codegen.java.AbstractJavaCodegenTest.apiTestFileFolder(AbstractJavaCodegenTest.java:176)
[ERROR] modelFileFolder(org.openapitools.codegen.java.AbstractJavaCodegenTest)  Time elapsed: 0.001 s  <<< FAILURE!
java.lang.AssertionError: expected [/User/open.api.tools/source.folder/org/openapitools/codegen/model] but found [/User/open.api.tools/source/folder/org/openapitools/codegen/model]
	at org.openapitools.codegen.java.AbstractJavaCodegenTest.modelFileFolder(AbstractJavaCodegenTest.java:194)
[ERROR] modelTestFileFolder(org.openapitools.codegen.java.AbstractJavaCodegenTest)  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError: expected [/User/open.api.tools/test.folder/org/openapitools/codegen/model] but found [/User/open.api.tools/test/folder/org/openapitools/codegen/model]
	at org.openapitools.codegen.java.AbstractJavaCodegenTest.modelTestFileFolder(AbstractJavaCodegenTest.java:185)

jimschubert · 2019-01-12T12:53:52Z

Sorry about that. I merged in master to fix the file conflicts and it introduced another path replacement and groupings ended up shifting during merge.

I'm currently needing to push to remote in order to have tests run, because after evaluating the Guava issue with Android Gradle Plugin, the test suite locks up on my machine.

…d noop elsewhere

jimschubert · 2019-01-12T13:43:47Z

Last commit should fix the shippable issue for Windows. I have concerns that a user on Windows would never provide a path like /Users/jim/path and require a / replacement to \. Considering / is an invalid path character in Windows I think this is fine from a testing perspective. However, most generators don't do this, so we may want to either update this universally across all generators or do it in DefaultGenerator before using these values.

…constraints

jimschubert · 2019-01-12T16:31:14Z

As mentioned above, and discussed in chat, I've used Paths.get at the point of writing files to cover
normalization of path for all generators.

This resolves an issue, for instance when you do Paths.get("C:/Users/jim/mypath") on Windows in any generator, we expect to write to C:\Users\jim\mypath. This should also handle normalizing filenames with spaces in all operating systems. Using Paths.get will also expose a better exception for invalid path characters, which should help us to track down issues with path characters a little better in the future.

ackintosh

Cool! LGTM ✨

wing328

LGTM

* Clean up file output handling This attempts to normalize all generators to use OS agnostic File.separator. It also cleans up some areas in code where we replace "." in full file output path with File.separator. We should only be modifying directory names we own, and should avoid modifying anything that can be provided by a user. It would probably be better to use Paths.get(…).toString() in all cases. * Fix missed path separators in java codegen * Adjust Java codegen path replacements * Convert / in full path replacements, as / is forbidden in Windows, and noop elsewhere * Use Paths.get where files are written, to better handle Windows path constraints

This should fix an issue where output path names containing . were not being interpretted properly REFERENCE: OpenAPITools/openapi-generator#1895

jimschubert added 2 commits January 11, 2019 22:40

Fix missed path separators in java codegen

e97527d

jimschubert added the Issue: Bug label Jan 12, 2019

jimschubert mentioned this pull request Jan 12, 2019

[BUG] Spring Generator wrong Path parsing #1834

Closed

Adjust Java codegen path replacements

27d93a9

Convert / in full path replacements, as / is forbidden in Windows, an…

3285693

…d noop elsewhere

Use Paths.get where files are written, to better handle Windows path …

113adda

…constraints

ackintosh approved these changes Jan 14, 2019

View reviewed changes

wing328 approved these changes Jan 14, 2019

View reviewed changes

wing328 merged commit db9102a into master Jan 14, 2019

wing328 deleted the fix-path-cleaning-on-write branch January 14, 2019 14:49

wing328 added this to the 4.0.0 milestone Jan 31, 2019

j-fontaine mentioned this pull request Jun 24, 2019

Update OpenAPITools openapi-generator from version 4.0.0-beta to 4.0.2 connexta/ion-ingest-api#10

Merged

3 tasks

j-fontaine mentioned this pull request Jun 26, 2019

Update the openapi.generator plugin from version 4.0.0-beta to 4.0.2 connexta/discovery-api#6

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Clean up file output handling #1895

Clean up file output handling #1895

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

wing328 commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

ackintosh commented Jan 12, 2019

Uh oh!

wing328 commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

ackintosh left a comment

Uh oh!

wing328 left a comment

Uh oh!

Uh oh!

Uh oh!

Clean up file output handling #1895

Clean up file output handling #1895

Uh oh!

Conversation

jimschubert commented Jan 12, 2019

PR checklist

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

wing328 commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

ackintosh commented Jan 12, 2019

Uh oh!

wing328 commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

jimschubert commented Jan 12, 2019

Uh oh!

ackintosh left a comment

Choose a reason for hiding this comment

Uh oh!

wing328 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!