[BUG][CLI][GENERATOR] NullPointer when not setting outputDir #3448
Conversation
This file contains hidden or 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
…builder. There are a few issues here: - outputDir's default was set to "." but was overwritten by the builder. - validateSpec was defaulted to true but was overwritten to false by the builder. - strictSpecBehavior was defaulted to true but was overwritten to false by the builder. A builder should not have primitive not nullable fields if there are fields containing default which are not nullable. The main issue I migrated the default value setter to the builder is that fields can be nullable but have set a notnull default. If null is an indicator whether to set the value or not those values could never be set to null.
|
The changes in Kotlin are not my fault, they also appear in the master. The ruby changes appear to be from me. Could someone from the Ruby team look over those if I can commit those changes? Those tests are against the latest master with the patch applied Ruby Team: @cliffano (2017/07) @zlx (2017/09) @autopp (2019/02) UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'
Perform git diff
diff --git a/samples/client/petstore/ruby/lib/petstore/api/pet_api.rb b/samples/client/petstore/ruby/lib/petstore/api/pet_api.rb
index 65039a8..ba75ece 100644
--- a/samples/client/petstore/ruby/lib/petstore/api/pet_api.rb
+++ b/samples/client/petstore/ruby/lib/petstore/api/pet_api.rb
@@ -103,7 +103,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.delete_pet"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -290,7 +290,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.get_pet_by_id"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -414,7 +414,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.update_pet_with_form"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -480,7 +480,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.upload_file"
end
# resource path
- local_var_path = '/pet/{petId}/uploadImage'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}/uploadImage'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -552,7 +552,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'required_file' when calling PetApi.upload_file_with_required_file"
end
# resource path
- local_var_path = '/fake/{petId}/uploadImageWithRequiredFile'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/fake/{petId}/uploadImageWithRequiredFile'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
diff --git a/samples/client/petstore/ruby/lib/petstore/api/store_api.rb b/samples/client/petstore/ruby/lib/petstore/api/store_api.rb
index 654e9d9..830cec6 100644
--- a/samples/client/petstore/ruby/lib/petstore/api/store_api.rb
+++ b/samples/client/petstore/ruby/lib/petstore/api/store_api.rb
@@ -43,7 +43,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'order_id' when calling StoreApi.delete_order"
end
# resource path
- local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -167,7 +167,7 @@ module Petstore
end
# resource path
- local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
diff --git a/samples/client/petstore/ruby/lib/petstore/api/user_api.rb b/samples/client/petstore/ruby/lib/petstore/api/user_api.rb
index e0a28bb..f4a5305 100644
--- a/samples/client/petstore/ruby/lib/petstore/api/user_api.rb
+++ b/samples/client/petstore/ruby/lib/petstore/api/user_api.rb
@@ -219,7 +219,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'username' when calling UserApi.delete_user"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -277,7 +277,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'username' when calling UserApi.get_user_by_name"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -465,7 +465,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'body' when calling UserApi.update_user"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
diff --git a/samples/openapi3/client/petstore/ruby/lib/petstore/api/pet_api.rb b/samples/openapi3/client/petstore/ruby/lib/petstore/api/pet_api.rb
index f235041..01beb41 100644
--- a/samples/openapi3/client/petstore/ruby/lib/petstore/api/pet_api.rb
+++ b/samples/openapi3/client/petstore/ruby/lib/petstore/api/pet_api.rb
@@ -103,7 +103,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.delete_pet"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -290,7 +290,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.get_pet_by_id"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -414,7 +414,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.update_pet_with_form"
end
# resource path
- local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -480,7 +480,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'pet_id' when calling PetApi.upload_file"
end
# resource path
- local_var_path = '/pet/{petId}/uploadImage'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/pet/{petId}/uploadImage'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -552,7 +552,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'required_file' when calling PetApi.upload_file_with_required_file"
end
# resource path
- local_var_path = '/fake/{petId}/uploadImageWithRequiredFile'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/fake/{petId}/uploadImageWithRequiredFile'.sub('{' + 'petId' + '}', CGI.escape(pet_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
diff --git a/samples/openapi3/client/petstore/ruby/lib/petstore/api/store_api.rb b/samples/openapi3/client/petstore/ruby/lib/petstore/api/store_api.rb
index 4229190..120fda0 100644
--- a/samples/openapi3/client/petstore/ruby/lib/petstore/api/store_api.rb
+++ b/samples/openapi3/client/petstore/ruby/lib/petstore/api/store_api.rb
@@ -43,7 +43,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'order_id' when calling StoreApi.delete_order"
end
# resource path
- local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -167,7 +167,7 @@ module Petstore
end
# resource path
- local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s).gsub('%2F', '/'))
+ local_var_path = '/store/order/{order_id}'.sub('{' + 'order_id' + '}', CGI.escape(order_id.to_s))
# query parameters
query_params = opts[:query_params] || {}
diff --git a/samples/openapi3/client/petstore/ruby/lib/petstore/api/user_api.rb b/samples/openapi3/client/petstore/ruby/lib/petstore/api/user_api.rb
index 265a387..1500cf5 100644
--- a/samples/openapi3/client/petstore/ruby/lib/petstore/api/user_api.rb
+++ b/samples/openapi3/client/petstore/ruby/lib/petstore/api/user_api.rb
@@ -225,7 +225,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'username' when calling UserApi.delete_user"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -283,7 +283,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'username' when calling UserApi.get_user_by_name"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
@@ -471,7 +471,7 @@ module Petstore
fail ArgumentError, "Missing the required parameter 'user' when calling UserApi.update_user"
end
# resource path
- local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s).gsub('%2F', '/'))
+ local_var_path = '/user/{username}'.sub('{' + 'username' + '}', CGI.escape(username.to_s))
# query parameters
query_params = opts[:query_params] || {}
Perform git status
On branch bug/overwrittenDefaultOutputDirNullPointer
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: samples/client/petstore/ruby/lib/petstore/api/pet_api.rb
modified: samples/client/petstore/ruby/lib/petstore/api/store_api.rb
modified: samples/client/petstore/ruby/lib/petstore/api/user_api.rb
modified: samples/openapi3/client/petstore/ruby/lib/petstore/api/pet_api.rb
modified: samples/openapi3/client/petstore/ruby/lib/petstore/api/store_api.rb
modified: samples/openapi3/client/petstore/ruby/lib/petstore/api/user_api.rb
no changes added to commit (use "git add" and/or "git commit -a")
Please run 'bin/utils/ensure-up-to-date' locally and commit changes (UNCOMMITTED CHANGES ERROR) |
|
@Fjolnir-Dvorak Thanks for letting me know. I'll check it. |
|
@Fjolnir-Dvorak Fmm... Currently, the Ruby client generation uses |
|
@Fjolnir-Dvorak Sorry for late reply. (BTW, this degradation seems to have happened in 7534df4 ) |
|
We'd discussed this issue in Gitter. The ruby client uses a json config during generation. Because of how this is deserialized, defaults will need to be in the type constructor and not the builder constructor. It seems this might be as simple as duplicating the default assignment in both. |
|
Ok, so I will forbid null values (without a possibility to force a null if it wanted or needed somehow) on the Dao. Null equals default then. Will add this change today or tomorrow |
|
Sounds like that should work. For reference, here's where that flattened json structure is hydrated: In the future, we might want to change this structure to match the workflow/generator separation. The JSON structure as it stands allows a user to specify global settings, workflow/generator settings, and additional properties all at the root level of the object. |
|
I tested it locally and no longer experience the NPE. If no further feedback on this PR, I'll merge it tomorrow (Wed) |
|
@wing328 can you verify whether the ruby client output had changed as I've posted above? I'm wondering if something is off locally for me. If output is different, feel free to merge and I can look at a fix later. |
|
@jimschubert you're right still an issue. Let me dig deeper this weekend. |
|
Oh, sorry, I wanted to move the defaults into the data class. Will do it today |
…ings. Defaults cannot be bypassed and the builder takes the default values from WorkflowSettings as initial values. Every value in WorkflowSettings is defaultable now. Defaults are public so these can be used for consistency and documentation purposes in (e.g.) the flags.
… meddle with fields/functions/classes which are private.
4 tasks
|
Thanks for this PR. While doing a thorough review based on the ruby template changes and discussions in Slack, I found a few other issues. I've included your work and fixes for those other issues in #3752 |
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.
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif 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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.master,4.1.x,5.0.x. Default:master.Description of the PR
A builder should not have primitive not nullable fields if there are fields containing default which are not nullable.
The main issue I migrated the default value setter to the builder is that fields can be nullable but have set a notnull default. If null is an indicator whether to set the value or not those values could never be set to null.
Fixed Issues:
Technical Commitee for Java:
@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04)