Skip to content

[BUG] Change default value for header parameter#15990

Merged
wing328 merged 3 commits intoOpenAPITools:masterfrom
gcatanese:header-default-value-null-string
Jul 7, 2023
Merged

[BUG] Change default value for header parameter#15990
wing328 merged 3 commits intoOpenAPITools:masterfrom
gcatanese:header-default-value-null-string

Conversation

@gcatanese
Copy link
Copy Markdown
Contributor

@gcatanese gcatanese commented Jul 3, 2023

Fix #15989

When a property has no default value the string "null" is used as default.

This PR changes the getPropertyDefaultValue for strings from "null" to "" (empty string)

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.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Jul 4, 2023

this fix is mainly for the postman collection generator, right?

if that's the case, I don't think we should update default codegen.

What about overriding the method in postman collection generator instead?

@gcatanese
Copy link
Copy Markdown
Contributor Author

Yes, that's an option but this applies to all generators IMO. When a parameter has no default the default codegen provides "null" string as default, for each type. Event for strings the default "null" is not a great option.
Looking at the comments in the code it looks to me they are supposed to be addressed at some point.

My suggestion would be to return null instead of the "null" string for each type. That's better as it respects the OpenAPI specs (if it doesn't specify a default value why would we generate one?) and let the generators decide what to do.
Let me know what you think, I can refactor in this way (return null) or go for the custom Postman fix.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Jul 5, 2023

Looking at the comments in the code it looks to me they are supposed to be addressed at some point.

The comment simply said the code block is to illustrate how to handle default value for different type so that generators can override this method easily.

When a parameter has no default the default codegen provides "null" string as default, for each type

The default codegen was created primarily for java client/server generators so it won't meet the requirement of generators in different languages. That's why generators should override toDefaultValue instead.

This PR changes the getPropertyDefaultValue for strings from "null" to "" (empty string)

I don't think the header parameter should default to "" (empty string) in most generators as the header with "" as the default value will now become

x-just-a-header: 

as opposed to simply omitting the header in the HTTP request.

Updating something in default codegen requires lots of tests and reviews and in this case I would suggest simply overriding toDefaultValue to meet your requirement in the postman collection generator.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Jul 5, 2023

FYI. I've fled #16003 to clean up the code a bit to avoid confusions.

@gcatanese gcatanese closed this Jul 5, 2023
@gcatanese gcatanese force-pushed the header-default-value-null-string branch from 9bfb8e6 to b465d88 Compare July 5, 2023 17:52
@gcatanese gcatanese reopened this Jul 5, 2023
@gcatanese
Copy link
Copy Markdown
Contributor Author

gcatanese commented Jul 5, 2023

@wing328 thanks for the clarifications.

The issue is that the default value "null" does not omit the header, and instead sends

x-just-a-header: "null"

I thought that would be an issue for most generators.

Anyway, I followed your suggestion and dealt with this within the Postman generator, please see the changes.

@wing328 wing328 added this to the 7.0.0 milestone Jul 7, 2023
@wing328 wing328 merged commit 315fbff into OpenAPITools:master Jul 7, 2023
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.

[BUG] Header default value set to "null"

2 participants